- Issue created by @kevinquillen
- πΊπΈUnited States kevinquillen
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.
- πΊπΈUnited States kevinquillen
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.
- πΊπΈUnited States kevinquillen
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?
- πΊπΈUnited States kevinquillen
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.
- Merge request !14Draft: Attempt to address the race condition of saving a node not persisting the... β (Open) created by kevinquillen
- πΊπΈUnited States kevinquillen
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.
- πΊπΈUnited States kevinquillen
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.
- πΊπΈUnited States kevinquillen
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.