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