feat: ability generate withSchedule for boostrap/app.php file#43
feat: ability generate withSchedule for boostrap/app.php file#43AZabolotnikov wants to merge 61 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds functionality to generate withSchedule method calls for Laravel's bootstrap/app.php file, enabling automated addition of scheduled commands. This feature complements the existing addExceptionsRender functionality in the AppBootstrapBuilder.
Changes:
- Added new
AddScheduleCommandvisitor to handle schedule command insertion with environment support - Extended
AppBootstrapBuilderwithaddScheduleCommandmethod for public API - Added comprehensive test coverage including edge cases for empty schedules, existing schedules, and combined operations
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Visitors/AppBootstrapVisitors/AddScheduleCommand.php | New visitor implementing schedule command insertion logic with support for creating withSchedule blocks when missing |
| src/Builders/AppBootstrapBuilder.php | Added public addScheduleCommand method and Schedule facade import handling |
| tests/AppBootstrapBuilderTest.php | Added three test cases covering empty schedule, existing schedule, and combined render scenarios |
| tests/fixtures/AppBootstrapBuilderTest/results/schedule.php | Expected output for adding schedule commands to empty bootstrap file |
| tests/fixtures/AppBootstrapBuilderTest/results/schedule_exists.php | Expected output for adding commands to existing withSchedule block |
| tests/fixtures/AppBootstrapBuilderTest/results/combine_render.php | Expected output for combining exception renders and schedule commands |
| tests/fixtures/AppBootstrapBuilderTest/original/schedule_exists.php | Original bootstrap file with empty withSchedule block for testing |
| src/Visitors/AppBootstrapVisitors/AbstractAppBootstrapVisitor.php | Added array type hint to FORBIDDEN_NODES constant for PHP 8.3+ compatibility |
| src/Visitors/AddArrayPropertyItem.php | Removed unused $propertyType variable from destructuring assignment |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Coverage Report for CI Build 25510549258Coverage decreased (-0.2%) to 98.361%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
# Conflicts: # src/Visitors/PropertyVisitors/AbstractPropertyVisitor.php
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Singleton is not suitable in this case because it guarantees that only one instance of a class exists. In our scenario, there is a need to create multiple objects (visitors) of the same class, each having its own independent state. Using a singleton would force all consumers to share the same instance, which could lead to incorrect behavior. |
|
Waiting for changes from this request: #51 |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96d468aba3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| $this->addImports([ | ||
| 'Illuminate\Support\Facades\Schedule', | ||
| ]); |
There was a problem hiding this comment.
Schedule unconditionally
addScheduleCommand() always adds Illuminate\Support\Facades\Schedule, but withSchedule() is commonly written with use Illuminate\Console\Scheduling\Schedule; for the callback parameter. Adding both imports creates a duplicate Schedule alias and the generated bootstrap/app.php will fail to parse (name is already in use) before the app can boot. This breaks valid existing bootstrap files that already follow Laravel's typed callback style.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The library provides removeImports() for exactly this case. If your bootstrap/app.php already has use Illuminate\Console\Scheduling\Schedule, chain removeImports() before addScheduleCommand() to drop the conflicting import first.
| $value = match ($type) { | ||
| 'int' => new Int_($value), | ||
| 'array' => $this->makeArrayValue($value), | ||
| 'string' => new String_($value), | ||
| 'float' => new Float_($value), |
There was a problem hiding this comment.
Schedule methods such as when / skip require callable arguments, but argument conversion only handles scalars, arrays, booleans, and null. Passing a closure (e.g. new ScheduleOption('when', [fn () => true])) now throws an UnhandledMatchError during generation, so a supported scheduling method cannot be expressed safely through this API.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Closure-based methods like when/skip should be passed as raw code strings, they are inserted into the file as-is without AST transformation. Example:
new ScheduleOption('when', ['fn () => User::count() > 0'])
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 821815bb17
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5d10db692
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| $this->addImports([ | ||
| 'Illuminate\Support\Facades\Schedule', | ||
| ]); |
There was a problem hiding this comment.
Adding Illuminate\Support\Facades\Schedule unconditionally can make generated bootstrap/app.php fail to parse when the file already imports Illuminate\Console\Scheduling\Schedule for the withSchedule callback parameter, because both resolve to the Schedule alias. This happens in valid Laravel bootstrap files and stops app boot with a duplicate import name error. Fresh evidence: in this commit tree there is no removeImports() API (rg "removeImports" returns no matches), so the suggested thread workaround is not available here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ada00c972
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| $methods = array_merge( | ||
| $this->getMethods(ManagesAttributes::class), | ||
| $this->getMethods(ManagesFrequencies::class), | ||
| $this->getMethods(Event::class), | ||
| ); |
There was a problem hiding this comment.
ScheduleOption::validateMethod() currently whitelists every public method on Event, which includes non-fluent accessors like getSummaryForDisplay() / nextRunDate(). AddScheduleCommand then blindly chains all selected options, so a valid option sequence can generate calls like Schedule::command(...)->getSummaryForDisplay()->daily(), which fails at runtime because the accessor no longer returns an event object. Limiting allowed methods to fluent scheduling configurators avoids generating invalid chains that crash when the scheduler is loaded.
Useful? React with 👍 / 👎.
refs: https://github.com/RonasIT/laravel-telescope-extension/issues