Delete bulk aliases also deletes custom url paths

Also ran into this. I think this is an important one to be able to cover. I've patched the module and will be uploading the patch shortly.

This is an initial stab at maintaining custom aliases that have been defined. It adds a checkbox titled "Keep custom aliases" to the bulk delete page, and the box defaults to checked. If checked, the path type state is acquired for every key and if it's set to 0 then it excludes that source path from being deleted.

What is left to do to complete this issue?

We will have to implement this feature the same way as Bulk update, to correctly check custom aliases.

Nice work, some feedback below.

1. +++ b/src/AliasTypeBatchUpdateInterface.php
   @@ -19,4 +19,11 @@ interface AliasTypeBatchUpdateInterface extends AliasTypeInterface {
   +  /**
   +   * Gets called to batch delete all entries.
   +   * @param array $context
   +   *   Batch context.
   

   needs an empty line above @param.

2. +++ b/src/Form/PathautoAdminDelete.php
   @@ -94,17 +108,77 @@ class PathautoAdminDelete extends FormBase {
   +    // Keeping custom aliases forces us to go the slow way to correctly check the automatic/manual flag.
   

   comments longer than 80 characters need to wrapped on two lines.

3. +++ b/src/Plugin/pathauto/AliasType/EntityAliasTypeBase.php
   @@ -179,6 +180,51 @@ class EntityAliasTypeBase extends ContextAwarePluginBase implements AliasTypeInt
   +    $query->range(0, 25);
   

   deleting aliases should be fairly fast, I'd process more than 25 per run, we can easily do 50 I think.

4. +++ b/src/Plugin/pathauto/AliasType/EntityAliasTypeBase.php
   @@ -223,6 +269,28 @@ class EntityAliasTypeBase extends ContextAwarePluginBase implements AliasTypeInt
   +
   +    $entities = $this->entityTypeManager->getStorage($this->getEntityTypeId())->loadMultiple(array_keys($id_to_pid));
   +    foreach ($entities as $entity) {
   +      // Skip if pathauto processing is disabled.
   +      if ($entity->path->pathauto == PathautoState::SKIP) {
   +        continue;
   

   We could consider to query the code in the state class to avoid loading the complete entity objects just to check that flag. Then we could possibly process an even larger amount of aliases per run.

About querying the PathautoState class, I have two questions:

1) How do I access methods in that class from EntityAliasTypeBase? I mean I cannot just write something like $this->pathautostate->getValue(), because there is no such pathautostate method available, and I wouldn't know how to define it anyway, and $this doesn't point to specific alias object.

2) I see that PathautoState::getValue() calls $this->parent->getEntity(). So aren't we going to load the entity object anyway?

Yes, we can't re-use the code. But all we need really is this part:

\Drupal::keyValue($this->getCollection()) ->get($this->parent->getEntity()->id());

I would just copy it for now, we don't need the fallback for the pattern based check, we'll handle that late.

All review comments addressed.

Thanks!

I think we need to check if we really have an entry in $states to avoid PHP notices.

Also, I'm wondering if we should switch it around and only delete those where we have an explicit CREATE. We've been discussing how to handle cases where there is no alias for an entity and if we should store SKIP or CREATE or nothing and we might end up saving nothing, in wihch case we would delete those aliases too. So the safe option seems to be to only delete those where we know for sure that it is a pathauto alias?

Would be great to have that covered with tests as well, it might already work for now with the current code base, but then we are sure not to break it when we make changes.

Also, we should inject the key value service like we did the database one. And thanks to the new test, we won't forget forum this time ;)

I wanted to include forums in PathautoMassDeleteTest, but it appears that tests fail badly, because PathautoAdminDelete is unable to differentiate taxonomy terms and forums. So PathautoAdminDelete always says that you have no forums, and selecting taxonomy terms also removes forums.

I see that ForumAliasType returns getSourcePrefix() = '/forum/', but AFAICS this source prefix doesn't match anything, which explains why PathautoAdminDelete is unable to recognize forums. Is that a known bug? For now, I will simply ignore forums or, if you prefer, I can simply load the 'forum' module and create a pattern for it to do minimal testing, i.e. to make sure that pathauto doesn't crash when the forum module is enabled. @Berdir, what do you think?

Meanwhile, I addressed all your other review comments.

Yes, there's a core issue about forum not working anymore. But as far as I see, nobody cares as nobody really has forums anymore on a Drupal site in 2017, apparently :)

The batch logic could not have worked as it did a max() on a list of arrays. Surprised that doesn't result in some PHP notices/warnings.

Changed to use fetchAllKeyed()* but this shows that we are missing test coverage. We need enough aliases to have at least two batch steps. So either we create 100+ entities with an alias, or we make the total configurable and set it very low to for the test.

That change also allowed to clean up the batchDelete() method quite a bit, as we no longer need to re-organize the array. Also fixed the comment and not-initialized arrays, which would have resulted in a notice if there is nothing to delete.

Fixed test to delete 100+ nodes and I also deleted data from the key_value table for one alias to check how the code behave. By default, aliases with an unknown state must be considered the same way as custom aliases, i.e. that they should not be deleted when custom aliases are asked to be left alone.

Nice. Verified that the patch from #16 failed with the new test, so this is covering that bug.

Committed.