Auto-discover non-Drupal commands #4696

claudiu-cristea · 2021-03-21T10:42:21Z

True, there is a way to discover such commands by adding their paths in Drush configuration. But I want to be able to have such commands available automatically, when I composer require a 3rd party library Drush commands provider.

This PR uses the Robo RelativeNamespaceDiscovery to grab the Drush commands living in 3rd party libraries, loadable via PSR4. Such a class should comply with the following requirements:

The commands class namespace ends with *\Drush\Commands, e.g. My\Custom\Library\Drush\Commands.
The class and file name ends with *DrushCommands, e.g. FooDrushCommands.

claudiu-cristea · 2021-03-21T10:51:45Z

We may want to deprecate global commands discoverable via config (?)

weitzman · 2021-03-21T10:56:09Z

LGTM. @greg-1-anderson would be right person to review and merge this.

weitzman · 2021-03-21T11:01:22Z

We should make sure that Generators can be discovered similarly, and edit docs at https://www.drush.org/latest/generators/ accordingly. #4547 might be related.

claudiu-cristea · 2021-03-21T11:33:01Z

@weitzman I see global generators are already discovered by their namespace (\Drush\Generators\*). Should we add the same logic, e.g. My\Custom\Library\Drush\Generators?

What about deprecating the old way for commands (see my previous comment) and generators (\Drush\Generators\*)?

weitzman · 2021-03-21T11:37:18Z

Should we add the same logic, e.g. My\Custom\Library\Drush\Generators

Yes, I think so.

What about deprecating the old way for commands (see my previous comment) and generators

Yes, IMO.

greg-1-anderson

This is spot on; thank you for submitting it.

I am 👍 on merging this, but holding off for a bit to see if we want to add deprecations first. Auto-discovering generators would be good for consistency, but could be follow-on work IMO.

claudiu-cristea · 2021-03-21T14:16:35Z

Generators are postponed to a followup. This is not a straight task as RelativeNamespaceDiscovery needs the class loader to grab the classes.

Consider to postpone also deprecations. In PR I'm suggesting to deprecate drush.commands discovery, which seems not documented anywhere, but can be observed here \Drush\Application::commandsFromConfiguration(). There is also drush.paths.include and this is confusing me.

greg-1-anderson · 2021-03-21T15:17:59Z

src/Application.php

