Support for Symfony's CommandLoaderInterface in Magento CLI

[mattwellss]

Description (*)

Magento applications come out of the box with a few particularly "heavy to initialize" CLI commands. Two examples: \Magento\Setup\Console\Command\InstallCommand and \Magento\Setup\Console\Command\ConfigSetCommand. Each of these probes all installed modules for ConfigOptionsList classes, initializes them, and reads options from them. That work is not necessary unless the user is interacting with either these commands, though it's currently necessary due to the fact that each command added directly to a Symfony Console app must have a Definition.

Expected behavior (*)

Without going too far into designing the implementation, I'd like to see an alternative to the \Magento\Framework\Console\CommandListInterface which defines a return value similar to that of the \Symfony\Component\Console\CommandLoader\CommandLoaderInterface. I'd additionally like to see Magento implement this alternative Command loading technique especially for time-consuming commands such as those listed in the description.

Benefits

Currently, a command must be "fully initialized" to be ready to add to Magento. This means it must be instantiated at runtime, along with its dependencies, definition, etc. In order to minimize the impact of new commands, extra care must be taken to use factories and proxies in dependencies.

If new commands can be added to Magento's CLI app without being fully initialized, implementors can be free to focus on the features.

Additional information

If this is a desired change, I'd be happy to request time from my employer to implement it and contribute it to the community. Thanks for considering it!

[m2-assistant]

Hi @mattwellss. Thank you for your report.
To help us process this issue please make sure that you provided the following information:

• Summary of the issue
• Information on your environment
• Steps to reproduce
• Expected and actual results

Please make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, please, add a comment to the issue:

@magento give me 2.4-develop instance - upcoming 2.4.x release

For more details, please, review the Magento Contributor Assistant documentation.

Please, add a comment to assign the issue: @magento I am working on this

---

• Join Magento Community Engineering Slack and ask your questions in #github channel.

⚠️ According to the Magento Contribution requirements, all issues must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of issues happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

[maghamed]

hey @mattwellss,

I think you are right that we currently have unnecessary computation here

namespace Magento\Setup\Model;

/**
 * Object manager provider
 *
 * Links Zend Framework's service locator and Magento object manager.
 * Guaranties single object manager per application run.
 * Hides complexity of creating Magento object manager
 */
class ObjectManagerProvider
{
   //..

    /**
     * Retrieve object manager.
     *
     * @return ObjectManagerInterface
     * @throws \Magento\Setup\Exception
     */
    public function get()
    {
        if (null === $this->objectManager) {
            $initParams = $this->serviceLocator->get(InitParamListener::BOOTSTRAP_PARAM);
            $factory = $this->getObjectManagerFactory($initParams);
            $this->objectManager = $factory->create($initParams);
            if (PHP_SAPI == 'cli') {
                $this->createCliCommands();
            }
        }
        return $this->objectManager;
    }

    /**
     * Creates cli commands and initialize them with application instance
     *
     * @return void
     */
    private function createCliCommands()
    {
        /** @var CommandListInterface $commandList */
        $commandList = $this->objectManager->create(CommandListInterface::class);
        $application = $this->serviceLocator->get(Application::class);
        foreach ($commandList->getCommands() as $command) {
            $application->add($command);
        }
    }

Loading all commands $commandList->getCommands() even though we don't use them. Briefly grep-ping the codebase I saw that there are more than 40 different commands there, and of cause, each of them has its own list of external dependencies that need to be initialized first.

As a quick way how we can resolve the performance issue - we can walk through all the occurrences in DI.xml and instead of real objects pass proxies of those objects to the CommandListInterface. Something like

    <type name="Magento\Framework\Console\CommandListInterface">
        <arguments>
            <argument name="commands" xsi:type="array">
                <item name="startConsumerCommand" xsi:type="object">Magento\MessageQueue\Console\StartConsumerCommand\Proxy</item>
                <item name="consumerListCommand" xsi:type="object">Magento\MessageQueue\Console\ConsumerListCommand\Proxy</item>
            </argument>
        </arguments>
    </type>

It should solve the existing performance issue we have now, and such an approach should be backward compatible so that it would be possible to release it in the scope of the next Magento patch release.

Regarding the part where you proposed to introduce a new Interface, similar to CommandLoaderInterface - we need to check it more thoroughly. If I got you right, you propose to use a method similar to getNames() which would save the time on costly initialization of all commands when we need to return just a list of all available command names:

interface CommandLoaderInterface
{
    /**
     * Loads a command.
     *
     * @param string $name
     *
     * @return Command
     *
     * @throws CommandNotFoundException
     */
    public function get($name);

    /**
     * Checks if a command exists.
     *
     * @param string $name
     *
     * @return bool
     */
    public function has($name);

