drupal_menu() incorrectly caches the active parent menu item in two-leveled menu.
Drupal 9.2.0
Twig tweak 3.1.2
Create two-leveled menu:
- Parent 1
-- Child 1
-- Child 2
- Parent 2
-- Child 3
-- Child 4
Do not expand every parent so only active parent should display its children.
Call menu in template:
{{ drupal_menu('main') }}
Clear cache and visit Parent 1 or Child 1 so new cache will be built with active trail on Parent 1.
We see current menu state like this:
- Parent 1 (active)
-- Child 1
-- Child 2
- Parent 2
Example:
Visit Parent 2 or some of its child.
We expect current menu state like this:
- Parent 1
- Parent 2 (active)
-- Child 3
-- Child 4
but we still see previous cached menu state with active Parent 1 and its child.
Example:
Try to combine two drupal_menu() calls:
{{ drupal_menu('main', 1, 1) }}
{{ drupal_menu('main', 2, 2) }}
The same result. We got only one menu state witch was first in cache.
Disable caches as usual in development environment and everything will work properly.
Align the drupal_menu() logic with the logic of menu from the core. So two-and-more-leveled menus can work properly with cache enabled.
-
-
-
-
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | twig_tweak-active_context-3222666.patch | 343 bytes | gpotter |
| Parent 2 page.png | 225.01 KB | theruslan | |
| Parent 1 page.png | 219.19 KB | theruslan |
Comments
Comment #2
chi commentedWhat Twig template did you use for testing?
Comment #3
theruslanChi, menu--main.html.twig from bootstrap_barrio with minor changes in class names.
Then just calling {{ drupal_menu('main') }} in header.html.twig.
The same behavior with original version - https://git.drupalcode.org/project/bootstrap_barrio/-/blob/5.x/templates...
Also tested with https://git.drupalcode.org/project/bootstrap/-/blob/8.x-4.x/templates/me... - the same behavior.
Works only in environment with caches disabled.
Comment #4
gpotter commentedI was also able to replicate the issue. Noticed that the cache contexts from the render array in the "MenuViewBuilder" class in twig_tweak only included "user.permissions" and "user.node_grants:view" contexts. There were no context denoting "the current page", or specifically an active trail. Made a patch and attached it here. It seemed to fix my issues, but I'm not sure if this is the right way, or why the additional cache context wasn't added elsewhere?
It appears Drupal\system\Plugin\Block\SystemMenuBlock in core does something similar in its getCacheContexts method.
Comment #5
letrollpoilu commentedI had the same problem and it looks like the patch working for me! Thanks a lot!
Comment #6
theruslanComment #7
superbiche commentedAlso had the same issue, #4 fixes it. Thanks a lot!
Comment #10
chi commentedFixed in Twig Tweak 3.1.3. Leaving open as the fix needs to be backported to 2.x branch.
Thank you.
Comment #11
smustgrave commentedIn order for this to be backported to 2.x the entire View src folder will need to be added. Is that something that should be done? Will that make 2.x close to 3.x and if so is it worth it?
Comment #12
chi commented@smustgrave Why do you think so?
Comment #13
smustgrave commentedThe file being patched here in 3.x doesn’t exist in 2.x none of the files in the directory do. And if backporting 3.x changes to 2.x is just making 2.x more closely resemble 3 should 2.x be retired for 3. If that makes any sense?
Comment #14
chi commentedThe file structure does not matter. The relevant code does exist in 2.x branch.
https://git.drupalcode.org/project/twig_tweak/-/blob/8.x-2.9/src/TwigExt...
Backporting a fix basically means creating a new patch that is applicable to the target branch. In some cases the new patch may be absolutely different from the initial one. However it should produce the same effect.
In this case backporting should be trivial.
Comment #15
smustgrave commentedSo looking at the code for the drupalMenu function in 2.x it calls Drupal cores build() function which already does
$build['#cache']['tags'][] = 'config:system.menu.' . $menu_name;
so not sure if I see what needs to be changed? Unless it's staring me in the face
Comment #16
chi commentedIt needs active trail cache context same as in 3.x patch.
Comment #17
smustgrave commentedcan you pass that to the $tree object before the build? Seems like a thing that was needed for the build object.
Comment #19
chi commentedFixed in 8.x-2.10. Thank you.