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.