Skip to content
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
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mattwellss
Copy link

This has also been discussed in the Magento application repository: magento/magento2#29266

Problem

Several console commands require significant processing to determine their definitions, reducing the overall performance of Magento's CLI even when they aren't used.

Solution

Integrate Symfony\Component\Console\CommandLoader\CommandLoaderInterface with Magento's CLI application, allowing commands to be added without being instantiated.

Requested Reviewers

@joni-jones, @maghamed

Thanks for looking!


use Symfony\Component\Console\CommandLoader\CommandloaderInterface;

class MagentoCommandloader implements CommandLoaderInterface
Copy link
Contributor

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.

@mattwellss
Copy link
Author

Thanks for the review, @joni-jones. I'll make that change.

A couple questions for you:

Regarding Interfaces

Should I assume there needs to be a Magento-namespaced interface? For example:

namespace Magento\Framework\Console;
interface CommandLoaderInterface extends \Symfony\Component\Console\CommandLoader\CommandLoaderInterface {}

I don't believe it would need to define any of its own methods. Everything necessary is already provided by the parent interface.

Otherwise, we'll end up with classes directly implementing the CommandLoaderInterface. This seems OK to me, because it's how console commands already work.

Regarding Command Instantiation

CommandList classes use both Magento's object manager and the Laminas service manager to intantiate commands:

  • Magento\Framework\Console\CommandList takes an array of initialized commands in its __construct. These basically always come from di.xml configuration, which uses the object manager
  • Magento\Setup\Console\CommandList keeps an internal list of commands, and instantiates them via a Laminas service manager

Because commands are created by two different means, and because Symfony applications can only have a single Command Loader, it follows that we might need to maintain multiple implementations of Command Loaders:

  • Magento\Framework\Command\CommandLoader: uses object manager to instantiate commands on-demand
  • Magento\Setup\Console\CommandLoader: uses laminas service manager to instantiate commands on-demand
  • Magento\Framework\Command\CommandLoader\Aggregate: iterates over multiple command loaders to find the requested command

The earliest commit I can find with commands used the Zend service manager: magento/magento2@2f2e4ec#diff-3b4aa36093da63881b26624512aaa8e0R49, so the continuing use of the Laminas version may be an historical artifact, but I'm not certain.

Do you know if we need to maintain two separate instantiation paths for Commands? If so, does my delineation of the types of CommandLoaders seem correct?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants