Original issue: #576290: Breadcrumbs don't work for dynamic paths & local tasks

Short version

breadcrumbs-HEAD-broken.png

breadcrumbs-fixed.png

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.)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Issue tags: +API change

.

plach’s picture

Status: Needs review » Needs work

The last submitted patch, drupal.breadcrumbs.295.patch, failed testing.

klonos’s picture

subscribing...

andypost’s picture

+1

moshe weitzman’s picture

295 now has the same performance as unpatched. would be good to get another confirm on performance.

lets get those tests passing.

MustangGB’s picture

Subscribe

sun’s picture

Status: Needs work » Needs review
FileSize
111.12 KB

owwwwwww. If this passes, then we're set! :)

Status: Needs review » Needs work

The last submitted patch, drupal.breadcrumbs.9.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
111.17 KB

oh yay! Easy to fix. :)

chx’s picture

I 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.

sun’s picture

This should fix the performance decrease.

sun’s picture

This should fix the performance problem.

aspilicious’s picture

1) I think #14 and#13 are the same?
2) why are there less tests in #14 o_O

sun’s picture

1) 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. ;)

sun’s picture

FileSize
111.62 KB

Running 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.

drifter’s picture

So, 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:

$ ab -c1 -n500 http://d7clean.mac/?q=admin/config/media/image-styles/edit/thumbnail/effects/1
Requests per second:    5.04 [#/sec] (mean)
Time per request:       198.256 [ms] (mean)
Connection Times (ms)
Total:        193  198   5.0    198     255

After patch:

$ ab -c1 -n500 http://d7patch.mac/?q=admin/config/media/image-styles/edit/thumbnail/effects/1
Requests per second:    4.60 [#/sec] (mean)
Time per request:       217.539 [ms] (mean)
Connection Times (ms)
Total:        210  217  21.0    216     624

For a simple node with a menu link, no performance loss:

Before:

$ ab -c1 -n500 http://d7clean.mac/?q=node/1
Requests per second:    4.59 [#/sec] (mean)
Time per request:       217.757 [ms] (mean)
Connection Times (ms)
Total:        211  218   9.1    214     267

After:

$ ab -c1 -n500 http://d7patch.mac/?q=node/1
Requests per second:    4.58 [#/sec] (mean)
Time per request:       218.496 [ms] (mean)
Total:        213  218   5.7    217     258

At the "halfway point", /admin/config/media:

Before:

$ ab -c1 -n500 http://d7clean.mac/?q=admin/config/media
Requests per second:    4.85 [#/sec] (mean)
Time per request:       206.374 [ms] (mean)
Connection Times (ms)
Total:        199  206   8.6    204     273

After:

$ ab -c1 -n500 http://d7patch.mac/?q=admin/config/media
Requests per second:    4.62 [#/sec] (mean)
Time per request:       216.660 [ms] (mean)
Connection Times (ms)
Total:        210  217   3.8    216     244
klonos’s picture

I 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.

klos’s picture

I pressed "re-test" button by accident, sorry.

sun’s picture

Hrm, 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.

moshe weitzman’s picture

So 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.

sun’s picture

ok, 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).

sun’s picture

This could even get into extremes. :)

Status: Needs review » Needs work
Issue tags: -API change

The last submitted patch, drupal.breadcrumbs.24.patch, failed testing.

ugerhard’s picture

Status: Needs work » Needs review

#24: drupal.breadcrumbs.24.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +API change

The last submitted patch, drupal.breadcrumbs.24.patch, failed testing.

drifter’s picture

Well, 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:

Requests per second:    5.06 [#/sec] (mean)
Time per request:       197.624 [ms] (mean)
Connection Times (ms)
              min  mean[+/-sd] median   max
Total:        193  198   2.3    198     218

After patch:

Requests per second:    4.80 [#/sec] (mean)
Time per request:       208.312 [ms] (mean)
Connection Times (ms)
              min  mean[+/-sd] median   max
Total:        203  208   7.2    207     328
drifter’s picture

Status: Needs work » Needs review

#24: drupal.breadcrumbs.24.patch queued for re-testing.

sun’s picture

oh 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).

sun’s picture

Although 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.

moshe weitzman’s picture

It seems like we are now satisfied with the functionality and performance of this patch. It adds terrific test coverage. Anything else to do?

sun’s picture

Technically and functionality-wise, this patch should be complete. I'd love to see this issue finally fixed.

drifter’s picture

OK, 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:

  • this patch fixes breadcrumbs on dynamic paths
  • it adds a motherload of tests covering both breadcrumbs and active trails
  • fixes bugs where those tests failed
  • enables easy and performant altering of the current breadcrumb trail

Are we ready?

Taxonomy page, 7 levels deep:

Home » vicuka » che » phufrode » kephadukaki » bepe » bretashethop » hedacrik

Before:

Requests per second:    4.58 [#/sec] (mean)
Time per request:       218.164 [ms] (mean)
Total:        215  218   6.4    217     349

After:

Requests per second:    4.46 [#/sec] (mean)
Time per request:       224.145 [ms] (mean)
Total:        220  224   9.5    223     367

Editing the same taxonomy page:

Before:

Requests per second:    4.30 [#/sec] (mean)
Time per request:       232.814 [ms] (mean)
Total:        229  233   7.2    232     386

After:

Requests per second:    4.19 [#/sec] (mean)
Time per request:       238.879 [ms] (mean)
Total:        236  239   6.6    239     381

Viewing a deeply nested node page:

Home » Huic Defui » Dolus Ideo Jugis Abluo » Conventio » Camur Illum Voco » Distineo Qui Te

Before:

Requests per second:    4.80 [#/sec] (mean)
Time per request:       208.500 [ms] (mean)
Total:        205  208   3.6    207     270

After:

Requests per second:    4.70 [#/sec] (mean)
Time per request:       212.963 [ms] (mean)
Total:        210  213   1.5    213     217

Editing the same page:

Before:

Requests per second:    4.20 [#/sec] (mean)
Time per request:       238.156 [ms] (mean)
Total:        233  238   3.7    238     255

After:

Requests per second:    4.18 [#/sec] (mean)
Time per request:       239.310 [ms] (mean)
Total:        236  239   1.6    239     251
moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community
+++ includes/common.inc
@@ -2264,15 +2264,27 @@ function drupal_attributes(array $attributes = array()) {
+  // Use the advanced drupal_static() pattern, since this is called very often.
+  static $drupal_static_fast;
+  if (!isset($drupal_static_fast)) {
+    $drupal_static_fast['active'] = &drupal_static(__FUNCTION__);
+  }
+  $active_href = &$drupal_static_fast['active'];
 

is this needed, and if so is it really part of this patch? we are only adding a few links to the page.

+++ includes/menu.inc
@@ -2319,6 +2476,8 @@ function menu_reset_static_cache() {
+  drupal_static_reset('l');

if the above static gets removed, this line can go too.

sun’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.43 KB
119.88 KB
+++ includes/common.inc
@@ -2264,15 +2264,27 @@ function drupal_attributes(array $attributes = array()) {
 function l($text, $path, array $options = array()) {
...
+  // Use the advanced drupal_static() pattern, since this is called very often.
+  static $drupal_static_fast;
+  if (!isset($drupal_static_fast)) {
+    $drupal_static_fast['active'] = &drupal_static(__FUNCTION__);
+  }
+  $active_href = &$drupal_static_fast['active'];
...
+  // The active item needs to be cached by $_GET['q'] to account for an altered
+  // path via menu_set_active_item().
+  if (!isset($active_href[$_GET['q']])) {
+    $item = menu_get_item();
+    $active_href[$_GET['q']] = (($item['type'] & MENU_LINKS_TO_PARENT) == MENU_LINKS_TO_PARENT ? $item['tab_root_href'] : $item['href']);
+  }
   // Append active class.
-  if (($path == $_GET['q'] || ($path == '<front>' && drupal_is_front_page())) &&
+  if (($path == $active_href[$_GET['q']] || ($path == '<front>' && drupal_is_front_page())) &&

The 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.

Status: Needs review » Needs work

The last submitted patch, drupal.breadcrumbs.36.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
119.88 KB
moshe weitzman’s picture

I don't see why this patch needs to touch l() at all. Please explain why l() has to change in this issue.

sun’s picture

Menu 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.

moshe weitzman’s picture

Makes sense. Though making 'active' work better seems like a different task from making breadcrumbs accurate.

drifter’s picture

And the results are in, patches #30 and #38 are equivalent performance-wise, so we can go with #38.

taxonomy/term/37

Patch #30:

Requests per second:    4.35 [#/sec] (mean)
Time per request:       230.026 [ms] (mean)

Patch #38:

Requests per second:    4.37 [#/sec] (mean)
Time per request:       228.640 [ms] (mean)

node/7

Patch #30:

Requests per second:    4.60 [#/sec] (mean)
Time per request:       217.369 [ms] (mean)

Patch #38:

Requests per second:    4.64 [#/sec] (mean)
Time per request:       215.524 [ms] (mean)

admin/config/media/image-styles/edit/thumbnail/effects/1

Patch #30:

Requests per second:    4.79 [#/sec] (mean)
Time per request:       208.939 [ms] (mean)

Patch #38:

Requests per second:    4.77 [#/sec] (mean)
Time per request:       209.564 [ms] (mean)
chx’s picture

I 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.

sun’s picture

Thanks 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.

pwolanin’s picture

Priority: Critical » Major

Sorry - 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().

moshe weitzman’s picture

+++ includes/menu.inc
@@ -404,7 +404,13 @@ function menu_set_item($path, $router_item) {
+  // Use the advanced drupal_static() pattern, since this is called very often.
+  static $drupal_static_fast;
+  if (!isset($drupal_static_fast)) {
+    $drupal_static_fast['items'] = &drupal_static(__FUNCTION__);
+  }
+  $router_items = &$drupal_static_fast['items'];

We said that 'fast' variant was to be used when we had 100+ calls in a request.

+++ includes/menu.inc
@@ -778,7 +798,11 @@ function menu_tail_to_arg($arg, $map, $index) {
+ *   path. Defaults to FALSE. Internally used for breadcrumbs only.

Personally, I would remove 'Defaults to FALSE' since that’s obvious from the code and remove the 'only' at the end. Happens again later.

+++ includes/menu.inc
@@ -799,13 +825,40 @@ function _menu_link_translate(&$item) {
+      // if this $item's path is within ) the router item's path. This means

stray parenthesis

Powered by Dreditor.

sun’s picture

Thanks again, moshe!

aspilicious’s picture

Personally, I would remove 'Defaults to FALSE' since that’s obvious from the code

Still saying "default to FALSE" ?

sun’s picture

Yes, optional arguments need to describe their default value. See http://drupal.org/node/1354

aspilicious’s picture

Nice sun

sun’s picture

@pwolanin: So it seems that all we're waiting for is you to sign off this patch.

David_Rothstein’s picture

Priority: Major » Critical

Mostly 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.

MustangGB’s picture

Awesome work
Glad to see this much needed issue finally nearing closure

JohnAlbin’s picture

I 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

pwolanin’s picture

There 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:

+    if ($data['link']['href'] == $router_item['tab_root_href'] && $data['link']['href'] != $_GET['q']) {
+      $data['link']['localized_options']['attributes']['class'][] = 'active';
     }

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.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, 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.

klonos’s picture

Just 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?

sun’s picture

@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.

pwolanin’s picture

@kionos - it's possible parts of this could be backported to Drupal 6 if someone is willing to do the work.

klonos’s picture

Thank 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.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Alright. Committed to CVS HEAD. Thanks.

rfay’s picture

The 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?

drifter’s picture

Status: Fixed » Needs work
Issue tags: +Needs documentation

We need to document:

- MENU_CALLBACK has a changed meaning
- hook_menu_breadcrumb_alter()

Anything else? I'll write up a draft.

catch’s picture

Title: Breadcrumbs don't work for dynamic paths & local tasks #2 » (Document) Breadcrumbs don't work for dynamic paths & local tasks #2
Priority: Critical » Normal

Moving this out of the blockers, so nice to see this get in.

sun’s picture

Title: (Document) Breadcrumbs don't work for dynamic paths & local tasks #2 » Breadcrumbs don't work for dynamic paths & local tasks #2

@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.

Damien Tournoud’s picture

Priority: Normal » Critical

Reopening. This patch introduced a bit-wise SQL operator with is non-standard:

+    // Include links
+    // - appearing in trees (MENU_VISIBLE_IN_TREE).
+    // - appearing in breadcrumbs (MENU_VISIBLE_IN_BREADCRUMB), since
+    //   breadcrumbs are based on regular menu link trees.
+    // - not mapping to any router path (NULL).
+    $query->condition(db_or()
+      ->condition('m.type', MENU_VISIBLE_IN_TREE, '&')
+      ->condition('m.type', MENU_VISIBLE_IN_BREADCRUMB, '&')
+      ->isNull('m.type')
+    );

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?

sun’s picture

Status: Needs work » Needs review
FileSize
1.08 KB

Good 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.

sun’s picture

Priority: Critical » Normal
Damien Tournoud’s picture

Priority: Normal » Critical

It is actually critical, because it completely broke Drupal on PostgreSQL :)

sun’s picture

@Damien: Either you RTBC or this is not critical. Not sure what you are waiting for.

sun’s picture

Priority: Critical » Normal

24 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.

Damien Tournoud’s picture

Priority: Normal » Critical
Status: Needs review » Reviewed & tested by the community

This definitely broke PostgreSQL :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Looks 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!

markabur’s picture

I'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 to user/1.

On user/2/edit and user/2/shortcuts, the breadcrumb links to user/1 and the title is user 1's username when it should be user 2's.

pwolanin’s picture

Status: Fixed » Active

if so, this needs a fix + tests.

drunken monkey’s picture

Status: Active » Fixed

See #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. ;)

markabur’s picture

Ah, good, seemed like it would have been reported already but I didn't see the other issue.

ericclaeren’s picture

subscribing, 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.

effulgentsia’s picture

Assigned: sun » Unassigned
Priority: Critical » Normal
Status: Fixed » Needs work

Congratulations, 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.

pwolanin’s picture

Version: 7.x-dev » 6.x-dev
Status: Needs work » Patch (to be ported)
David_Rothstein’s picture

Version: 6.x-dev » 7.x-dev
Status: Patch (to be ported) » Needs work

There is no documentation in the module upgrade guide yet.

catch’s picture

jhodgdon’s picture

changing tag to new tagging scheme for update page

catch’s picture

This 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.

jhodgdon’s picture

Status: Needs work » Fixed
Issue tags: -Needs documentation updates

I 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.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

pillarsdotnet’s picture

Version: 7.x-dev » 8.x-dev
Status: Closed (fixed) » Needs review
Issue tags: +Needs backport to D7
FileSize
1.08 KB

This issue created two "undefined variable" errors in the _menu_translate() function. Patch/fix attached.

aspilicious’s picture

Isn't it better to initialize this once at the start of the control structure?

sun’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs review » Closed (fixed)
Issue tags: -Needs backport to D7

Please create a new issue and link to it from here.

pillarsdotnet’s picture

effulgentsia’s picture

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).

People 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.