+        if ($commandsFromConfig) {
+            @trigger_error("Discovering global commands by specifying them in 'drush.commands' config is deprecated and will be removed in Drush 11.0.0. See docs/commands.md on how to create auto-discoverable global commands.", E_USER_DEPRECATED);
+        }
+        $commandClasses = array_merge(


The other question about this PR is, what happens if the same commandfile is found twice by multiple methods? For example, the example-drush-extension project creates an autoloadable class that is discovered based on its location, and is probably also discovered by this PR. I am presuming that this array_merge will cause these two identical items to become a single entry in the resulting array, since they should have the same key.

Well, not all parts of the merge have keys. So, we should remove the keys > merge > array_unique(), assuming that there are no same two namespaces with different paths in this universe. That would be a nonsense and a bug somewhere.

greg-1-anderson · 2021-03-21T15:20:40Z

src/Application.php

@@ -320,8 +320,12 @@ public function configureAndRegisterCommands(InputInterface $input, OutputInterf
        $discovery = $this->commandDiscovery();
        $commandClasses = $discovery->discover($commandfileSearchpath, '\Drush');
        $commandClasses[] = \Consolidation\Filter\Hooks\FilterHooks::class;
+        $commandsFromConfig = $this->commandsFromConfiguration();
+        if ($commandsFromConfig) {
+            @trigger_error("Discovering global commands by specifying them in 'drush.commands' config is deprecated and will be removed in Drush 11.0.0. See docs/commands.md on how to create auto-discoverable global commands.", E_USER_DEPRECATED);


I am wondering if trigger_error is too noisy. Maybe we should deprecate with trigger_error in Drush 11, and remove in Drush 12? I know that's a long long ways off, but folks are very slow at upgrading Drush commands in modules.

I know that's a long long ways off, but folks are very slow at upgrading Drush commands in modules.

This is something that leads to exactly the opposite conclusion: If they're slow to adopt changes, then we should deprecate as soon as possible, giving them enough time to take action. We can change the removal version from 11.0.0 to 12.0.0 but the deprecation should occur now. Deprecating doesn't mean the command doesn't work. Also note that this is a muted deprecation @trigger_error(...), so there should be no noise in console. Normally code deprecation tests should catch this error but that's fine.

Anyway, deprecation is postponed.

greg-1-anderson · 2021-03-21T15:21:51Z

My last comment is that maybe we should note in the documentation that this new loading mechanism is experimental, and subject to change until Drush 10.5 or 11.0 is tagged.

greg-1-anderson · 2021-03-21T17:04:37Z

I realize that it's early for this PR to deprecate the drush.commands discovery method, as this PR does not provide dependency injection for discovered commands, whereas drush.commands does.

I think this PR is still good to merge; we can add DI as follow-on work. We should back out the deprecation, though.

We could potentially deprecate auto-discovery by location (__DRUPAL_ROOT__ . /drush), potentially.

claudiu-cristea · 2021-03-21T17:23:50Z

I realize that it's early for this PR to deprecate the drush.commands discovery method, as this PR does not provide dependency injection for discovered commands, whereas drush.commands does.

Not sure it provides DI but, yes, let's drop the deprecation for now, it needs more analysis.

Co-authored-by: Claudiu Cristea <clau.cristea@gmail.com>

greg-1-anderson · 2021-03-21T21:46:48Z

I guess I'm not that opposed to AutoloaderAwareTrait.

AdamPS · 2021-04-07T10:30:09Z

Many thanks I confirm this satisfies my requirement mentioned in #4641

claudiu-cristea added 4 commits March 21, 2021 12:32

Provide test coverage.

55f9c6e

Auto-discover commands.

09d0172

Adapt documentation.

73fef69

Coding standards.

c93d0c1

claudiu-cristea added the area-annotated label Mar 21, 2021

Fix tests.

b7086e8

Improve docs.

9638b8b

greg-1-anderson approved these changes Mar 21, 2021

View reviewed changes

greg-1-anderson reviewed Mar 21, 2021

View reviewed changes

claudiu-cristea added 2 commits March 21, 2021 19:52

Warn that command auto-discovery is experimental.

9467245

Prevent duplicate classes in discovery.

e84b128

claudiu-cristea force-pushed the autodiscover-3rd-party-commands branch from 4212b4f to e84b128 Compare March 21, 2021 18:09

claudiu-cristea and others added 2 commits March 21, 2021 20:11

Typo.

f5cd4ce

Add classloader to auto-discovery PR (#4697)

159f3f8

Co-authored-by: Claudiu Cristea <clau.cristea@gmail.com>

weitzman approved these changes Mar 22, 2021

View reviewed changes

weitzman merged commit 09ef1ac into 10.x Mar 22, 2021

weitzman deleted the autodiscover-3rd-party-commands branch March 22, 2021 03:55

weitzman mentioned this pull request Apr 2, 2021

Namespace problems with Site-Wide Drush Commands #4641

Closed

weitzman mentioned this pull request Jan 21, 2022

Return values instead of keys in discoverCommandsFromConfiguration. #5039

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto-discover non-Drupal commands #4696

Auto-discover non-Drupal commands #4696

claudiu-cristea commented Mar 21, 2021 •

edited

claudiu-cristea commented Mar 21, 2021

weitzman commented Mar 21, 2021

weitzman commented Mar 21, 2021 •

edited

claudiu-cristea commented Mar 21, 2021

weitzman commented Mar 21, 2021

greg-1-anderson left a comment

claudiu-cristea commented Mar 21, 2021 •

edited

greg-1-anderson Mar 21, 2021

claudiu-cristea Mar 21, 2021

greg-1-anderson Mar 21, 2021

claudiu-cristea Mar 21, 2021

claudiu-cristea Mar 21, 2021

greg-1-anderson commented Mar 21, 2021

greg-1-anderson commented Mar 21, 2021

claudiu-cristea commented Mar 21, 2021

greg-1-anderson commented Mar 21, 2021

AdamPS commented Apr 7, 2021

Auto-discover non-Drupal commands #4696

Auto-discover non-Drupal commands #4696

Conversation

claudiu-cristea commented Mar 21, 2021 • edited

claudiu-cristea commented Mar 21, 2021

weitzman commented Mar 21, 2021

weitzman commented Mar 21, 2021 • edited

claudiu-cristea commented Mar 21, 2021

weitzman commented Mar 21, 2021

greg-1-anderson left a comment

Choose a reason for hiding this comment

claudiu-cristea commented Mar 21, 2021 • edited

greg-1-anderson Mar 21, 2021

Choose a reason for hiding this comment

claudiu-cristea Mar 21, 2021

Choose a reason for hiding this comment

greg-1-anderson Mar 21, 2021

Choose a reason for hiding this comment

claudiu-cristea Mar 21, 2021

Choose a reason for hiding this comment

claudiu-cristea Mar 21, 2021

Choose a reason for hiding this comment

greg-1-anderson commented Mar 21, 2021

greg-1-anderson commented Mar 21, 2021

claudiu-cristea commented Mar 21, 2021

greg-1-anderson commented Mar 21, 2021

AdamPS commented Apr 7, 2021

claudiu-cristea commented Mar 21, 2021 •

edited

weitzman commented Mar 21, 2021 •

edited

claudiu-cristea commented Mar 21, 2021 •

edited