Drush commands for bulk alias updates

Created a patch, based on the work done in https://www.drupal.org/node/867578.

+++ b/pathauto.drush.inc
@@ -0,0 +1,125 @@
+    'core' => ['8+'],

we don't need this as it is part of the pathauto 8.x module.

This looks pretty good, thanks.

Can we include the new features from #2451307: Delete bulk aliases also deletes custom url paths and #2678558: Bulk generate should be able to recreate aliases to have feature parity between the drush commands and the UI? should be fairly simple to add some options for this?

Fixed the minor issue.

I refactored the drush commands so they can work with an AliasCliAction-service and implemented the feedback provided by @Berdir.

I tested last patch with D 8.3.4 by unit test - all tests are passed.

Thanks for working on this. Did not expect to actually make this services as well, but that's an option of course. But I think we can simplify the architecture a bit, see my review below.

1. +++ b/src/Cli/AbstractAliasCliAction.php
   @@ -0,0 +1,67 @@
   +/**
   + * Class AbstractAliasCliAction.
   + *
   + * @package Drupal\pathauto\Cli
   + */
   

   we don't use @package in Drupal.

2. +++ b/src/Cli/AbstractAliasCliAction.php
   @@ -0,0 +1,67 @@
   +  public function __construct(
   +    ConfigFactoryInterface $configFactory,
   +    AliasTypeManager $aliasTypeManager
   +  ) {
   

   I prefer function arguments to be on the same line, there is no strict 80 character limit in Drupal, (except for comments).

   Also, the current coding standard still requires to use underscore ($config_factory) for local variables/method arguments.

3. +++ b/src/Cli/AbstractAliasCliAction.php
   @@ -0,0 +1,67 @@
   +    $types = [
   +      self::ALIAS_TYPE_ID_ALL => $this->t('All'),
   +    ];
   +    foreach ($this->aliasTypeManager->getVisibleDefinitions() as $id => $definition) {
   

   static:: should be used instead self:: when possible.

4. +++ b/src/Cli/AliasCliActionInterface.php
   @@ -0,0 +1,40 @@
   +/**
   + * Interface AliasActionCLIInterface.
   + *
   + * @package Drupal\pathauto\Cli
   + */
   +interface AliasCliActionInterface {
   +
   

   I'm not sure we really need an interface. That's always a question and I think it's enough to add interfaces in case we actually expect to have multiple alternative implementations. Especially since the result is that the interface is very generic and its documentation not very meaningful.

   My recommendation would be to have a single service/class with a bulkGenerate() and bulkDelete() method instead of an interface and a generic execute() method.

5. +++ b/src/Cli/AliasDeleteAction.php
   @@ -0,0 +1,111 @@
   +          sprintf('%s::batchProcess', PathautoAdminDelete::class),
   

   Interesting use of sprintf(), haven't seen that before. I personally would just use PathautoAdminDelete::class . '::batchProcess' but I can live with this.

6. +++ b/src/Cli/AliasDeleteAction.php
   @@ -0,0 +1,111 @@
   +      // Process the batch.
   +      drush_bachend_batch_process();
   

   shouldn't be that backend instead of bachend? :)

For #9
1. removed @package, but maybe We need a new description
2. done
3. done
4. pending
5.
6. done

This is pretty good. It will help a lot running staging/deployment scripts now in the era of config/sync. I wonder if that will also update the information for redirect, I guess yes?.
Congrats.

Patch applies successfully on
Drupal Core Version - 8.5.x
Pathauto Version - 8.x-1.x

But facing following error on running the drush command to generate alias.

drush pathauto-aliases-generate
[0]  :  Cancel                           
 [1]  :  all                              
 [2]  :  canonical_entities:node          
 [3]  :  canonical_entities:taxonomy_term 
 [4]  :  canonical_entities:user

i chose
2

[0]  :  Cancel 
 [1]  :  create 
 [2]  :  update 
 [3]  :  all

i chose 3

It is throwing PHP Fatal error

PHP Fatal error:  Call to undefined function Drupal\pathauto\Form\PathautoBulkUpdateForm::batchFinished() in phar:///usr/local/bin/drush/commands/core/drupal/batch.inc on line 231
Drush command terminated abnormally due to an unrecoverable error. 

From the UI Bulk Generate is working fine for me admin/config/search/path/update_bulk

However is working fine.

We need to adapt to Drush 9

Started to adapt this for Drush 9. This still needs work.

Finished an initial implementation of the Drush 9 integration. This is ready for a first look.

Please note when testing that there is some additional output being generated at the very end when executing the Drush commands, for example an integer can be output that represents the number of aliases that were updated, or the word Array may be output.

This is not caused by this patch but rather by the fact that the batch processing in Pathauto is not implemented correctly. The current implementation abuses the $context['results'] array to pass structured data to functions such as PathautoBulkUpdateForm::batchFinished() which then use this data to generate appropriate messages. This is not valid according to the Drupal Batch API which expects the results to be a flat list of strings (or translatable markup). Fixing this is out of scope for this issue.

1. +++ b/src/Commands/PathautoCommands.php
   @@ -0,0 +1,301 @@
   +   * @usage drush pathauto-aliases-delete all --force
   

   Is "force" the right word here? It is not that something is persistent. Maybe --purge or something similar? or --include-manual.

2. +++ b/src/Commands/PathautoCommands.php
   @@ -0,0 +1,301 @@
   +      $this->logger()->success(dt('All of your path aliases have been deleted.'));
   ...
   +        $this->logger()->success(dt('All of your %label path aliases have been deleted.', [
   +          '%label' => $alias_type->getLabel(),
   +        ]));
   

   Is there a specific reason we are logging everything here? When trying to update using the UI and the form itself, nothing is logged. Here, it is more or less the same. A direct action taken from the admin.
   In case this is used from a cron job, the cron job should be responsible for possibly logging changes.

   Plus, the other actions don't seem to log anything.

3. +++ b/src/Commands/PathautoCommands.php
   @@ -0,0 +1,301 @@
   +        $this->aliasStorageHelper->deleteBySourcePrefix((string) $alias_type->getSourcePrefix());
   

   The ::getSourcePrefix() method already returns a string according to the interface and the couple of methods that implement it. No need for typecasting. It won't be a markup class, or shouldn't at least.

4. +++ b/src/Commands/PathautoCommands.php
   @@ -0,0 +1,301 @@
   +   * @command pathauto-aliases-generate
   ...
   +   * @command pathauto-aliases-delete
   ...
   +   * @hook interact pathauto-aliases-generate
   ...
   +   * @hook validate pathauto-aliases-generate
   

   Since we are adopting to Drush 9, we can also go with the new naming of the commands. Now, the commands are prefixed with the module name followed by a column, so the corresponding commands would be something like pathauto:aliases-generate

   Currently, both pathauto commands are listed under _global when running the drush list command.

5. Edit: Also, why is it that the entity types are presented as canonical_entities:node|user|whatever and not simply node/user/blah?
6. Edit 2: When running the pathauto-aliases-delete with an empty set (e.g. after the node entity type aliases are already deleted), there is an Array printed in the terminal as such:

   drush pathauto-aliases-delete canonical_entities:node
    [notice] Message: All of your automatically generated /Content/ path aliases have been deleted.
   
   	Array
   

   otherwise, there is a number printed.

Thanks for the review!

1. Fixed. Changed it to --purge, thanks for the suggestion!
2. You are confusing the logger in Drupal with the logger in Drush. In Drush this is used to output messages to the console. This code is a direct translation of the calls to $this->messenger()->addMessage() in PathautoAdminDelete::submitForm().

   You can verify this by executing the command $ drush pad all --purge, this will output the message in the console rather than log it to the watchdog.

3. Fixed. This was taken directly from PathautoAdminDelete::submitForm() so I didn't question it, but you're right, the interface dictates that a string should be returned.
4. Great suggestion, fixed!
5. This is simply a naming convention used in Pathauto. It is the name of the `AliasType` plugin type.
6. This is explained in comment #2717721-15: Drush commands for bulk alias updates. It is due to the results array of the batch being improperly used. This is out of scope for this issue.

@pfrenssen it seems that while you agreed with it, you forgot to do No 3.

Also,

+++ b/src/Commands/PathautoCommands.php
@@ -0,0 +1,302 @@
+   * @throws \Drupal\Component\Plugin\Exception\PluginException
+   *   Thrown when an alias type can not be instantiated.

a quick nitpick. This seems a bit of a leftover or from an earlier version. Sorry, I just opened the patch in PhpStorm and it complained that this is never thrown.

For number 5, up to now, this naming convention is never presented in the UI as the label of the entity types are shown. So it wouldn't matter up to now. Now, we are presenting the options to the user as such

drush pathauto-aliases-generate
[0]  :  Cancel                           
 [1]  :  all                              
 [2]  :  canonical_entities:node          
 [3]  :  canonical_entities:taxonomy_term 
 [4]  :  canonical_entities:user

That is why I was thinking to change this. But anyway, if pathauto wants to use this style, it is not a blocker for me.

Thanks for having another look @idimopoulos!

1. Sharp eye! Fixed.
2. I was using the labels in my first iteration but this causes problems. This interactive menu is a substitute for the command argument. The actual command that is executed is this:

   $ drush pathauto:aliases-generate create canonical_entities:node
   

   On the command line we need to use the machine name. This makes sense for a lot of reasons. Most importantly because the machine names are guaranteed to be unique, but the labels are not. If there would be multiple alias types with the same label we wouldn't be able to know which one is intended. Also using the labels makes it more difficult for the end user since they need to deal with the correct capitalization, and the possibility of having spaces and special characters in the labels makes scripting more difficult.

   I think it's important to show the exact same arguments in the interactive menu as are used in the command. Otherwise it would be confusing for the user.

   In general I think it is a good practice to resort to machine names on the CLI. After all it is intended for technical users. It is not intended to work exactly like a UI which is designed for non-technical users.

Forgot to remove the unneeded string casting again.

Thanks @pfrenssen. These are all for me. I find the patch working fine :) I also tested it in my current installation which contains a few thousand nodes.
Putting this to RTBC as far as I am concerned.

Thanks!

Thanks. I'm still confused about certain things with drush9, e.g. whether or not a composer.json is required, a similar patch in troken.module adds one to explicitly register the drush commands. But it has been tested and seems to work, resolved a minor conflict on commit.

A composer.json is recommended for modules carrying drush commands. It is not yet mandatory, even in Drush 10. It might become mandatory one day.