Deprecate Annotated Commands in favor of native Symfony Console commands

[weitzman]

Open issues

• Depends on Return in setInput(). Saves a line of boilerplate. consolidation/output-formatters#113
• Keep dependency on consolidation/filter-via-dot-access-data, move that into output-formatters, or drop the feature. See \Drush\Formatters\FormatterTrait::alterResult
• Update: we will do this when the PR is ready for merge. Move FormatterTrait and related Attributes into consolidation/output-formatters.
• Update: Moshe hacked image:flush into Dex and it worked!. Attempt to re-use the new style commands and listeners in a separate vanilla Console app, or use Dex. The goal is to verify how consumable these commands are.
• Decide between two methods of delivering OptionSets. SqlDumpCommand demonstrates both approaches:
  • The sql optionset is added by calling a static method from configure(). No Attribute is involved. This is less magical (good) but perhaps slightly harder to discover as a command author.
  • The tableSelection optionset is added by adding #[OptionsetTableSelection] on the class. This signals OptionsetTableSelectionListener to add the options.
• Same as above, for validators. ImageFlushCommand demonstrates the two approaches.
  • The top of execute() calls the static Validators::entityLoad()
  • The #[ValidateModulesEnabled] Attribute signals to ValidateModulesEnabledListener to do its validation work

Description

It is our hope that Drush commands will be consumed by a future CLI in Drupal core. To make our commands more consumable, this PR changes Drush commands to directly extend Symfony's Command class.

Converted commands are sql:dump, twig:unused and image:flush. Some highlights:

1. Each command gets its own file. Attributes are placed on the class instead of the method.
2. Use Symfony's #[AsCommand] Attribute to declare command name and alias. The Drush #[Command] Attribute is deprecated.
3. A Drush command extends Symfony's Command (not DrushCommands) 💪.
4. Use configure() and interact() methods as per usual with Symfony commands. The #[Argument] and #[Option] PHP Attributes are deprecated.
5. Drush and Drupal services may still be autowired. This is how you access the logger. Build own $io as needed.
6. Commands that wish to offer multiple output formats (yes please!) should:
   1. inject FormatterManager in __construct()
   2. use FormatterTrait
   3. call $this->configureFormatter() in configure() in order to automatically add the needed options. Example.
   4. execute() is pretty boilerplate. do your work in doExecute() instead.

We have more plans for simplifying Drush's Preflight and removing the consolidation/annotated-command and consolidation/robo dependencies - this is just a start!

[greg-1-anderson]

We had talked about moving FormatterTrait to consolidation/output-formatters. I would like to do that. I think this implies that src/Attributes/FieldLabels.php and the other format-related attributes would also need to move over. We can maintain compatibility with Drush 13 if we make Drush\Attributes\FieldLables & c. extend the consolidation/output-formatters attribute.

Should I go ahead and do that, and update this PR to use the output-formatters copy?

[weitzman]

I'm undecided about moving FormatterTrait and the related Attributes. I do agree that both are coupled and probably should live together. When authoring a command, its helpful for all Attributes to be in same namespace so you can browse them in your IDE. If we move the Formatter attributes, we bifurcate them, or we thinly extend the ones provided by output-formatters. We do that thin extension today for those provided by annotated-command, but I always found that slightly awkward. Lets mull it.

Update: the implementation now complete. Maybe @greg-1-anderson has a suggestion about how to implement --filter in the new paradigm. See DrushCommandAlterer and FilterHooks. In this PR I've added #[CLI\FilterDefaultField(field: 'template')] to twig:unused, and [\Drush\Formatters\FormatterTrait::addFormatterOptions] auto-adds the --filter option when needed. Lets discuss how to implement result altering. I'm hoping that command authors don't have to worry about. Maybe we add result altering to \Drush\Formatters\FormatterTrait::execute just before formatting?

--filter is a seldom used feature of Drush and perhaps we should drop it.

[greg-1-anderson]

Not super excited to do this outside of tests. Is this for some sort of "alter" hook? If we want to do this, maybe we should, during preflight, add commands to some other list and provide a specific hook to alter that before adding to the application. That might be complicated, since today we add in two steps (commands from modules being added later).

Alternately, instead of removing a command, maybe we could change its name, remove its aliases and hide it?

I'm not sure why I think that setAccissible is worse than that.

[weitzman]

add commands to some other list and provide a specific hook to alter that before adding to the application

I think thats more more easily done once we use a container to store commands and maybe use a CommandLoader. Yes, this is just used by Woot today - we can remove it if it offends the eyes.

[greg-1-anderson]

Yes, I was thinking that filter could happen in the format trait.

I think that consolidation/output-formatters should have the trait and format attributes, so that they are available when that library is used outside of Drush. I'll probably duplicate the code there regardless, even if Drush doesn't use it. Perhaps thin extensions would be a workable solution, where necessary.

[weitzman]

OK, when this PR is ready to go in, lets move the FormatterTrait and format Attributes, and Drush may thinly extend the Attributes.

[Chi-teck]

The formatter thing looks complex and too magic. Will it save much time for command developers?

I think FormatterTrait should have just one simple getter for the formatter instance.

Something like this.

private static function getOutputFormatter(): FormatterManager {
  return \Drupal::service(FormatterManager::class);
}

It seems quite easy to apply it manually.

protected function configure(): void
{
    $this
       ->addArgument('some-arg', InputArgument::REQUIRED, 'Description');
    self::getOutputFormatter()->configureCommand($this);
}

public function execute(InputInterface $input, OutputInterface $output): int
{
  $data = [];
  self::getOutputFormatter()->write($output, $input->getOption('format'), new RowsOfFields($data));
  return self::SUCCESS;
}

I would actually inject the formatter through constructor rather than through trait.

[Chi-teck]

Given that Command::addOption() is a public method. We could potentially add format option automatically to commands that indicate a need for it through some PHP attribute.

Also curious if it is possible to integrate the formatter with DrushStyle. For instance SymfonyStyle has methods like table(), listing(), etc. Why can't DrushStyle have something like writeFormatted()?

[greg-1-anderson]

Here and elsewhere, KEY is defined in the class but not used in handle()

[weitzman]

Just pushed a fix with a couple tweaks. Depends on another PR in output-formatters consolidation/output-formatters#113

[greg-1-anderson]

Maybe the static method technique of holding option sets would sit better if it lived in some "sql" namespace, rather than putting all option set domains in the same class. (This would be more obvious if there were more than one here.)

[weitzman]

True. The benefit of putting multiple methods on a single OptionSet class is that the optionsets are more discoverable. The class analogous to our existing https://github.com/drush-ops/drush/blob/13.x/src/Commands/OptionsCommands.php

[greg-1-anderson]

I think this can be refactored to avoid the intermediate instances and private field access. This can wait until moving this code into output-formatter.

[greg-1-anderson]

Could we maybe use a command hook to find and dispatch validate hooks?

[weitzman]

I have experimented with that approach earlier in this PR and can't decide whats best. Currently the validation is done right in execute() and isn't not DRY. See ImageFlushCommand. Compare with https://github.com/drush-ops/drush/blob/f923a801dd50d4aa17fd2390790336bc962785a3/src/Listeners/ValidateModulesEnabledListener.php and its associated Attribute. IMO the current approach is less magical and I'm slightly valuing that these days.