This is a cross-post of https://www.drupal.org/node/2691929 since the issue lies in Pathauto rather than Token.

As described on that issue, re: using a Pathauto pattern with ':menu-link:parent' token/s, when updating the alias for an entity that has a menu link that is a parent, the aliases for the child menu links are not updated.

The attached patch adds a call at the end of PathautoGenerator::createEntityAlias() for insert or update operations (if alias changed) to update the aliases for any child menu link/s (if any) of the entity's menu link/s (if any). It only runs this update if any saved pathauto pattern contains ':menu-link:parent/s' tokens. Perhaps an admin setting for this might be preferred?

NB this only operates on entity insert/update, not on menu link hierarchy restructuring.

NB tested successfully with pattern '[node:menu-link:parent:url:relative]/[node:title]'. The pattern '[node:menu-link:parents:join-path]/[node:title]' seemed to replace with stale aliases, not sure of the issue.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bgilhome created an issue. See original summary.

bgilhome’s picture

bgilhome’s picture

Issue summary: View changes
ducktape’s picture

Status: Needs review » Needs work

The use case of menu hierarchy changes need to be covered as well.

I don't think the for-each will scale very well, maybe switch to a queue for processing.

mpp’s picture

Priority: Normal » Major

Hi @bgilhome, thanks for your patch.

I consider this a major bug (or missing feature). Changing the priority.

Like @ducktape mentioned:
Note that the foreach code in #2 will cause performance issues for sites with large menu's (I've seen sites with 50k+ menu items) where a webmaster can move a large subtree in the menu. I would consider using a queue for this: fill it up with all the children when a node's parent menu is moved, then process a configurable amount on cron.

ducktape’s picture

Status: Needs work » Needs review
FileSize
7.22 KB

I rerolled that patch against the latest dev. I have also added support for menu tree restructuring or menu link updates.

I have not added a queue yet, as the processing seems to be fine, even for large trees, however I did not have a 50k+ menu to my disposal to test. @mpp, can you have that tested? :)

Fernly’s picture

The patch in #6 outputs a fatal error when there is no parsable request url because the exception class in the catch statement is not recognised, namespacewise (Exception => \Exception).

Added an updated patch.

Fernly’s picture

Removed the txt extension from the patch file.

gremy’s picture

Status: Needs review » Reviewed & tested by the community

We were were running into this problem with one of our clients, and decided to write a custom module to solve this issue. After running into some issues with our custom module, we found this issue and tried pathauto-update_child_aliases-3016532-8.patch instead of our module.
It works great! Thank you!

0livier’s picture

Berdir’s picture

Issue tags: +Needs tests

This is quite a lot of non-trivial code, I didn't review in depth yet but some test coverage would be very helpful in getting this committed. to ensure that this doesn't get broken in the future.

almunnings’s picture

Getting some strange behaviour with this patch with pathauto 1.8 & Drupal 8.9.6
Removing patch #10 stops this happening.

On creating a new node:
Select 'Provide a menu link' and save.

Node saves, but the link in the menu gets the url node/[uuid] rather than the alias. Menu seems to save incorrectly.

So not sure whats going on here. Race condition?

Edit. My issue appears to be unrelated to this patch. Issue could be memcached 2.2.

https://www.drupal.org/project/memcache/issues/3176451

0livier’s picture

sylvain lavielle’s picture

The #13 patch worked for me but I had to do something more to make the whole thing to work in my case.

The problem that I faced was the Pathauto generation for content related to menu item was triggered to early while menu item update and before the menu tree was updated. Hence, the ":menu-link:parent" pathauto token generated an old version of the alias due to a non-updated menu tree structure.

I fixed it in our project by applying a patch to the core in the menu link manager (core/lib/Drupal/Core/Menu/MenuLinkManager.php), updating the updateDefinition this way :

  public function updateDefinition($id, array $new_definition_values, $persist = TRUE) {
    $instance = $this->createInstance($id);
    if ($instance) {
      $new_definition_values['id'] = $id;

      // Update link just to get the definition but do not persist menu link entity.
      $changed_definition = $instance->updateLink($new_definition_values, FALSE);

      // Update tree using the definition.
      $this->treeStorage->save($changed_definition);

      // Update link and persist.
      // This MenuLinkInterface::updateLink re-call is made to persist menu-link after menu tree update and in order to fix the
      // following pathauto related issue : https://www.drupal.org/project/pathauto/issues/3016532
      if($persist === TRUE) {
        $instance->updateLink($new_definition_values, TRUE);
      }
    }
    return $instance;
  }

I'm aware that is certainly not the good manner. That's only a quick fix in order to make it work. But there is somehow a tricky point there that appears only in case of using :menu-link:parent token/s :

Is pathauto have to change the way it works to handle it ?
Is the core have to change something about the way it works while updating menu tree / menu items ?
an what is the best way to do this (or that)

I thing I'm not deeply aware about those subjects to handle this further right now.

I let you - the aware guys :) - react on it and suggest the best solution

sylvain lavielle’s picture

kevineinarsson’s picture

In #13, the logic that determines whether or not to update a child looks like this:

($child_entity->id() !== $entity->id && $child_entity->getEntityTypeId() !== $entity->getEntityTypeId())

By changing the condition to be an || instead makes more sense to me. That would mean entities of different types with the same ID would evaluate to true as well as entities of the same type but with different ID:s.

kevineinarsson’s picture

Status: Reviewed & tested by the community » Needs review
fengtan’s picture

A combination of #16 with #3181070-2: Possible need to change the way MenuLinkManager update menu-item and menu-tree seems to work.

However, given that the latter requires to patch core, we implemented the following workaround instead as suggested by #3068943-2: Menu navigation change does not affect node path:


// mymodule.module

/**
 * Implements hook_cron().
 */
function mymodule_cron() {
  $batch = [
    'title' => t('Bulk updating URL aliases'),
    'operations' => [
      ['Drupal\pathauto\Form\PathautoBulkUpdateForm::batchStart', []],
      ['Drupal\pathauto\Form\PathautoBulkUpdateForm::batchProcess',
        [
          "canonical_entities:node",
          "update",
        ],
      ],
    ],
    'finished' => 'Drupal\pathauto\Form\PathautoBulkUpdateForm::batchFinished',
    'progressive' => FALSE,
  ];

  batch_set($batch);
  if (PHP_SAPI === 'cli') {
    drush_backend_batch_process();
  }
}

The paths get updated only on cron (instead of right after updating a node or menu item), but that is acceptable for us.

Daniel Korte’s picture

Status: Needs review » Needs work
FileSize
7.45 KB
505 bytes

I’m seeing duplicate path aliases created when updating a node by adding it to a menu via the “Menu settings” fieldset on the node edit page. This is because there is an “insert” operation on the MenuLinkContentInterface entity which prevents createEntityAlias() from setting $existing_alias causing a duplicate alias to be created.

Changing to “Needs work” because tests are still needed and I’m not sure that hardcoding the operation to “update” here is the best solution.