Problem/Motivation
When several menu links point at the same route and differ only by their query string, the active trail always resolves to the same menu link, no matter which of them the visitor actually followed. The wrong parent menu item is highlighted.
With the merge request from #2845319 applied, the correct child link receives is-active while the wrong parent is still highlighted, because in_active_trail is computed on a separate code path that the merge request does not touch.
Steps to reproduce
1. Create a menu structure similar to this:
-- Parent A > /all-topics
---- Child A > /all-topics?foo=1
-- Parent B > /node
---- Child B > /all-topics?foo=2
2. Visiting Child A will mark the link as is-active and the Parent A as active
3. Visiting Child B will mark the link as is-active and Parent A as active
Expected: Parent B should be marked as active when visiting Child B.
Actual: Parent A is marked active when visiting Child B.
Proposed resolution
When more than one menu link matches the current route, prefer the link whose query options are all present in the current request. Fall back to the current behaviour when none match.
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | 3618456-11.x-7.patch | 7.45 KB | robertragas |
Issue fork drupal-3618456
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
robertragas commentedComment #3
robertragas commentedTest coverage added
Comment #5
robertragas commentedPushed fixes for the code quality pipeline.
Comment #6
robertragas commentedAdded more test coverage and a fix now.
Had to include the RequestStack as MenuActiveTrail needs the current query string so it can compare it against the query options a menu link declares. Nothing already injected exposes it I believe.
That means it would need to land in Drupal 12, and will create a patch for Drupal 11 with the deprecation notice. Not sure how that goes with the gitlab PRs if you want to create a PR for Drupal 11 too.
Comment #7
robertragas commentedAdded a patch for Drupal 11 that includes a deprecation notice similar to PathMatcher. The Drupal 11 version in which this will be deprecated still needs to be determined, and a change record needs to be created. I have left those out for now, as I would first like to get feedback on the proposed solution and whether this is the right direction.
Comment #8
robertragas commentedComment #9
robertragas commentedRebased
Comment #10
ribelThe approach looks correct as a follow-up to #2845319: The highlighting of the active links does not respect query strings and fragment identifiers: when several menu links share a route,
MenuActiveTrailnow prefers an enabled link whose query options are a subset of the current request (with unrelated params like pager still allowed), falls back to the previous first-enabled-link behaviour when nothing matches, and includes the query in getCid() so the trail cache varies correctly.I also have tested patch #7 locally and it works as expected.
Thanks @robertragas!
Comment #11
sonikam commentedI have tested the fix. Selecting a particular child always points to it's own parent now
Comment #12
kingdutchMarking as RTBC based on review by ribel and the testing by our QA specialist Sonika.
And congratulations to her on her first contribution to a Drupal.org issue 🎉