node:menu-link:parents:join-path doesn't work as expected

I guess that's the same issue as #2748805: Menu based URL pattern does apply on initial new node save. which should be fixed now if you use the latest token dev as described there?

Yeah, we didn't do anything with parents from memory in #2748805: Menu based URL pattern does apply on initial new node save.

I believe this is more a core issue than pathauto or token.

When updating a node with menu-link. The parent value is not saved in the menu definition of the menu-link when it is the root ('' or empty). When the the menu-definition parent value is not empty everything works as expected. Some extra digging needs to be done to get to the root of this problem.

I have same issue.

IRC user send me this url to patch it.

http://drupal.stackexchange.com/questions/144227/replace-standard-node-p...

I see. Yes, this needs to be fixed token module, not here. We need to transform that test for the existing menu link token tests that we have there (we can still decide to commit this test to pathauto as well in the end as an integration test, I suggest you open a new issue for that. That would be especially interesting if we decide to get this into core and remove the custom token code.

@Tom Robert: It is likely that the code that we added to token module is breaking this, so it might look like a core issue but it's most likely not. We'd have noticed if this wouldn't work there.

Moving to token module. Setting to major, this is hopefully the last of 3 bugs that we found with the menu link stuff and hopefully also the last blocker for a new release.

Test only patch exposing the bug, for the token module.

And my attempt at fixing this issue. It should work for :parent, but most likely other tokens are broken, too. Basically, the menu link plugin's definitions aren't updated when saving the menu link, neither in token module or in menu ui, form what I've observed. Token replacement relies on this, though.

Arg. I wrote very long comment here, explaining lots of fun things about this that you might not expect but now it is late and I lost my comment :(

So, here is the "short" version. happy to provide more explanations tomorrow on IRC or so, ping me.

[node:menu-link:parents:join-path]/[node:title]

That is a weird pattern IMHO. It only works the way you think it does under two conditions: a) node title == menu link title of all parents and b) no parent has a manual or different path. That is because parents is just an array of menu link *titles*, and join-path is a pathauto-specific token that combines them to a path where the / is not getting url encoded. it is not at all about the *path* of the parent menu links.

[node:menu-link:parent:url:relative]/[node:title] is IMHO what you really want here, it is the combination of the url of the parent menu link and node title appended. The first test fail goes away with that pattern in your test, the other two still fail, because there is a bug somewhere when *removing* parent. That's the bug that @Bambell also found in his test (he couldn't reproduce the first fail, more about that in the next paragraph). I haven't found the reason for that yet, but it *should* work. will look more into that tomorrow.

So, why is the first fail in your test happening? Because while parents:join-path is weird, it should still do what it says in this case... the answer is... static caches! Specifically the one in token_menu_link_load_all_parents(). If you comment that out, we're down to two test fails as well. The reason for that is that the form is rebuild on form submission, including the old default value for the alias. At this point, this menu link has no parent yet, this is cached, then we submit the form and save the node, now the menu link was updated but the static cache still has the old entry. So to fix this properly, we need to find how to invalidate the static cache there. We could do it on menu_link_content presave hook, it won't work if for some reason a non-content link parent changes but that seems somewhat unlikely.

Yeah, as I thought, delete is a core bug: #2781673: MenuLinkContent menu links can not be set to no parent once they had one. With that core patch and either the static cache fix or the other token, the original test in this issue is green.

Here is a test only patch exposing the caching issue with the :parents token. I also uploaded a patch with the caching commented in token_menu_link_load_all_parents(). With that commented out, tests turn green except for the case where the menu link parent is changed to the main one, but that is indeed fixed with #2781673: MenuLinkContent menu links can not be set to no parent once they had one. Now got to write a proper fix.

Static cache invalidation in the entity's presave hook and a test. The remaining fail is fixed by #2781673: MenuLinkContent menu links can not be set to no parent once they had one.

Looks nice!

Lets wait a bit, hopefully the core issue gets committed soon, it is done IMHO. This is not a major bug after all (I set that because I thought it was a regression caused by the token menu link code), so doesn't block a new release.

If it turns out to take longer, we can always comment out the last assertion with a @todo on the core issue and get it in already.

@berdir I've tried changing to using the pattern you suggest in #19 but it doesn't work as expected. When bulk updating some of my menu links had aliases like "node17/node-title" (note the lack of slash between node and 17)

When creating new content it was fine with 2 levels (i.e about-us/foo) but 3rd level children were getting the slash stripped again so it was "about-usfoo/bar". I assume this is due to the pathauto stripping the slash in its rules.

My issue #2691929: Pathauto token for node menu hierarchy not working after updating parent node was closed as a dupe of this. Using this patch and testing those steps doesn't provide a fix for my use case.

Pathauto has special logic to not remove / from url tokens. I guess it doesn't support ...:url:relative, we need to fix that there.

Not sure what else could be your problem then. Did you test with this patch and your original token?

@berdir yeah I reverted back to my original pattern and added the patch, cleared cache etc and repeated the steps in my original issue which are:

- Create parent with path /foo
- Create child with path /foo/bar
- Update parent so generated path is /foo-2
-- This adds the nid to a queue worker
- Run cron to process the queue worker which gathers children of the each node in the queue and runs PathautoGenerator::updateEntityAlias with the child entity and an op of "update"

Expected: child gets updated path to /foo-2/bar

Actual: Child path remains the same.

I've done debugging into the token replace call but I haven't had much luck figuring it out.

There is still an issue with Bulk update on a multilingual site with menu fixed per language:

Language A Menu has an entry for node x in Language A
Language B Menu has an entry for node x in Language B.

When using bulk update, the menu path found is the first menu entry. So node x wil have the same alias for Language A and B.

I added an extra language filter.

 if ($links = $menu_link_manager->loadLinksByRoute($url->getRouteName(), $url->getRouteParameters())) {
            $links = array_filter($links, function ($link) use ($node) {
              $link_content = MenuLinkContent::load($link->getPluginDefinition()['metadata']['entity_id']);

              return (in_array($link_content->language()->getId(), array(
                $node->language()->getId(),
                LanguageInterface::LANGCODE_NOT_SPECIFIED,
                LanguageInterface::LANGCODE_NOT_APPLICABLE)));
            });
            $link = reset($links);
            $replacements += \Drupal::token()->generate('menu-link', $menu_tokens, array('menu-link' => $link), $options, $bubbleable_metadata);
}

Can you open a new issue for that?

That fix makes sense but it is a new, different bug that will need dedicated test coverage.

The core issue was fixed and this test is now passing.

@berdir so can I reopen my issue that was marked as a duplicate on this one?

Adding issue reported in #34 as related issue.

Committed #26, I think we have all-the-followups now to figure out the remaining problems.

Solved this issue with lastest upgrade of core and plugins ;P