When building active trail, Drupal assumes that the first link matching the current path is the active link.
It is not always true, especially when using the Menu Firstchild module (see http://drupal.org/node/671564 about this problem).
Consider you have this kind of menu:
item A linking to path 1
-- item B linking to path 1
---- item C linking to path 1
---- item D linking to path 2
-- item E linking to path 3
Clicking on item A, item B or item C will bring user to the same path 1.
As item A and item B are just parent containers of item C, item C should be the active link.
But Drupal shows item A as the active link, ignoring item B and item C.
To fix this bug, loop should continue until the last descendant matching current path is found:
<?php
while ($curr) {
// Terminate the loop when we find the current path in the active trail.
// patch
/*
if ($curr['link']['href'] == $item['href']) {
$trail[] = $curr['link'];
$curr = FALSE;
}
else {
*/
// Add the link if it's in the active trail, then move to the link below.
if ($curr['link']['in_active_trail']) {
$trail[] = $curr['link'];
$tree = $curr['below'] ? $curr['below'] : array();
}
list($key, $curr) = each($tree);
/*
}
*/
//~patch
}
?>| Comment | File | Size | Author |
|---|---|---|---|
| #59 | drupal-deepest_active_trail_link-732370-59-d8.patch | 577 bytes | Syndz |
| #56 | drupal-deepest_active_trail_link-732370-56-d8.patch | 659 bytes | Syndz |
| #26 | includes_menu_inc_drupal_6_20_patch.txt | 556 bytes | anrikun |
| #23 | includes_menu_inc_drupal_6_19_patch.txt | 619 bytes | anrikun |
| #21 | includes_menu_inc.patch | 588 bytes | anrikun |
Comments
Comment #1
kroimon commented+1 for this one as it really fixes the issue and doesn't seem to create new ones
Comment #2
anrikun commentedHere's the corresponding patch:
Comment #4
anrikun commented#2: menu.inc_.patch queued for re-testing.
Comment #6
anrikun commentedCould someone tell me what's wrong with the format of this patch?
Comment #7
damien tournoud commentedSee http://drupal.org/patch/create for more information about how to create a patch for Drupal.
I would not call this a bug per say, it's just a difference between what you expect the menu system to do and what it actually does. Anyway, bugs are fixed and discussed first in the current development version (7.x) before being eventually backported.
Comment #8
anrikun commentedI've followed http://drupal.org/patch/create throughoutly to create the patch.
That's why I don't get why it doesn't pass the test :-(
Comment #9
anrikun commentedUpdated patch:
Comment #10
drifter commentedSet the issue to the "needs review" state if you want it tested. It will fail again though, you need to create it from the drupal site root, not the menu module root.
Comment #12
anrikun commentedHere's an updated patch for 7.x.
Please consider committing it.
It does not hurt at all and will please all the many users who create parent menu items simply as containers.
I will post a backported 6.x-16 version after this one.
Comment #14
damien tournoud commentedThis is actually a feature request. I doubt it's backportable to D6, has it is an important change of behavior.
This needs test, I'm not completely convinced that your patch works as you intend it to.
Comment #15
anrikun commentedI've been using this patch on several sites for more than 1 year now and it works perfectly without any side effect.
Comment #16
anrikun commentedComment #18
anrikun commentedComment #20
anrikun commentedComment #21
anrikun commentedDrupal 7.x-dev version
Comment #22
anrikun commentedDrupal 6.16 version
Here is the same patch rolled back for Drupal 6.16:
Comment #23
anrikun commentedDrupal 6.19 version
Here is the same patch rolled back for Drupal 6.19:
Comment #24
halefxThank you
Comment #25
aanjaneyam commentedCould this patch be committed to drupal core.
Comment #26
anrikun commentedDrupal 6.20 version
Here is the same patch for Drupal 6.20:
Comment #27
cyberwolf commentedsubscribing
Comment #28
HerrSerker commentedI want this too in the drupal 6 core. It worked for me and fixed the issue with pathauto and menu firstchild modules
--edit
I changed my opinion.
Does not fix the [menupath-raw] token
I have a structure like
But automatic url aliases are
but should be
Comment #29
bensnyder commentedsubscribe - please commit :)
Comment #30
bryancasler commented+1 for D7 commit
Comment #31
anrikun commentedSeeing #24, #25, #29, #30, I guess this can changed to RTBC.
Comment #32
catchComment #33
damien tournoud commentedThis patch doesn't even apply, and just cannot be right.
It feels like we should add a
ORDER BY weightto the query inmenu_link_get_preferred(). In the current D7 code, we just pick a link as random as the preferred link if there are several links with the same path in the same menu.Comment #34
anrikun commentedYou mean on D8?
Do patches have to be for D8 now that is D7 is out?
Comment #35
damien tournoud commented@anrikun: it applies either on D7 nor D8. A lot of things have changed in this area since one year ago :)
Comment #36
anrikun commentedCode in D7 has changed a lot indeed!
If I provide a new patch, does it necessarily have to be for D8 first?
Comment #37
anrikun commentedFYI, the patch at #26 applies to Drupal 6.22 too.
Comment #38
gagarine commented#21: includes_menu_inc.patch queued for re-testing.
Comment #40
omercioglu commentedsub
Comment #41
candelas commentedsubscribe
Comment #42
adamdicarlo commentedSubscribing.
Comment #43
manObject commented#2: menu.inc_.patch queued for re-testing.
Comment #44
manObject commentedThis patch fixes a bug in the core Menu module. It has been widely tested by many satisfied users for a long period of time. The code is fine, it doesn't affect compatibility in either direction, it makes Drupal better, so why hasn't it been committed to the core yet?
Comment #45
manObject commentedThere is a coding error in the patch. The new "if" clause does not have a closing curly bracket. This would have shown up had it been properly indented.
Comment #46
bleen commentedre #36:
Yup
Comment #47
klonos...unassigning Henri since he seems to not be working on this.
Do have an agreement on this being a feature request and not a bug? If this is deemed a bug and it doesn't break any API or cause any critical issues, it could be backported to D7 and perhaps D6.
Comment #48
sylvain lecoy commentedFull support for this to land in Drupal 7.
Comment #49
klonosI consider this to be a bug in my use case. If it remains a simple feature request, I doubt it has any chance of getting in D8 (when in fact this bug goes as back as D6).
Comment #50
dariogcode commentedI also need this feature!. Breadcrumbs doesn't work well for first items. Thanks!
Comment #51
klonos...I think I'm gonna go ahead and set this to be a bug after all. Even the issue summary makes it sound like one. I honestly cannot understand under what context people consider this a feature.
Comment #52
steveoriolAt the moment, anyone know how to operate the Breadcrumb in D7 with the "FirstChild Menu" module?
In fact, it is working with "menu breadcrumb" module but not with the "path breadcrumbs" module...
Comment #53
abarpetia commented@steveoriol: Did you found solution for "FirstChild Menu" module?
Thanks
Comment #56
Syndz commentedI ran into the same problem for Drupal 8.
Here's a patch that returns the last found link instead of the first one.
Comment #57
Syndz commentedComment #59
Syndz commentedRerolled patch using git diff
Comment #65
bramdriesenAlso bumped into this issue today. Patch seems to fix the issue for me as well.
Comment #66
sam152 commentedComment #67
bramdriesenFYI: We are not using the menu block module :) but it's indeed related to this one I guess.
Comment #70
juanolalla commentedPatch #59 works as expected in 9.2.x
Comment #71
sam152 commentedThis is a behavioural change that would require some degree of configuration or backwards compatibility. It also is missing test coverage.
Comment #73
douggreen commentedThere's already a code comment in MenuActiveTrail.php that says
So I think the first pass solution is a contrib module that implements this. Before I write such a module, I'm wondering if one exists already.
Comment #78
juanolalla commentedI just released a module which just extends the MenuActiveTrail service to do exactly this: https://www.drupal.org/project/menu_active_trail_deepest