magento / architecture

A place where Magento architectural discussions happen
274 stars 154 forks source link

Add design document for integration with Symfony Command Loader #408

Open mattwellss opened 4 years ago

mattwellss commented 4 years ago

This has also been discussed in the Magento application repository: https://github.com/magento/magento2/issues/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!

mattwellss commented 4 years ago

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:

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:

The earliest commit I can find with commands used the Zend service manager: https://github.com/magento/magento2/commit/2f2e4ecca26b907d3f15a25b14e69b63c65fb8a8#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?