Original issue: #576290: Breadcrumbs don't work for dynamic paths & local tasks
Short version
Problem
- The breadcrumb is bogus (or entirely empty) on pages that are paths or sub-paths of dynamic menu router items.
- You cannot navigate through Drupal using the Standard installation profile, because it does not enable the Management menu block. (which makes this critical)
- Breadcrumbs do not contain the real trail for local tasks, so context can be lost.
Goal
- Make breadcrumbs match our real expectations.
- Ensure proper navigational breadcrumb links throughout Drupal
Details
- The current breadcrumb retrieval code is based on a regular menu tree. A menu link can be contained in any menu, possibly multiple times even. Currently, the menu system has a notion of "preferred menu names" (which are currently hard-coded, AFAICS). The current code does not properly take all available menus into account, and when happening to take the right menus into account, it doesn't respect the order of preference. This means that a potentially wrong menu link is used as "target" to build the active trail for a page. In turn, breadcrumbs may be empty or bogus.
This patch introduces a new function menu_link_get_preferred($path), which makes sure that the proper link from the proper menu is returned to build a menu tree.
- The current breadcrumb retrieval code entirely skips router items that contain untranslated path arguments ('%') when generating the breadcrumb. Breadcrumbs are based on regular menu trees. The menu tree building code has been enhanced with a new tree parameter + argument that introduces a special behavior when building a menu tree for breadcrumbs: We additionally try to translate all links in the breadcrumb based on the current path.
In other words: If we would be on node/3/edit, then a hypothetical breadcrumb would contain:
<front> » node/2 » node/3 » node/%/edit
Since we are on node/3/edit, the new code tries to take that current path argument map to translate untranslated paths in the breadcrumb:
<front> » node/2 » node/3 » node/3/edit
Which in turn, makes the breadcrumb work for dynamic paths.
- In current HEAD, menu router items of the type MENU_CALLBACK are considered for breadcrumbs, too. This is utterly wrong and completely contradicts the very documentation of MENU_CALLBACK itself (which is almost the same since D6, so no idea why this has been wrong for such a long time). The docs for MENU_CALLBACK clearly state that it's "A hidden, internal callback, typically used for API calls." However, it was defined as MENU_VISIBLE_IN_BREADCRUMB, which, obviously, means that such menu callbacks do appear in breadcrumbs. This broke a range of tests and expectations. Therefore, type MENU_CALLBACK is changed into 0 (zero), which has the additional performance benefit in that the menu system does no longer try to auto-generate any menu links for menu callbacks; i.e., they only exist in {menu_router}, but not at all in the {menu_links} table. Thus, without a link, they do not appear anywhere, which matches our expectations, and also its very own phpDoc.
The MENU_CALLBACK constant should be removed from all menu router items that previously defined it just because the path contained a dynamic argument. This is the only high-level API change for contributed modules (the above mentioned menu tree generation functions are normally not used, thus I'd call the rest low-level API changes).
- The current menu system tests do not contain any assertions for proper breadcrumb generation, page titles, and active trail handling in menu trees. This patch adds a massive MenuBreadcrumbsTestCase that very precisely asserts aforementioned parts in various places of Drupal core; including administration pages, scenarios with and without menu links, local tasks, multi-level local tasks, as well as other special edge-cases. It is insane that page titles and menu trees are related to breadcrumbs functionality in the first place (while "related" doesn't even cut it; that other functionality totally depends on breadcrumbs, so by altering the breadcrumbs you're altering lots of other functionality and behavior of the menu system...), but properly cleaning this up is D8 material.
API change
-
The effect of not removing MENU_CALLBACK from menu router item definitions that currently use it is:
- no breadcrumb for the path and child paths
- no corresponding link in {menu_links}
- if there are child paths/items registered, and those are not menu callbacks, then they will appear in {menu_links}, but not "below" the parent path; i.e., the menu system (re-)parenting process cannot find the parent link, so they will end up using different pX columns (perhaps even start a new tree).
I'm unable to do benchmarks on my computer, so I hope someone else can do that.
--
Different patches have been transferred from the original issue:
- #285: Adds an entirely new tree building parameter $only_active_trail, which is then used by menu_set_active_trail() to separately build a minimal tree containing the links of the active trail only, i.e., a tree that is the breadcrumb and nothing else.
- #294: Reverted $only_active_trail, contains plenty of in-code documentation about menu system internals and the problem we are trying to solve. If anything in those docs is not clear, please don't hesitate to ask.
- #295: The most recent attempt to fix the active trail behavior by including not accessible links from the active trail in menu trees, which are ignored for the regular tree output, and manually translated by menu_get_active_breadcrumb(). (It is possible that this code needs to be moved into menu_set_active_trail() to not break custom set breadcrumbs.)
Comment | File | Size | Author |
---|---|---|---|
#87 | _menu_translate-undefined-variables-907690-87.patch | 1.08 KB | pillarsdotnet |
#67 | drupal.breadcrumbs.67.patch | 1.08 KB | sun |
#55 | 907690-breadcrumb-55.patch | 120.79 KB | pwolanin |
#55 | 907690-interdiff-47-55.diff.txt | 6.49 KB | pwolanin |
#47 | drupal.breadcrumbs.interdiff.47-d6.patch | 2.24 KB | sun |
Comments
Comment #1
sun.
Comment #2
plachComment #4
klonossubscribing...
Comment #6
andypost+1
Comment #7
moshe weitzman CreditAttribution: moshe weitzman commented295 now has the same performance as unpatched. would be good to get another confirm on performance.
lets get those tests passing.
Comment #8
MustangGB CreditAttribution: MustangGB commentedSubscribe
Comment #9
sunowwwwwww. If this passes, then we're set! :)
Comment #11
sunoh yay! Easy to fix. :)
Comment #12
chx CreditAttribution: chx commentedI gave the anon user "Administer image styles" (after clicking override defaults) and tested http://localhost/7/admin/config/media/image-styles/edit/thumbnail/effects/1 with and without the "Use the administration pages and help" permission. In both case I still see a rather consistent 10% performance decrease. I still need to run xhprof and diff it to see why.
Comment #13
sunThis should fix the performance decrease.
Comment #14
sunThis should fix the performance problem.
Comment #15
aspilicious CreditAttribution: aspilicious commented1) I think #14 and#13 are the same?
2) why are there less tests in #14 o_O
Comment #16
sun1) yes, d.o varnish cache hiccup.
2) Happens all of the time, not sure why; I suspect testbot differences.
According to a (too large to attach) callgraph that chx produced, it seems like menu_link_get_preferred() is guilty for a 20ms slowdown per request. I've to study that query (explain + stuff), but if anyone has a clue, don't hesitate to share, please. ;)
Comment #17
sunRunning menu_link_get_preferred()'s query manually reports 0.0004 seconds only, so this cannot be the cause.
I'm running out of ideas. Attached patch shouldn't affect performance, but should nevertheless be used for any benchmarks.
Comment #18
drifter CreditAttribution: drifter commentedSo, I can confirm the performance loss. Using patch #17, and chx's
/admin/config/media/image-styles/edit/thumbnail/effects/1
with the correct anon permisssions and after clicking on "override defaults" for the thumbnail preset. Also tested for two other pages.Not enough data points to be sure, but the loss could be related to the depth of the current menu link?
/admin/config/media/image-styles/edit/thumbnail/effects/1 : about 10% loss
/admin/config/media : about 5% loss
/node/1 : no performace loss
Before patch:
After patch:
For a simple node with a menu link, no performance loss:
Before:
After:
At the "halfway point",
/admin/config/media
:Before:
After:
Comment #19
klonosI cannot confirm because I'd need to know more about testing performance etc, so I cannot be sure if this is the cause of it or simply a random side-effect, but that was a great catch Zoltan! Lets see if other can confirm.
Comment #20
klos CreditAttribution: klos commentedI pressed "re-test" button by accident, sorry.
Comment #21
sunHrm, well at least that particular performance decrease on dynamic paths is totally expected and only natural.
Currently, the menu system does nothing at all for those pages, which is apparent by looking at an empty breadcrumb. With this patch, like on any other page, it has to build a menu tree to be able render active trail to the menu link for the current page - this involves building a full menu tree, checking for node access, and trying to translate every link on all visible levels - identical to any other menu tree and breadcrumb that is built.
The big question is whether we're somehow able to resemble the patch's behavior on HEAD (support for dynamic paths), but most importantly, make Garland your default and admin theme, and enable the Management menu block in a sidebar. Visiting that list of test pages should have the identical performance decrease then. (Perhaps we don't even have to resemble anything at all and the menu block is sufficient already?) If that is the case, then there is no unexpected performance decrease.
Comment #22
moshe weitzman CreditAttribution: moshe weitzman commentedSo we are saying that it takes 10% of page generation to build a friggin breadcrumb for a path like /admin/config/media/image-styles/edit/thumbnail/effects/1? I guess admin paths are so edge case that we don't care. But are we now introducing deep breadcrumbs on popular pages as well?
What should we do in D8? This menu link system is not working out.
Comment #23
sunok, this is fun. :) Actually it's not, but it's one of the last options I'm able to come up with. See interdiff.
If this won't fly, then I need more profiling/benchmark data. (Sorry, my local Windows box is unusable for doing benchmarks.)
EDIT: I've manually tested and debugged this pach, so this approach would work. But it's not really guaranteed that there won't be another preprocessor being invoked after template_preprocess_page() or whatnot that might build the same menu (again).
Comment #24
sunThis could even get into extremes. :)
Comment #26
ugerhard CreditAttribution: ugerhard commented#24: drupal.breadcrumbs.24.patch queued for re-testing.
Comment #28
drifter CreditAttribution: drifter commentedWell, we're down to 5% performance decrease for the deep edge case (
/admin/config/media/image-styles/edit/thumbnail/effects/1
). Will do some more extensive benchmarking later on.Before patch:
After patch:
Comment #29
drifter CreditAttribution: drifter commented#24: drupal.breadcrumbs.24.patch queued for re-testing.
Comment #30
sunoh yay! Well, ~10ms is not so bad when considering that we're generating a full menu tree for 8 tree levels, and performing access checks on every single one. I guess you get the same times when switching to Garland and enabling the Management menu block (should actually be as slow as #18, as that really renders an entire menu tree, potentially containing other visible links on all tree levels).
However, my main concern was node/% and such. It looks like (according to #18) there's almost no performance decrease there.
--
On another note, I'm going to squeeze a hook_menu_breadcrumb_alter() into menu_get_active_breadcrumb()... I totally wasn't aware that re-building a breadcrumb has such heavy performance issues, and so I know for sure that some of the sites I've built must be slower than they have to (due to building a new custom breadcrumb).
Comment #31
sunAlthough it slightly feels like a hack, I like the latest improvements. We need to entirely rethink breadcrumbs in D8 anyway (and for that sake, I want them to become a core module), but for D7, it looks like the latest patch fixes the bug and heavily improves the situation, without substantial or unexpected performance issues.
Comment #32
moshe weitzman CreditAttribution: moshe weitzman commentedIt seems like we are now satisfied with the functionality and performance of this patch. It adds terrific test coverage. Anything else to do?
Comment #33
sunTechnically and functionality-wise, this patch should be complete. I'd love to see this issue finally fixed.
Comment #34
drifter CreditAttribution: drifter commentedOK, just to make sure. Testing on deep taxonomy pages and deeply nested nodes. Slowdown is now consistently in the 5ms range (2-3%). I think sun solved the performance issue. So, to summarize:
Are we ready?
Taxonomy page, 7 levels deep:
Home » vicuka » che » phufrode » kephadukaki » bepe » bretashethop » hedacrik
Before:
After:
Editing the same taxonomy page:
Before:
After:
Viewing a deeply nested node page:
Home » Huic Defui » Dolus Ideo Jugis Abluo » Conventio » Camur Illum Voco » Distineo Qui Te
Before:
After:
Editing the same page:
Before:
After:
Comment #35
moshe weitzman CreditAttribution: moshe weitzman commentedis this needed, and if so is it really part of this patch? we are only adding a few links to the page.
if the above static gets removed, this line can go too.
Comment #36
sunThe reason for that (fast) static is that l() is in the critical path, often called hundreds of times in a single request.
While menu_get_item() is also statically cached (but not using the advanced pattern), this would mean one extra call for each invocation of l().
I can surely do a patch + hopefully @drifter can benchmark that once more.
Powered by Dreditor.
Comment #38
sunUpdated for #914458: Remove the format delete reassignment "feature"
Comment #39
moshe weitzman CreditAttribution: moshe weitzman commentedI don't see why this patch needs to touch l() at all. Please explain why l() has to change in this issue.
Comment #40
sunMenu links are rendered using l(). If the current page is a local task that is either the default local task or a local task that is a child of a local task, then the parent link (tab_root) of the local task(s) should be active.
Current HEAD only compares a link's path directly to $_GET['q'], which marks a corresponding menu link as active in a menu tree (for instance, if there's a menu link for node/123 and you are on node/123, then that menu link is marked as "active"). However, if you are on node/123/view or on node/123/edit or any other local task, then the menu link is not marked as active.
Comment #41
moshe weitzman CreditAttribution: moshe weitzman commentedMakes sense. Though making 'active' work better seems like a different task from making breadcrumbs accurate.
Comment #42
drifter CreditAttribution: drifter commentedAnd the results are in, patches #30 and #38 are equivalent performance-wise, so we can go with #38.
taxonomy/term/37
Patch #30:
Patch #38:
node/7
Patch #30:
Patch #38:
admin/config/media/image-styles/edit/thumbnail/effects/1
Patch #30:
Patch #38:
Comment #43
chx CreditAttribution: chx commentedI have read this patch several times in the past. I have even fixed one issue with it in the original issue. I am fine with it now. I would like to see pwolanin's opinion before we get this in, however.
Comment #44
sunThanks for the reviews and benchmarks!
The question whether we have to touch l() at all here made me re-consider it. We don't have to touch it, since we only want to adjust menu links. So in attached patch, I entirely reverted the change to l() and fixed the appropriate locations in menu.inc instead. That's much cleaner now and keeps the logic internal to the menu system.
--
Side note: This entire work led to plenty of insights. I'll try to speed up various parts in menu.inc for D7 after this patch landed.
Comment #45
pwolanin CreditAttribution: pwolanin commentedSorry - didn't realize until today that this new issue was in progress.
As before - this is not a beta block and not critical, but it's important to get fixed. I'd rather see use leave MENU_CALLBACK alone and define e.g. MENU_NONE, but I don't' care that much.
At first look I like the caching optimization in function menu_tree_page_data().
Comment #46
moshe weitzman CreditAttribution: moshe weitzman commentedWe said that 'fast' variant was to be used when we had 100+ calls in a request.
Personally, I would remove 'Defaults to FALSE' since that’s obvious from the code and remove the 'only' at the end. Happens again later.
stray parenthesis
Powered by Dreditor.
Comment #47
sunThanks again, moshe!
Comment #48
aspilicious CreditAttribution: aspilicious commentedStill saying "default to FALSE" ?
Comment #49
sunYes, optional arguments need to describe their default value. See http://drupal.org/node/1354
Comment #50
aspilicious CreditAttribution: aspilicious commentedNice sun
Comment #51
sun@pwolanin: So it seems that all we're waiting for is you to sign off this patch.
Comment #52
David_Rothstein CreditAttribution: David_Rothstein commentedMostly just subscribing, but...
This is still critical (at least parts of it), because of the broken breadcrumbs in the admin interface (shown in the original screenshot). That prevents navigation of the admin section and is a regression from D6.
Most likely this was caused a long time ago by #273137: Split Navigation to User and Administration menu which makes me wonder if there isn't a simpler fix more targeted at the critical bug. But the overall change here seems nice anyway.
Comment #53
MustangGB CreditAttribution: MustangGB commentedAwesome work
Glad to see this much needed issue finally nearing closure
Comment #54
JohnAlbinI do have concerns about how modules can set an active trail for the menu trees. I do see a breadcrumb alter hook, but that insufficient. HOWEVER, I'm fine with doing a follow-up patch to this one that address that concern over in #520106: Allow setting the active menu trail for dynamically-generated menu paths.; there's no reason to hold this up since this thread + its parent #576290 is over 350 comments. :-p
Comment #55
pwolanin CreditAttribution: pwolanin commentedThere is another issue dealing with accessing default local tasks - should we remove from this patch the code related to those giving them the 'active' class? like:
Also, why are you taking the code out of function menu_enable()? We DO want that so that each menu shows up in the tree in the menu block when you're using something like Bartik as your admin theme. Adding that code back. Removing an unrelated change in modules/node/content_types.inc Fixed the code comments for assertNoResponse. Fixed the code comments for hook_menu_breadcrumb_alter
Revised patch + interdiff for sun. I think this is looking very close to RTBC.
Comment #56
sunThanks, pwolanin! Your changes work for me, so let's get this in!
Some of the 'active' class fixes are required to make this patch pass the tests, so we need to keep them. Happy to re-purpose any other existing issue to add more tests though.
Comment #57
klonosJust a couple quick questions: Does D6 benefit form this (referring to patch in #55)?
I know this hasn't even been implemented in D7, but since most of my production sites are D6 I need to know. I understand that the D6 patch in #47 has only minor changes in the literature and that the only change in actual code is the removal of
drupal_static()
(for reasons explained in #46). Checking the D6 patch in #44 though shows more actual code changes. So second question is:Are the D6 'interdiff' patches supposed to be applied sequentially or perhaps after the main patch is applied or what?
Comment #58
sun@klonos: There is no patch for D6 at all here (and there won't be). I merely used the -D6 suffix to be able to upload the interdiffs as .patch files and make the testbot ignore them.
Comment #59
pwolanin CreditAttribution: pwolanin commented@kionos - it's possible parts of this could be backported to Drupal 6 if someone is willing to do the work.
Comment #60
klonosThank you both for taking the time to reply. I now realie that interdiff files are changes between patches. As for the backport to d6, we'll need to remember to set this to 'PTBP' once resolved.
Comment #61
Dries CreditAttribution: Dries commentedAlright. Committed to CVS HEAD. Thanks.
Comment #62
rfayThe outstanding initial issue statement explains the API change.
1. Is this still accurate?
2. Does this one have BC implications and need to be announced?
Comment #63
drifter CreditAttribution: drifter commentedWe need to document:
- MENU_CALLBACK has a changed meaning
- hook_menu_breadcrumb_alter()
Anything else? I'll write up a draft.
Comment #64
catchMoving this out of the blockers, so nice to see this get in.
Comment #65
sun@randy, @drifter: Yes, the original description takes still effect. Basically, it could be translated into:
Any module: Check your hook_menu(), and only keep 'type' => MENU_CALLBACK for items that real callbacks. See http://api.drupal.org/api/constant/MENU_CALLBACK/7 for what a callback means. It can and should be removed from paths that just happen to have a dynamic path argument in it.
Menu-related modules, especially menu tree building and breadcrumb modules: Check the menu.inc and theme.inc changes in this patch to see whether you may be calling things wrongly. Fundamentally, there's no hard API change here though, so this rather targets developers who take performance into account.
And http://api.drupal.org/api/function/hook_menu_breadcrumb_alter/7 has been introduced.
Comment #66
Damien Tournoud CreditAttribution: Damien Tournoud commentedReopening. This patch introduced a bit-wise SQL operator with is non-standard:
Also, I don't get why we need this condition on the menu router at all. After all, what type of menu router can have a corresponding entry in {menu_links} if it's not either MENU_VISIBLE_IN_TREE or MENU_VISIBLE_IN_BREADCRUMB? Is that big set of conditions always true?
Comment #67
sunGood point! It looks like you're right and we can remove that entire condition. Not sure why I added it, but I suspect in order to fix handling of untranslated paths, which got fixed elsewhere in the end.
Comment #68
sunComment #69
Damien Tournoud CreditAttribution: Damien Tournoud commentedIt is actually critical, because it completely broke Drupal on PostgreSQL :)
Comment #70
sun@Damien: Either you RTBC or this is not critical. Not sure what you are waiting for.
Comment #71
sun24 hours later and still no feedback on the actually ready and no-brainer patch at hand. Also, I doubt that Postgres has an issue with those query conditions, as we use similar query conditions elsewhere in menu.inc already. Thus, I believe that #66 is bogus. However, as the testbot proved, we can still remove those conditions anyway, but that's not critical, not even major.
Comment #72
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis definitely broke PostgreSQL :)
Comment #73
webchickLooks like the tests don't seem to mind if those additional conditions are done. If this ends up breaking something, we'll need to expand our test coverage, but it looks like both Damien and sun are in agreement that this logic isn't necessary.
Committed to HEAD! Thanks!
Comment #74
markabur CreditAttribution: markabur commentedI'm seeing a couple oddities introduced by this patch. Too see the bug you need to create a second user, then:
On
user/2
, the breadcrumb links touser/1
.On
user/2/edit
anduser/2/shortcuts
, the breadcrumb links touser/1
and the title is user 1's username when it should be user 2's.Comment #75
pwolanin CreditAttribution: pwolanin commentedif so, this needs a fix + tests.
Comment #76
drunken monkeySee #925778: User edit title is broken (so is beta1-beta2 upgrade path), where webchick declared this major, not critical.
And in my opinion we should let this issue rest, it has seen enough posts as it is. ;)
Comment #77
markabur CreditAttribution: markabur commentedAh, good, seemed like it would have been reported already but I didn't see the other issue.
Comment #78
ericclaeren CreditAttribution: ericclaeren commentedsubscribing, need a solution for this issue, my panel pages with arguments can't be linked with menu items, hope this brings a fix for this too.
Comment #79
effulgentsia CreditAttribution: effulgentsia commentedCongratulations, all! This is great! Still needs a mention in http://drupal.org/node/224333 I think. Then, according to #59 and #60, should be assigned to D6, though given we can't break BC in D6, I'm somewhat suspicious of what's actually doable for that.
Comment #80
pwolanin CreditAttribution: pwolanin commentedComment #81
David_Rothstein CreditAttribution: David_Rothstein commentedThere is no documentation in the module upgrade guide yet.
Comment #82
catchLooks like this caused a regression over at #950034: Using the same source for main/secondary with a custom menu doesn't work.
Comment #83
jhodgdonchanging tag to new tagging scheme for update page
Comment #84
catchThis also undid the good work done on #910190: Entire schema cache needlessly loaded on all pages, but see #402896: Introduce DrupalCacheArray and use it for drupal_get_schema() which should stop that happening again.
Comment #85
jhodgdonI just documented this on the module update guide:
http://drupal.org/update/modules/6/7#menu_breadcrumb
I'm not sure about whether there is still a desire to port this to d6 or not -- doesn't seem like it could be? For now, just removing the tag and marking fixed.
Comment #87
pillarsdotnet CreditAttribution: pillarsdotnet commentedThis issue created two "undefined variable" errors in the
_menu_translate()
function. Patch/fix attached.Comment #88
aspilicious CreditAttribution: aspilicious commentedIsn't it better to initialize this once at the start of the control structure?
Comment #89
sunPlease create a new issue and link to it from here.
Comment #90
pillarsdotnet CreditAttribution: pillarsdotnet commentedPosted #1249794: The $tab_root_map and $tab_parent_map variables in _menu_translate() should be unconditionally defined.
Comment #91
effulgentsia CreditAttribution: effulgentsia commentedPeople here might be interested in #1843668: Move building of breadcrumb render array from template_process_page() to template_preprocess_page() which undid #24 for D8. Please comment in that issue if you believe that to be problematic.