6.x: command refactor #18983
6.x: command refactor #18983
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the console command architecture by making Arguments and ConsoleIoInterface instance properties of commands rather than parameters passed to the execute() method. The ConsoleIoInterface becomes a required constructor dependency, while Arguments is set as a property after option parsing. This simplifies the command execution flow by eliminating the need to pass these mandatory dependencies through method signatures.
Key changes:
- The
execute()method signature no longer requiresArgumentsandConsoleIoInterfaceparameters - Commands access
$this->argsand$this->ioproperties instead of method parameters CommandCollectionnow only accepts class strings, not command instances
Reviewed Changes
Copilot reviewed 58 out of 58 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Console/BaseCommand.php | Core refactoring: adds $args and $io as properties, updates run() to set these before calling execute() |
| src/Console/CommandInterface.php | Updates run() signature to make $io optional with default null |
| src/Console/CommandFactory.php | Adds $io parameter to create() method and registers it in container |
| src/Console/CommandRunner.php | Updates command creation and execution to pass $io during instantiation |
| src/Console/CommandCollection.php | Restricts storage to class strings only, removes support for command instances |
| src/Command/*.php | Updates all command implementations to use $this->args and $this->io |
| tests/test_app/**/*.php | Updates test command implementations to match new signature |
| tests/TestCase/**/*.php | Updates test setup to provide $io via constructor |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
0fefff5 to
a2a975e
Compare
|
The rector problems are not related to this PR. Will fix this in another PR for 6.x |
I definitely have apps/plugins where I add command instances ro the collection from the Application/Pugin class. |
Then we need to set both the IO instance as well as the arguments instance after the object has been created but before its being executed/run. |
a2a975e to
86b221f
Compare
|
The functionality to add command objects directly to the collection has been restored. |
|
Creating custom rector rules to make this as easy as possible for our users is going to be fun... but imho its still worth it |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 55 out of 55 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
src/Command/CompletionCommand.php:1
- Commands instantiated via reflection in
getOptions()lack the required IO instance. These commands will fail if they access$this->io. Either inject IO after instantiation or use the command factory if available.
<?php
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
e216ae0 to
1bdbc06
Compare
|
I am wondering if we can also merge /cc @markstory |
|
As we discussed in core chat the Command class has references to the ORM Package, so we should keep it as it is right now. |
| public function execute(Arguments $args, ConsoleIoInterface $io): ?int | ||
| public function execute(): ?int |
There was a problem hiding this comment.
Is this a change that we can make with rector? Converting all the command code could get tricky. 🤔
There was a problem hiding this comment.
I will try to make a custom rector for that, yes.
Removing the args here wouldn't be that hard, but making all the appearances inside that method (and sub methods) use $this->args and $this->io instead of those vars could be a bit tricky.
But then again this is also a pretty easy search-replace operation to be honest.
| /** @var int|null $result */ | ||
| $result = $this->execute($args, $io); | ||
| $this->dispatchEvent('Command.afterExecute', ['args' => $args, 'io' => $io, 'result' => $result]); | ||
| $result = $this->execute(); |
There was a problem hiding this comment.
Could save a lot of BC breaks if we continued to send args and io as parameters.
There was a problem hiding this comment.
well this whole PR is meant to cleanup the command structure. This of course comes with BC.
If you want to keep the old way for BC reasons then we can close this.
There was a problem hiding this comment.
I also think we should clean this up now, or it will never happen.
The changes to use props instead of local args should be fully automatable, I assume?
There was a problem hiding this comment.
The changes to use props instead of local args should be fully automatable, I assume?
If we can make the upgrade automated, lets go for it. I know I bang on a lot about making upgrades easy. The reason for that is that each big breaking change we introduce adds friction to upgrades. I'm on-board with making changes if we can also make the upgrade process easy.
There was a problem hiding this comment.
See attached upgrade PR. Rector is pretty insane
1bdbc06 to
9dacd81
Compare
890099a to
f3a7490
Compare
Fixes #18394
Alright, this one is a doozy.
The main thing we want to achieve here is the simple fact, that both the argument as well as the console IO instance are mandatory for commands to execute at all. So it doesn't really make sense, that both those instances are getting passed down/arround the whole console/command flow.
The Console IO instance gets created (or passed down by tests) inside the CommandRunner, so thats an easy constructor dependency.as discussed bellow there are situations, where command instances are being created manually for the Command collection, so adding it to the constructor dependencies was not an option.The Arguments instance is a bit more complicated, since it depends on the options configuration inside each command - therefore its not a constructor dependency but rather a normal property inside the class which gets set/built after
initializeis being called (maybe could be moved beforeinitializeas well)Happy to hear your feedback.