[Console] Run InvokableCommand via execute() method.
#60353
Conversation
|
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: <?php
namespace App\Console\Commands;
use Illuminate\Console\Command;
use Illuminate\Contracts\Config\Repository as ConfigRepository;
class HelloWorldCommand extends 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.
*/
public function __invoke(ConfigRepository $config)
{
$this->components->info($config->get('app.name'));
return self::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.
I wonder if the fix instead could be to additionally check here that the execute() method is not overridden.
There was a problem hiding this comment.
In the example, App\Console\Commands\HelloWorldCommand doesn't have an execute() method, only Illuminate\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 on is_callable().
There was a problem hiding this comment.
For the check I had in mind it doesn't really matter if it's an intermediate class or the actual command class that implements execute(). We would just ensure that the class declaring the method is not Symfony's Command class.
There was a problem hiding this comment.
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.
In the example,
App\Console\Commands\HelloWorldCommanddoesn't have an execute() method, onlyIlluminate\Console\Commandhas it.
This confirms that the execute() method is overridden in Illuminate\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 = new InvokableCommand($this, $this(...));
}@crynobone The solution you propose solves the issue you raise, but creates a new one because it introduces a new protected method tied to an implementation detail and becomes a maintenance constraint.
There was a problem hiding this comment.
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.
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 the getNativeDefinition() method (that’s why we perform this check in the constructor). Also, moving the code invocation to the execute() method will break the priority behavior, and thus affect some apps relying on ->setCode() having higher priority over execute() (which is currently the case).
There was a problem hiding this comment.
My suggestion mainly moving changes from run() to execute(). I don't see how that would make a different.
There was a problem hiding this comment.
I opened #60361
The value of $this->code influences how the command definition is generated.
089ea87 to
f4503b6
Compare
__construct()InvokableCommand via execute() method.
|
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 | MIT Implements #60353 (comment) Commits ------- f77c403 Ensure overriding Command::execute() keep priority over __invoke
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