Skip to content

Conversation

weitzman
Copy link
Member

@weitzman weitzman commented Oct 14, 2024

Status

  • We will present this work at Drupalcon Vienna and hope to gain consensus for moving most of our commands into Drupal core or Drupal CMS.
  • This PR is backward compatible. Therefore we intend to merge it into the 13.x branch. Drush 14 may remove support for old style commands. That is a separate decision.

Open issues

  • Keeping the feature as is. Keep dependency on consolidation/filter-via-dot-access-data, move that into output-formatters, or drop the feature. See \Drush\Formatters\FormatterTrait::alterResult
  • redispatch.hook currently looks at AnnotationData in order to stop redispatches. It needs to be able to operate on Console commands.
  • Topics are not part of the Console data model. I'm thinking we append links to www.drush.org pages at bottom of a command's help.
  • 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.
  • Update: we chose Attributes. Decide between two methods of delivering OptionSets. SqlDumpCommand demonstrates both approaches:
    1. The tableSelection optionset is added by adding #[OptionsetTableSelection] on the class (i.e. keep the status quo). This signals OptionsetTableSelectionListener to add the options.
    2. 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.
  • Update: we chose Attributes. Same as above, for validators. ImageFlushCommand demonstrates the two approaches.
    1. The #[ValidateModulesEnabled] Attribute signals to ValidateModulesEnabledListener to do its validation work
    2. The top of execute() calls the static Validators::entityLoad()

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 /description/alias. The Drush #[Command] Attribute is deprecated.
  3. Our commands extend Symfony's Command (not DrushCommands) 💪.
  4. Use configure() and interact() methods as per usual with Symfony commands. Drush's #[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:

Console recently introduced invokable commands - symfony/symfony#59340 (blog post). This PR fully supports them when used Symfony with 7.4+ (releases in Nov 2025). An example in Drush is MyCatCommand. This will become the recommended way to write commands after Nov 2025 when you only care about supporting Drupal 11+.

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
Copy link
Member

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
Copy link
Member Author

weitzman commented Oct 14, 2024

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.

@weitzman weitzman changed the title Deprecate AnnotatedCommands in favor of native Symfony Console commands Deprecate Annotated Commands in favor of native Symfony Console commands Oct 15, 2024
public function remove(string $id): void
{
$rf = new \ReflectionProperty(\Symfony\Component\Console\Application::class, 'commands');
$rf->setAccessible(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Collaborator

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
Copy link
Collaborator

Chi-teck commented Oct 15, 2024

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()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants