Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Console] RunInvokableCommand viaexecute() method.#60353
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
carsonbot commentedMay 6, 2025
It looks like you unchecked the "Allow edits from maintainer" box. That is fine, but please note that if you have multiple commits, you'll need to squash your commits into one before this can be merged. Or, you can check the "Allow edits from maintainers" box and the maintainer can squash for you. Cheers! Carsonbot |
__construct()friendly ping@yceruto |
__construct()__construct()Can you elaborate a bit on why the upcoming changes for 7.3 are causing issues for Laravel and how the proposed change would fix it? Right now, I have some trouble understanding why this is necessary. |
Given the following class on a basic Laravel installation: <?phpnamespaceApp\Console\Commands;useIlluminate\Console\Command;useIlluminate\Contracts\Config\RepositoryasConfigRepository;class HelloWorldCommandextends Command{/** * The name of the console command. * * @var string */protected$name ='app:hello-world';/** * The console command description. * * @var string */protected$description ='Command description';/** * Execute the console command. */publicfunction__invoke(ConfigRepository$config) {$this->components->info($config->get('app.name'));returnself::SUCCESS; }} Result on Symfony 7.2
Result on Symfony 7.3
|
Since |
| @@ -134,9 +134,7 @@ public function __construct(?string $name = null) | |||
| $this->setHelp($attribute?->help ?? ''); | |||
| } | |||
| if (\is_callable($this)) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I wonder if the fix instead could be to additionally check here that theexecute() method is not overridden.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In the example,App\Console\Commands\HelloWorldCommand doesn't have anexecute() method, onlyIlluminate\Console\Command has it.
It would be nice if Symfony introduce an interface to handle invokable command (as this is a new feature for Symfony 7.3) so it doesn't rely onis_callable().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
For the check I had in mind it doesn't really matter if it's an intermediate class or the actual command class that implementsexecute(). We would just ensure that the class declaring the method is not Symfony'sCommand class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It would be nice if Symfony introduce an interface to handle invokable command (as this is a new feature for Symfony 7.3) so it doesn't rely on is_callable().
An interface is not desired on our side as we want commands to be any callable really.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In the example,
App\Console\Commands\HelloWorldCommanddoesn't have an execute() method, onlyIlluminate\Console\Commandhas it.
This confirms that theexecute() method is overridden inIlluminate\Console\Command.
I wonder if the fix instead could be to additionally check here that the execute() method is not overridden.
I agree with this solution, here's an implementation.
if (\is_callable($this) && (new \ReflectionMethod($this,'execute'))->getDeclaringClass()->name !==self::class) {$this->code =newInvokableCommand($this,$this(...));}
@crynobone The solution you propose solves the issue you raise, but creates a new one because it introduces a newprotected method tied to an implementation detail and becomes a maintenance constraint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice, happy if anyone take over and make a PR for that as I will only be able to work on it much later today/tomorrow. Thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I agree to add only && (new \ReflectionMethod($this, 'execute'))->getDeclaringClass()->name !== self::class to this line, that does solve the original issue, right? please also add a new test around this case, thanks!
Alternatively wouldn't it make more sense to resolve/invoke $this->code from execute() method, otherwise throw LogicException?
I don’t think it’s a good idea: invokable code detection should happen as early as possible due to its configuration in thegetNativeDefinition() method (that’s why we perform this check in the constructor). Also, moving thecode invocation to theexecute() method will break the priority behavior, and thus affect some apps relying on->setCode() having higher priority overexecute() (which is currently the case).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My suggestion mainly moving changes fromrun() toexecute(). I don't see how that would make a different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I opened#60361
The value of$this->code influences how the command definition is generated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks@GromNaN
089ea87 tof4503b6Compare__construct()InvokableCommand viaexecute() method.carsonbot commentedMay 6, 2025
It looks like you unchecked the "Allow edits from maintainer" box. That is fine, but please note that if you have multiple commits, you'll need to squash your commits into one before this can be merged. Or, you can check the "Allow edits from maintainers" box and the maintainer can squash for you. Cheers! Carsonbot |
Let's close here in favor of#60361 Thanks@crynobone for reporting it and suggest a solution! |
…ority over `__invoke()` (GromNaN)This PR was merged into the 7.3 branch.Discussion----------[Console] Ensure overriding `Command::execute()` keeps priority over `__invoke()`| Q | A| ------------- | ---| Branch? | 7.3| Bug fix? | yes| New feature? | no| Deprecations? | no| Issues | Replace#60353| License | MITImplements#60353 (comment)Commits-------f77c403 Ensure overriding Command::execute() keep priority over __invoke
Uh oh!
There was an error while loading.Please reload this page.
Laravel Framework always allows handling a command either via
handle()method or__invoke()method. The changes made for Symfony 7.3 will create an issue on existing Laravel applications on Laravel 11 and 12 when version 7.3.0 becomes available.Having the changes in this PR will allow Laravel to override
configureAsInvokableCommand()to keep the current behavior.Given the following class on a basic Laravel installation:
Result on Symfony 7.2
Result on Symfony 7.3