Stop using Drupal container for contrib command discovery and instantiation

[weitzman]

Use Factory method ...

[greg-1-anderson]

The general idea here is that we find command classes by discovery, like we used to do (and still do for site-wide commands). We could also consider discovering command classes via the autoloader, as Robo plugins do.

Once we have a list of command classes, we instantiate them via a static create factory method (that must exist? or only if it exists?). The create method is passed a DI container, and it can use that to fetch and inject its dependencies into the commandfile class.

League/container supports chained DI containers. Symfony DI did not; I'm not sure if it does now, but I suspect we might be able to chain from a League DI container to a Symfony (Drupal) DI container. If not, we might need to pass two DI containers to the create method.

[weitzman]

Looks like neither the Symfony container nor the Drupal container (a close fork of Symfony) support delegate() so we'll need to pass two containers I think.

[greg-1-anderson]

Symfony containers implement ContainerInterface, which is the requirement for deletage() in league / container, so I think that maybe we could delegate to the Drupal container from the Drush container.

We should only do this if we change our bootstrap process to do the delegation; if we don't do that, then we should pass two containers.

Also, just because we can pass just one container doesn't mean we should. I'm undecided on this point.

[weitzman]

Could we do this in a backward compat way? Seems like we could mimic service instantiation so they continue to work?

[greg-1-anderson]

Hm, yeah, that might work.

[weitzman]

A bit more discussion about motivation for this (or lack thereof) https://drupal.slack.com/archives/C62H9CWQM/p1633443431221800?thread_ts=1633433650.221500&cid=C62H9CWQM

[joachim-n]

It would be really nice if Drush looked for commands in all installed packages, not just modules.

There's a comment in DrupalKernelTrait::discoverServiceProviders() about this.

I think it's doable by instantiating Composer and getting a list of packages from it (see https://github.com/composer/composer/issues/4445), but is the command list cached at all? Getting a list of packages out of Composer every single time is going to be a bit slow.

[Seldaek]

If you need a list of packages, please use https://getcomposer.org/doc/07-runtime.md#installed-versions - there's also getInstalledPackagesByType which could be useful for this perhaps if the packages have defined types.

[claudiu-cristea]

@joachim-n right now commands are discoverable in packages but they need to follow the these rules https://github.com/drush-ops/drush/blob/11.x/docs/commands.md#auto-discovered-commands

[weitzman]

@Seldaek - thats a helpful new feature and we will use it soon, now that most people are on Composer 2 (which is great!).

[Seldaek]

Nice, don't forget to require the appropriate composer-runtime-api package to avoid confusing people still running old composer versions (which tends to happen a lot accidentally it seems)

[weitzman]

Oh, I forgot that we only need to use the Composer API for loading global packages, and we can use an existing discovery method for commands that ship in modules.

~~One thing we havent discussed here is how we will avoid loading commands that belong to uninstalled modules. Do we load the command if ($outside_docroot || $path_is_under_an_enabled_module)?~~

[joachim-n]

@claudiu-cristea That looks great, thank you!

[weitzman]

Seems like we could experiment with command lazy loading at same time https://symfony.com/doc/current/console/lazy_commands.html

[weitzman]

I got curious about how Laravel auto-discovers Service Providers (analogous to command providers here). See https://divinglaravel.com/laravels-package-auto-discovery. Seems like that composer-centric discovery approach could work for us.

Sources:

• https://github.com/laravel/framework/blob/9.x/src/Illuminate/Foundation/Console/PackageDiscoverCommand.php which calls into https://github.com/laravel/framework/blob/9.x/src/Illuminate/Foundation/PackageManifest.php#L114-L138

[weitzman]

Cake does similar where composer writes a vendor/cakephp-plugins.php file. See https://book.cakephp.org/4/en/plugins.html

[weitzman]

This was largely done in #5548. We could explore the service providers approach and Lazy Commands in future issues.

[greg-1-anderson]

Enhancing the psr4 discovery mechanism to use the Composer API instead is a good idea. This reduces the utility a little (in the existing mechanism, folks can add a second autoload location to put their commands somewhere else in the source tree), but I think it also simplifies things a bit.

[weitzman]

OK, reopening so we dont lose it.