Report Drush 12 deprecations with command classes and drush.services.yml file

[davereid]

trafficstars

Feature request

If a site has written custom Drush commands using Drush 11 or lower, and they update to Drush 12, the drush.services.yml files are now deprecated as per https://www.drush.org/12.x/commands/

Drush 12 expects commandfiles to use a create() method to inject Drupal and Drush dependencies. Prior versions used a drush.services.yml file which is now deprecated and will be removed in Drush 13.

Drush 12 expects all commandfiles in the /Drush/<Commands|Generators> directory. The Drush subdirectory is a new requirement.

I guess I could write a rule for any classes that extend the DrushCommands class...

[mglaman]

So anything which extends DrushCommands and doesn't have a create method, throw an error?

[davereid]

So anything which extends DrushCommands and doesn't have a create method, throw an error?

Only if the module that owns that command file has a drush.services.yml file for dependency injection.

[mglaman]

Because if they do not, it's assumed it doesn't need dependency injection?

[davereid]

Because if they do not, it's assumed it doesn't need dependency injection?

I think it depends if there is a drush.services.yml record for that class that contains arguments or not.

[mglaman]

As of, things have changed yet again: https://github.com/drush-ops/drush/releases/tag/12.5.0

Commandfiles are now encouraged to inject their dependencies via Autowire (https://github.com/drush-ops/drush/pull/5893). Prior methods such as a create() method or a drush.services.yml file are deprecated and may be removed in Drush 13. Now is also a good time to update commandfiles to use PHP Attributes instead of annotations.

[mglaman]

I think this can be DrushAutowireTraitRule which reports an error if a command class doesn't have the AutowireTrait on the class.

[davereid]

Well, I'm confused about the messaging from Drush about create() given that Drupal core also isn't strong on autowiring:

Prior methods such as a create() method or a drush.services.yml file are deprecated and may be removed in Drush 13.

... vs the docs from https://www.drush.org/12.x/dependency-injection/:

If Autowire is still insufficient, a commandfile may implement its own create() method (see below).

I would caution to warn only if AutowireTrait and create() are not used, because otherwise I can't make a contrib drush command compatible with the current and previous major versions, which is personally what I target doing.

[weitzman]

1. I would warn if a drush.services.yml is present. Thats the only thing thats going to be deprecated in the foreseeable future. Point people to https://www.drush.org/latest/dependency-injection/ for more info.
2. Perhaps warn if composer.extra.drush is present. Its no longer needed with Drush 12+

@davereid - yeah create() is not part of the deprecation. Its more that Autowire is new and shiny and slightly preferred. Feel free to keep using create(). AFAICT autowire is very much available and recommended for Contrib. Its just Drupal core thats slow.

[mglaman]

Thanks for the input and clarifications!

Right now we have:

            $servicesFileName = $module_dir . '/' . $module_name . '.services.yml';
            if (file_exists($servicesFileName)) {
                $this->serviceYamls[$module_name] = $servicesFileName;
            }

I guess we need the same for drush.services.yml. This way, the DrushAutowireRule can do something like the following.

If the class name is found in the service map, via a drush.services.yml check if it has ::create and/or AutowireTrait. If it does not, report an error that command classes need to use modern dependency injection, see https://www.drush.org/latest/dependency-injection/

I shouldn't need to check composer.json (which is harder to do, more of an Upgrade Status thing) then.

[weitzman]

OK, but the presence of drush.services.yml should always trigger a warning. Its always good to remove it. If you have one AND you have autowire/create, you are doing the same work twice. Thats likely harmless, but its confusing where changes should be made.

[davereid]

OK, but the presence of drush.services.yml should always trigger a warning. Its always good to remove it. If you have one AND you have autowire/create, you are doing the same work twice. Thats likely harmless, but its confusing where changes should be made.

Yup, agreed.

[mglaman]

Perfect. Thanks! I hit major camp brain fog and fatigue before writing any code. But at least the requirements are now captured. I appreciate it!