Comments

sime created an issue. See original summary.

LammensJ’s picture

LammensJ’s picture

Status: Active » Needs review
Berdir’s picture

Status: Needs review » Needs work
+++ 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?

gaurav.kapoor’s picture

gaurav.kapoor’s picture

Status: Needs work » Needs review
LammensJ’s picture

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

nabiyllin’s picture

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

Berdir’s picture

Status: Needs review » Needs work

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

Adita’s picture

Status: Needs work » Needs review
FileSize
2.88 KB
12.42 KB

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

Yaazkal’s picture

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.

Prashant.c’s picture

Status: Needs review » Needs work
FileSize
132.93 KB

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.

claudiu.cristea’s picture

We need to adapt to Drush 9