The core system for retrieving the active menu trail does not check if the matching menu link is enabled or disabled. This can lead to several problems when a menu link is disabled, such as an "active" class inappropriately set on a parent menu link, or a wrong breadcrumb if Menu Breadcrumb is used.
A similar problem can happen if the matching menu link is active but one of its parent links is disabled. But:
* this would be a very strange use case,
* I don't know what we should expect from such a configuration,
* in that case, the problem could be easily avoided by also disabling the matching menu link,
* handle this would require to load all the parents to check if they are enabled, which might harm performance.
So I guess we only need to handle the case where the matching menu link is disabled. Here's a patch.
Create menu items with this structure in the main menu:
Browse to Node 2 and inspect the menu markup: the Node 1 menu item has the primary-nav__menu-link--active-trail class.
Disabled menu items should not be used to generate the active trail.
Review
Commit
N/A
N/A
N/A
We should add a heads-up to the release notes, since some sites might rely on the previous buggy behaviour (see #36):
<p>When retrieving the active menu trail, <a href="/node/3544512">disabled menu links will no longer be taken into account</a>.</p>
| Comment | File | Size | Author |
|---|---|---|---|
| #60 | drupal-core-2973515-revert.patch | 987 bytes | alexander tallqvist |
| #34 | 2973515-nr-bot.txt | 91 bytes | needs-review-queue-bot |
Show commandsStart 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:
changes
Comments
Comment #2
borisson_We should add tests for this usecase so that we confirm this patch actually resolves the issue.
Comment #4
joachim commentedSetting to needs work because tests are needed.
Comment #10
pameeela commentedClosed #2923368: Parent menu is active when accessing disabled child menu link as a duplicate so adding credit here.
Comment #13
mohit_aghera commentedAdded PR with initial test cases https://git.drupalcode.org/project/drupal/-/merge_requests/369
Test cases need some review.
Comment #17
quietone commentedThe new test is failing setting to NW. I have not reviewed the patch or the test.
Comment #18
smustgrave commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
Seems there was a test failure in the MR.
Hiding patch as fix is in MR.
Comment #19
mohit_aghera commentedI tried to reproduce the issue manually using following approach:
My menu tree looks like given below
- Home
- Parent (path: node/)
--Node 1 (active) (path: node/1)
--Node 2 (disabled) (path: node/2)
Now when I visit node/2 in the browser directly, i can see proper menu tree in the UI
Besides, I don't see any `is-active` class applied to Parent or Home menu links.
Can we get additional steps to reproduce the issue.
Comment #21
prudloff commentedComment #26
prudloff commentedI added steps to reproduce and created a new MR that targets 11.x.
Comment #27
smustgrave commentedhttps://git.drupalcode.org/issue/drupal-2973515/-/jobs/4680214 shows the test coverage and the actual fix seems pretty straight forward to me.
Comment #28
catchI left a minor comment on the MR, which is a bit of a nitpick and I'm 50/50 whether it's in scope or not. Back to needs review, but would be fine with either fixing it here or opening a follow-up.
Comment #29
catchComment #30
smustgrave commentedSeems fair
Comment #32
shalini_jha commentedI have gone through the feedback on the MR and addressed the requested changes.I also re-ran the tests locally, and they are passing.
Moving this back to Needs Review for further feedback.kindly review.
Comment #33
smustgrave commentedFeedback addressed
Comment #34
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #35
prudloff commentedMerged the latest 11.x.
Comment #36
catchI've just done some profiling of a site that was relying on this behaviour - a very large menu with about 180 enabled links and 1500+ disabled links, the disabled links are being used so that menu_breadcrumb will match the disabled links to the enabled parent items.
That set-up is causing other problems but would definitely break as a result of the fix here, so on the basis that potentially more than one site might be doing some version of this, I think we need a release note and a change record.
Comment #37
prudloff commentedI added a basic CR: https://www.drupal.org/node/3544512
Comment #38
smustgrave commentedRegarding #36 do we think this would warrant a deprecated sub module to maintain existing behavior that would be removed in D13?
Comment #39
smustgrave commentedKnow it's hard to know but
feel as a CR it should have an example of what "adapt your code" is.
Comment #40
smustgrave commentedre-reading #36 also mentions a release note
If no one else has a concern #39 it can be ignored.
Comment #41
feyp commentedJust ran into this once more and the MR fixes it nicely, thanks for working on this.
Actually, I agree with #39. We don't provide an API to disable the new behaviour, so I think just "adapting the code" isn't a solution. I have removed this paragraph from the CR. The whole CR is more like a heads-up for people who might possibly rely on this behavior so that they can look into how to best deal with removal of this bug before updating. I don't think we actually need to provide a "solution" to restore the buggy behavior in the CR. "There is no replacement"-CRs are also not that uncommon, so it should be fine that way for this obvious bug.
Likewise, I don't think we should add an API to restore the old behavior and then provide a BC layer (most likely including UI so that you could enable it somehow) just to remove all of it again in Drupal 13. I don't think the case in #36 is common enough to warrant this and @catch didn't ask for it.
I added a release note to the IS and also added a sentence on why we think a release note might be a good idea.
Comment #42
smustgrave commentedCR definitely reads more like a "heads up" change now. Believe all feedback has been addressed here
Comment #43
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #44
john.oltman commentedWe needed this (patch based on the MR) after upgrading from Drupal 11.2.9 to 11.3.2, otherwise would get an exception from the menu_breadcrumb module for any page with a disabled menu item.
I'll post over there as well.
Comment #45
prudloff commentedI merged the latest 11.x.
Comment #47
smustgrave commentedRestoring status from the bot.
Comment #48
godotislateOne minor comment on the MR.
Comment #49
smustgrave commentedApplied the suggestion. Verified the docs too allow for null return
\Drupal\Core\Menu\MenuLinkInterface|nulljust in caseComment #50
smustgrave commentedAlso rebased this as think it would be a good thing for D12 to have, maybe it should be tagged as a priority or highlight item?
Comment #54
godotislateCommitted and pushedt to main and 11.x. Published the CR for 11.4.0. Thanks, everyone!
Comment #57
joelpittetI think I’ve been using this as a feature not a bug. I could trick the breadcrumbs into giving a path back from a page I didn’t want visible in the menu. I guess I should give more attention to menu_item_visibility module now. Or some menu_position rule
Comment #58
rmantics commentedSame here. Our uni had relied on this as a feature for some custom menu building paragraph types we had in place. (In addition to the breadcrumbs) Trying to figure out how best to handle after migrating our site to Drupal 11.
Did you find a good approach @joelpittet?
Comment #60
alexander tallqvist commentedQuite a few of our sites also rely on this behaviour. For now, I’ve created a patch that reverts the change while I look for a better long-term solution.
Comment #61
briangon commented-1 ... Was relying on this, and the D11 upgrade turfed it.
I can lean on menu_position -- but for one-off links this was more elegant.
Will appreciate the patches and/or a custom mod to keep it reversed.