path_alias record does not update on entity save.

I think the issue may lie here:

      if ($node instanceof NodeInterface) {
        $langcode = \Drupal::languageManager()
          ->getCurrentLanguage(LanguageInterface::TYPE_CONTENT)->getId();
        $langcode = $entity->get('langcode')->getString() ?? $langcode;
        $translation_languages = $node->getTranslationLanguages();
        if (in_array($langcode, array_keys($translation_languages))) {
          $domain_id = $helper->getDomainIdFromEntity($node);
        }
      }

The path was already updated before this, but the presave hook fires again, and this code is hit. It gets the value from the original node, which sets it back to the old value. On the next save, without making any changes, the correct value is inserted.

It almost seems like both the node and the path entity are updated and the latter overwrites the former. If I comment out the code in #4, it updates correctly on every save.

As far as I can tell, since this happens in the presave hook that by the time the path alias logic runs the node it is attached to has not had its changes committed yet. So loading it from either the route or with the entityTypeManager is going to give you the previous Domain Access field value no matter what, until you save it again.

This is a real issue because its hard to track this kind of bug. Ultimately, it means changing the Domain Access value on an entity will lag behind one save and causing pathing issues as content resolves to potentially the wrong domain, causing a 404 on another domain (where it should be).

How can both cases effectively be handled without overwriting?

So far removing the logic in presave that updates if the entity is a PathAlias resolves what I am seeing.

I think that the case here is when the user is either updating a node or updating a Path entity directly. But theres a few things I don't quite get:

    // Get domains configuration.
    $options = \Drupal::entityTypeManager()
      ->getStorage('domain')
      ->loadOptionsList();

    // Get domain_id from get request param.
    $helper = \Drupal::service('domain_unique_path_alias.helper');
    $domain_id = $helper->getDomainIdByRequest();

    // Get domain_id from field_domain_source field.
    if (empty($domain_id) || !in_array($domain_id, $options)) {
      // Get field_domain_source data if a new translation.
      $node = \Drupal::routeMatch()->getParameter('node');

If we are updating from the path form from admin/config/search/path/edit/{id} it seems like we should be getting the entity not from the route (because any content entity could have Path enabled) but try to extrapolate it from the path value, which is here:

      if (!$node instanceof NodeInterface) {
        $path = explode("/", $entity->getPath());
        if (
          isset($path[1], $path[2])
          && $path[1] === 'node'
          && is_numeric($path[2])
        ) {
          $node = Node::load($path[2]);
        }
      }

Which is likely fine as it is, but I think to prevent the race condition this may have to move to a submit hook on the path edit form.

I've added an initial change here that tries to do two things:

• - Eliminate the race condition in hook_entity_presave(). I tried using hook_entity_update, but I saw the same behavior(s) reported. With this change I cannot replicate the behavior.
• - Update the path entity from the Node level on save.
• - Move the other path entity update logic to a form submit on the path entity form.

It's not exactly perfect because the path entity form has no awareness of the domain id (perhaps it could be a hidden value) so on submit I have to call entity save, which is likely happening more than once and could introduce new problems. This part would need more work, I believe - especially to expand support beyond nodes to any content entity with a path.

I've updated the change to incorporate what was there previously, except without loading the node from the route parameter. Using the helper to get the domain ID seems to work 100% of the time, but grabbing the node from the request or otherwise loads the unsaved node with the previous value.

This now appears to work reliably on create/update of a node.

However, I now notice that node edit links are incorrect (like tasks). I will make a new issue for that.

The additional saving of path alias in presave is also problematic if you are editing content for multiple Domains from one domain. At this level, I am not sure why the PathAlias handling is in a presave hook when it is explicitly being set otherwise.

s1933 → made their first commit to this issue’s fork.

Hi Kevin,

Thanks for your report again. I was away the past few weeks.
FYI, I committed a functional test to cover this issue—it occurs not only on the edit submit but also when adding a new node.

In the coming days, I should be available to review and rework your MR.

I just committed changes to your merge request.

My position:
- We should not save the domain_id in node_presave(), because it’s the responsibility of path_alias_presave() to handle that.
- To determine which domain_id needs to be saved in path_alias_presave(), I set a new request attribute in node_presave() and retrieve it in path_alias_presave().

I also updated the functional test.

What do you think ?

I've also been trialling this module and run into this problem, but there's extra complication from 🐛 Duplicate path aliases can be added Needs work - somehow (probably due to Migrate) I have duplicate path_alias entities, and only the first one gets updated on save. This messes with later requests as there are multiple matches for the alias, but the domain_id isn't necessarily correct.

Unsure what we can do about this until the duplicate aliases problem is solved in core.