    /**
     * @return string[] All registered command names
     */
    public function getNames();
}

But most probably this change would affect the semantic of how the commands are being registered to the command list.
Something like

    <type name="Magento\Framework\Console\CommandListInterface">
        <arguments>
            <argument name="commands" xsi:type="array">
                <item name="configSetCommand" xsi:type="array">
                    <item name="className" xsi:type="array">Magento\Config\Console\Command\ConfigSetCommand</item>
                    <item name="name" xsi:type="string">CommandName</item>
            </argument>
        </arguments>
    </type>

And this might lead to Backward Incompatible changes. Unless, we will be using "names" being passed as attributes right now, which is not the case for several commands.
Here we need to consider in more details how possibly we can mitigate the impact

[maghamed]

regarding new interface(-s) proposal and how it should be incorporated into existing code I recommend to create appropriate proposal on architectural GitHub repo - https://github.com/magento/architecture

[mattwellss]

Proxies were my initial thought as well. However, (and maybe I'm misusing it), whenever I try to convert a command into a proxied command, the Laminas service manager throws this exception:

An abstract factory could not create an instance of magentosetupconsolecommandadminusercreatecommandproxy(alias: Magento\Setup\Console\Command\AdminUserCreateCommand\Proxy).   

Also, when a command is added as happens currently, the proxy benefit is lost when getDefinition, getName, etc is called:

public function add(Command $command)
{
    $this->init();

    $command->setApplication($this);

    if (!$command->isEnabled()) {
        $command->setApplication(null);

        return null;
    }

    // Will throw if the command is not correctly initialized.
    $command->getDefinition();

    if (!$command->getName()) {
        throw new LogicException(sprintf('The command defined in "%s" cannot have an empty name.', \get_class($command)));
    }

    $this->commands[$command->getName()] = $command;

    foreach ($command->getAliases() as $alias) {
        $this->commands[$alias] = $command;
    }

    return $command;
}

---

edit: this is not where my CLI app gets a list of commands...

It happens in \Magento\Framework\Console\Cli::getDefaultCommands (which calls getApplicationCommands). There's no particularly "nice" way to configure a command loader this way. Here's a way that works, though it messes with the purity of the function:

    protected function getApplicationCommands()
    {
        /** @var CommandLoaderInterface $commandLoader */
        $commandLoader = $this->objectManager->create(MagentoCommandLoader::class, [
            'serviceManager' => $this->serviceManager
        ]);
        $this->setCommandLoader($commandLoader);

        $commands = [];

Everything from the start of the function body up to $commands = []; is new. As can be seen, I'm calling a setter inside a "get"-style function. Not great!

end edit

In order to maintain BC, I'd suggest this small change to the ObjectManagerProvider:

private function createCliCommands()
{
    /** @var CommandListInterface $commandList */
    $commandList = $this->objectManager->create(CommandListInterface::class);
    /** @var Application $application */
    $application = $this->serviceLocator->get(Application::class);
    foreach ($commandList->getCommands() as $command) {
        $application->add($command);
    }

    // Here's the new line. MagentoCommandLoader is the imaginary class I made up
    $application->setCommandLoader($this->objectManager->create(MagentoCommandLoader::class));
}

And then any console commands wishing to be loaded lazily can be DI'd into the MagentoCommandLoader

<type name="MagentoCommandLoader">
    <arguments>
        <argument name="commands" xsi:type="array">
            <item name="setup:config:set" xsi:type="string">Magento\Setup\Console\Command\ConfigSetCommand</item>
        </argument>
    </arguments>
</type>

The Magento Command Loader will use the same Laminas service manager as \Magento\Setup\Console\CommandList::getCommands, but only when the command is specifically requested:

// Inspired by \Symfony\Component\Console\CommandLoader\FactoryCommandLoader::get
public function get($name)
{
    $className = $this->commands[$name]; // check for valid index when this is real code
    return $this->serviceManager->get($className);
}

[maghamed]

Proxies were my initial thought as well. However, (and maybe I'm misusing it), whenever I try to convert a command into a proxied command, the Laminas service manager throws this exception:

But in some of the cases, we are already use Proxies. So, this snippet I took from real core code:

    <type name="Magento\Framework\Console\CommandListInterface">
        <arguments>
            <argument name="commands" xsi:type="array">
                <item name="startConsumerCommand" xsi:type="object">Magento\MessageQueue\Console\StartConsumerCommand\Proxy</item>
                <item name="consumerListCommand" xsi:type="object">Magento\MessageQueue\Console\ConsumerListCommand\Proxy</item>
            </argument>
        </arguments>
    </type>

And we are not dealing with Laminas Service Manager here, but rather with Magento Object manager.
All those proxies are being materialized and created in the codebase (under generated/code) when we execute setup:di:compile

Try to run di compile command and check whether those classes materialized in your project.
It should look like

<?php
namespace Magento\MessageQueue\Console\StartConsumerCommand;

/**
 * Proxy class for @see \Magento\MessageQueue\Console\StartConsumerCommand
 */
class Proxy extends \Magento\MessageQueue\Console\StartConsumerCommand implements \Magento\Framework\ObjectManager\NoninterceptableInterface
{
    /**
     * Object Manager instance
     *
     * @var \Magento\Framework\ObjectManagerInterface
     */
    protected $_objectManager = null;

    /**
     * Proxied instance name
     *
     * @var string
     */
    protected $_instanceName = null;

    /**
     * Proxied instance
     *
     * @var \Magento\MessageQueue\Console\StartConsumerCommand
     */
    protected $_subject = null;

    /**
     * Instance shareability flag
     *
     * @var bool
     */
    protected $_isShared = null;

    /**
     * Proxy constructor
     *
     * @param \Magento\Framework\ObjectManagerInterface $objectManager
     * @param string $instanceName
     * @param bool $shared
     */
    public function __construct(\Magento\Framework\ObjectManagerInterface $objectManager, $instanceName = '\\Magento\\MessageQueue\\Console\\StartConsumerCommand', $shared = true)
    {
        $this->_objectManager = $objectManager;
        $this->_instanceName = $instanceName;
        $this->_isShared = $shared;
    }

    /**
     * Get proxied instance
     *
     * @return \Magento\MessageQueue\Console\StartConsumerCommand
     */
    protected function _getSubject()
    {
        if (!$this->_subject) {
            $this->_subject = true === $this->_isShared
                ? $this->_objectManager->get($this->_instanceName)
                : $this->_objectManager->create($this->_instanceName);
        }
        return $this->_subject;
    }


    /**
     * {@inheritdoc}
     */
    public function getDescription()
    {
        return $this->_getSubject()->getDescription();
    }
   //....
}

Also, when a command is added as happens currently, the proxy benefit is lost when getDefinition, getName, etc is called:

This is true if we do that do all commands in the list because each time when we call either getName or getDescription, Magento in fact would instantiate the real object behind the proxy:

    protected function _getSubject()
    {
        if (!$this->_subject) {
            $this->_subject = true === $this->_isShared
                ? $this->_objectManager->get($this->_instanceName)
                : $this->_objectManager->create($this->_instanceName);
        }
        return $this->_subject;
    }

You proposed to define those parts (Name and Description) separately out of existing commands definition.
Currently, we have all the commands being defined through DI configuration of CommandListInterface.

    <type name="Magento\Framework\Console\CommandListInterface">
        <arguments>
            <argument name="commands" xsi:type="array">
                <item name="configSetCommand" xsi:type="object">Magento\Config\Console\Command\ConfigSetCommand</item>
                <item name="configShowCommand" xsi:type="object">Magento\Config\Console\Command\ConfigShowCommand</item>
            </argument>
        </arguments>
    </type>

Ideally to have all the configurations in one place. Because it's non-obvious that for adding a command 3rd party developer needs to set up CommandListInterface, but to specify Name/Description - he/she should be using different entity - MagentoCommandLoader and provide another entity there. So, the command would be defined by 2 different classes, which decrease the code cohesion.

Thus, I prefer to have all the command configuration in one place, like here:

    <type name="Magento\Framework\Console\CommandListInterface">
        <arguments>
            <argument name="commands" xsi:type="array">
                <item name="configSetCommand" xsi:type="array">
                    <item name="className" xsi:type="string">Magento\Config\Console\Command\ConfigSetCommand</item>
                    <item name="name" xsi:type="string">CommandName</item>
                    <item name="description" xsi:type="string">Command Description goes here</item>
            </argument>
        </arguments>
    </type>

But here we need to prevent as much as possible BiC changes for already defined commands.

[mattwellss]

And we are not dealing with Laminas Service Manager here, but rather with Magento Object manager.
All those proxies are being materialized and created in the codebase (under generated/code) when we execute setup:di:compile

Ahh, I should've been more specific... Sorry about that. Because I'm looking at setup:install and setup:config:set, I was trying to proxy ConfigSetCommand and InstallCommand from the . Both of those are instantiated by \Magento\Setup\Console\CommandList, which uses the Laminas ServiceManager rather than Magento's Object Manager.

Thanks again for your attention to this. I've started working on a document for https://github.com/magento/architecture, but it's a new process to me, so it could take a bit of time. Are there any proposals in there that you feel are particularly useful as guides (beyond what the design doc template provides)?