-
Notifications
You must be signed in to change notification settings - Fork 154
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add design document for integration with Symfony Command Loader #408
Open
mattwellss
wants to merge
2
commits into
magento:master
Choose a base branch
from
mattwellss:feature/symfony-command-loader-integration
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,128 @@ | ||
# Overview | ||
|
||
In order to maximize the speed of the Magento CLI app, extraneous processing should be minimized. Magento ships with multiple commands which require significant processing to determine their "definitions" (`\Magento\Setup\Console\Command\ConfigSetCommand` is noteworthy). Rather than spend effort minimizing the impact of each of these commands, we can introduce Symfony's concept of lazy loaded commands. When a command is added to the application this way, it isn't initialized until it's needed (either to use the command or as part of `list`). | ||
|
||
## Terms | ||
|
||
- Magento CLI app: a Symfony Console application (subclassed in this case) | ||
- Magento command: a class which extends a Symfony Console Command class, to be used in the Magento CLI app | ||
- definition: the set of [input arguments and options](https://symfony.com/doc/4.4/console/input.html) for a command | ||
- lazy loaded command: A command which isn't initialized until it is needed. See [Symfony's docs](https://symfony.com/doc/4.4/console/lazy_commands.html) for more | ||
- Command Loader: an instance of `\Symfony\Component\Console\CommandLoader\CommandLoaderInterface` | ||
|
||
## Problem | ||
|
||
_For initial discussion, see https://github.com/magento/magento2/issues/29266_ | ||
|
||
### Summary | ||
A Magento command added via `\Symfony\Component\Console\Application::add` must be initialized beforehand, including its definition. For some commands, this requires significant processing. | ||
|
||
### Examples | ||
|
||
Both `\Magento\Setup\Console\Command\InstallCommand` and `\Magento\Setup\Console\Command\ConfigSetCommand` collect the full list of modules, probe them for ConfigOptionsList di configs, and add all discovered options. None of this work is cached, so it happens every time the Magento CLI app is invoked. | ||
|
||
|
||
## Design | ||
|
||
To solve this problem, I propose that the Magento CLI app uses an instance of Symfony's `\Symfony\Component\Console\CommandLoader\CommandLoaderInterface` to provide access to commands which would otherwise require initialization overhead. | ||
|
||
### Implementation Challenges | ||
|
||
#### DRYness issues | ||
|
||
One candidate for direct usage in the Magento CLI app is the `FactoryCommandLoader`. It is provided with an associative array of commands with command names as keys and command initializers as values. The obvious issue with this option is that command names are generally not defined in Magento commands in a statically-available way (such as `const NAME = 'cache:flush;`). As such, command names must be repeated in string literals. | ||
|
||
#### Backwards compatibility | ||
|
||
At this moment, Magento commands are added to the Magento CLI app via `\Magento\Framework\Console\CommandListInterface`. This interface will not suffice for use with lazy loaded commands because its output is initialized commmands. That means either new functionality for adding lazy-loaded commands is required OR a major BC break will occur. | ||
|
||
#### Maintainability | ||
|
||
If new functionality is added to support lazy loaded commands in addition to the current system, both of these systems must be maintained. | ||
|
||
#### Lack of Integration points | ||
|
||
The current structure of the Magento CLI app does not provide an obvious place for either injecting or creating its own Command Loader. All Magento commands are currently added during the Magento CLI app's `init` method, which calls `getDefaultCommands`, which calls other methods, all of which return Magento command instances. It's not ideal to mutate the Magento CLI app inside a getter, so initializing a Command Loader there is not ideal. | ||
|
||
### Proposed implementation | ||
|
||
#### New Class: MagentoCommandLoader | ||
|
||
The exact structure of `$commands` is undecided. For simplicity, this proposal assumes an associative array of Magento command names to classnames. | ||
|
||
```php | ||
namespace Magento\Framework\Console; | ||
|
||
use Symfony\Component\Console\CommandLoader\CommandloaderInterface; | ||
|
||
class MagentoCommandloader implements CommandLoaderInterface | ||
{ | ||
private $commands; | ||
private $obj; | ||
|
||
public function __construct(ObjectManagerInterface $obj, array $commands = []) | ||
{ | ||
$this->commands = $commands; | ||
$this->obj = $obj; | ||
} | ||
|
||
public function get($name) | ||
{ | ||
$class = $this->commands[$name]; | ||
return $this->obj->get($class); | ||
} | ||
|
||
public function has($name) | ||
{ | ||
return isset($this->commands[$name]); | ||
} | ||
|
||
public function getNames() | ||
{ | ||
return array_keys($this->commands); | ||
} | ||
} | ||
``` | ||
|
||
#### Connection to Magento CLI app | ||
|
||
Next, the Command Loader must be initialized and attached to the Magento CLI app: | ||
```php | ||
|
||
//inside \Magento\Framework\Console\Cli::__construct | ||
|
||
try { | ||
// ... | ||
$this->initObjectManager(); | ||
} catch (\Exception $exception) { | ||
// ... | ||
} | ||
|
||
parent::__construct($name, $version); | ||
// ... | ||
$this->logger = $this->objectManager->get(LoggerInterface::class); | ||
|
||
// NEW CODE! | ||
$this->setCommandLoader($this->objectManager->get(MagentoCommandloader::class, [ | ||
'obj' => $this->objectManager | ||
])); | ||
``` | ||
|
||
#### DI configuration | ||
At this point, when a Magento command's initialization should be deferred for the sake of speed, it can be added via the `MagentoCommandLoader` in DI: | ||
|
||
```xml | ||
<type name="MagentoCommandLoader"> | ||
<arguments> | ||
<argument name="commands" xsi:type="array"> | ||
<item name="a:heavy:command" xsi:type="string">My\Module\Commands\HeavyCommand</item> | ||
</argument> | ||
</arguments> | ||
</type> | ||
``` | ||
|
||
#### Notes | ||
|
||
* The current `CommandList` class and related DI configuration will be left alone, except where Magento commands must be lazily loaded | ||
* As mentioned, the structure of the `$commands` parameter of `MagentoCommandLoader` is just one idea. I'm not particularly attached to it | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 would suggest removing
Magento
from the class name, as it already a part of the namespace.