For example, at admin/config/media/image-styles/edit/thumbnail

CommentFileSizeAuthor
#296 drupal.breadcrumbs.295.patch111.78 KBsun
#294 drupal.breadcrumbs.294.patch112.56 KBsun
#289 breadcrumbs-HEAD-broken.png24.41 KBsun
#289 breadcrumbs-fixed.png13.38 KBsun
#285 drupal.breadcrumbs.285.patch119.83 KBsun
#283 drupal.breadcrumbs.283.patch109.81 KBchx
#279 drupal.breadcrumbs.279.patch115.66 KBsun
#277 drupal.breadcrumbs.277.patch115.52 KBsun
#275 drupal.breadcrumbs.275.patch111.35 KBdrifter
#274 258-263-diff.txt4.64 KBdrifter
#268 drupal.breadcrumbs.268.patch111.45 KBsun
#267 drupal.breadcrumbs.267.patch111.42 KBsun
#266 drupal.breadcrumbs.264.patch111.22 KBsun
#263 drupal.breadcrumbs.263.patch106.61 KBsun
#258 drupal.breadcrumbs.258.patch105.36 KBdrifter
#256 drupal.breadcrumbs.256.patch104.91 KBdrifter
#254 drupal.breadcrumbs.253.patch105.12 KBsun
#252 drupal.breadcrumbs.251.patch104.17 KBsun
#250 576290-bc-249.patch79.86 KBpwolanin
#249 drupal.breadcrumbs.249.patch101.01 KBsun
#245 drupal.breadcrumbs.245.patch83.05 KBsun
#237 576290-drupal.breadcrumbs.237.patch76.51 KBpwolanin
#227 drupal.breadcrumbs.227.patch81.08 KBsun
#220 drupal.breadcrumbs.220.patch80.92 KBsun
#218 drupal.breadcrumbs.218.patch76.87 KBsun
#216 MENU_CALLBACKs.txt7.31 KBsun
#209 drupal.breadcrumbs.208.patch71.88 KBsun
#206 drupal.breadcrumbs.206.patch71.46 KBsun
#205 drupal.breadcrumbs.205.patch69.87 KBsun
#203 drupal.breadcrumbs.203.patch69.72 KBsun
#202 drupal.breadcrumbs.202.patch68.33 KBsun
#201 drupal.breadcrumbs.201.patch68.32 KBsun
#199 576290-breadcrumbs-198.patch51.46 KBpwolanin
#196 breadcrumbs.patch67.39 KBquicksketch
#191 breadcrumbs.patch76.75 KBquicksketch
#189 breadcrumbs.patch67.48 KBquicksketch
#188 breadcrumbs.patch0 bytesquicksketch
#184 breadcrumbs.patch67.14 KBquicksketch
#175 drupal.breadcrumbs.174.patch66.77 KBsun
#172 drupal.breadcrumbs.172.patch66.25 KBsun
#171 drupal.breadcrumbs.171.patch62.84 KBsun
#155 site-info-wrong-title-test.png16.68 KBjhodgdon
#151 drupal.breadcrumbs.151.patch62.87 KBsun
#148 drupal.breadcrumbs.148.patch61.24 KBsun
#147 drupal.breadcrumbs.147.patch59.8 KBsun
#146 drupal.breadcrumbs.146.patch54.25 KBsun
#144 drupal.breadcrumbs.144.patch54.2 KBsun
#143 drupal.breadcrumbs.143.patch54.2 KBsun
#141 drupal.breadcrumbs.141.patch52.56 KBsun
#140 drupal.breadcrumbs.137.patch52.57 KBsun
#136 drupal.breadcrumbs.136.patch52.32 KBsun
#131 drupal.breadcrumbs.131.patch47.06 KBsun
#130 drupal.breadcrumbs.129.patch44.74 KBsun
#128 drupal.breadcrumbs.128.patch44.8 KBsun
#127 drupal.breadcrumb-test.127.patch11.15 KBsun
#123 drupal.breadcrumb-test.123.patch8.67 KBsun
#121 search-test-page-title.patch.txt2.24 KBjhodgdon
#120 blog-entry-view.jpg21.12 KBdhthwy
#120 blog-entry-edit.jpg17.06 KBdhthwy
#120 blog-view.jpg12.75 KBdhthwy
#120 book-view.jpg12.69 KBdhthwy
#120 book-page-view.jpg14.77 KBdhthwy
#120 book-page-edit.jpg11.5 KBdhthwy
#120 book-edit.jpg10.95 KBdhthwy
#116 576290-dynamic-breadcrumbs.patch33.97 KBandypost
#114 576290-dynamic-breadcrumbs-3.patch33.69 KBandypost
#107 576290-dynamic-breadcrumbs-3.patch34.54 KBdrifter
#94 576290-dynamic-breadcrumbs-2.patch33.65 KBdrifter
#93 Screen shot 2010-05-14 at 9.53.16.jpg60.78 KBdrifter
#89 576290-dynamic-breadcrumbs.patch32.62 KBDamien Tournoud
#88 576290-dynamic-breadcrumbs-88.patch32.71 KBlotyrin
#84 drupal.dynamic-breadcrumbs.84.patch33.46 KBsun
#69 576290-69.patch33.42 KBdrifter
#67 576290-67.patch30.67 KBdrifter
#66 576290-66.patch30.65 KBdrifter
#54 D7 bad breadcrumbs.png81.88 KBjoachim
#47 Picture 66.png111.99 KBNick Lewis
#40 drupal.dynamic-breadcrumbs.40.patch35.89 KBsun
#38 drupal.dynamic-breadcrumbs.38.patch21.12 KBsun
#36 drupal.dynamic-breadcrumbs.36.patch20.75 KBsun
#36 breadcrumbs-1.png11.86 KBsun
#34 drupal.dynamic-breadcrumbs.34.patch13.5 KBsun
#27 576290-breadcrumb-logic.patch13.16 KBDamien Tournoud
#26 576290-breadcrumb-logic.patch12.32 KBDamien Tournoud
#22 576290-fix-breadcrumb-active-trail-logic.patch12.35 KBDamien Tournoud
#21 drupal.dynamic-breadcrumbs.21.patch8.87 KBsun
#20 drupal.dynamic-breadcrumbs.20.patch8.78 KBsun
#18 drupal.dynamic-breadcrumbs.17.patch7.61 KBsun
#15 drupal.dynamic-breadcrumbs.14.patch5.55 KBsun
#14 576290-fix-breadcrumb-active-trail-logic.patch3.74 KBDamien Tournoud
#12 menu-tree-page-data-on-edit.png162.42 KBsun
#10 BC-node-view.png73.06 KBsun
#10 BC-node-edit.png115.16 KBsun
#9 drupal.dynamic-breadcrumbs.9.patch4.54 KBsun
#5 breadcrumbs.png74.77 KBsun
#5 drupal.dynamic-breadcrumbs.4.patch4.43 KBsun
#3 drupal.dynamic-breadcrumbs.patch4.42 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Yes. We need to do the same as we did in #508570: Restore URL consistency for node types and menus

Sivaji_Ganesh_Jojodae’s picture

The same thing happens in content type page as well eg admin/structure/types/manage/page

sun’s picture

Component: image system » menu system
Status: Active » Needs review
FileSize
4.42 KB

Well.

Then let's make it work?

Either I'm completely missing something, or I totally don't get those rumors about breadcrumbs.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
4.43 KB
74.77 KB

breadcrumbs.png

I mean. This patch makes all regular breadcrumbs work. I really wonder whether the case of a node (edit) page that ....

scratch that. Slight adjustment solves that, too.

Status: Needs review » Needs work

The last submitted patch failed testing.

Damien Tournoud’s picture

+++ includes/menu.inc	1 Nov 2009 06:04:00 -0000
@@ -1994,35 +1994,44 @@ function menu_set_active_trail($new_trai
-      $args = arg();
+      $map = $item['original_map'];

Removing the arg() sounds like a good idea, but why using the original (ie. untranslated) map? Looks to me that you need $item['map'] here.

+++ includes/menu.inc	1 Nov 2009 06:04:00 -0000
@@ -1994,35 +1994,44 @@ function menu_set_active_trail($new_trai
+      $root_path = db_query("SELECT link_path FROM {menu_links} ml WHERE ml.menu_name IN (:menus) AND ml.link_path IN (:paths) ORDER BY LENGTH(link_path) DESC LIMIT 0, 1", array(':menus' => $menu_names, ':paths' => $candidates))->fetchField();

- you need db_query_range()
- could we use ORDER BY fit DESC here?
- I wonder if we shouldn't favor the menu of tab... there is a risk to jump to a different menu here.

+++ includes/menu.inc	1 Nov 2009 06:04:00 -0000
@@ -2052,10 +2061,11 @@ function menu_set_active_trail($new_trai
     // Make sure the current page is in the trail (needed for the page title),
     // but exclude tabs and the front page.
     $last = count($trail) - 1;
-    if ($trail[$last]['href'] != $item['href'] && !(bool)($item['type'] & MENU_IS_LOCAL_TASK) && !drupal_is_front_page()) {
+    if ($trail[$last]['href'] != $item['href'] && !drupal_is_front_page()) {

Not sure why you want to remove the condition on the tabs, but at a minimum, the comment looks inconsistent with the code.

This review is powered by Dreditor.

sun’s picture

Status: Needs work » Needs review

Removing the arg() sounds like a good idea, but why using the original (ie. untranslated) map? Looks to me that you need $item['map'] here.

'map' contains the already translated path index map (i.e. a $node object for the dynamic argument %node), but we need the original map here, because we are only interested in the path argument strings.

could we use ORDER BY fit DESC here?

No, this queries {menu_links}, not {menu_router}. The router is completely unrelated here.

- you need db_query_range()
- I wonder if we shouldn't favor the menu of tab... there is a risk to jump to a different menu here.

Good points, I'll try that.

Not sure why you want to remove the condition on the tabs, but at a minimum, the comment looks inconsistent with the code.

With this condition, the breadcrumb element "Blocks" would not be displayed on admin/structure/block/manage/%/%/configure, but you have a point here in that we can probably keep the condition, but replace $item with $non_tab_item. I'll try that. :)

sun’s picture

heh, no, we cannot use $item['menu_name'], because exactly those pages that are missing a breadcrumb currently, are no menu links, and thus, they don't have a menu_name ;)

And sorry again for the LIMIT... erm... that was probably the most stupid line of code I ever wrote (not that db_query_range() exists since like ever) ;)

So we're really close here. Attached patch contains those changes and clean-ups.

However, one small issue remains - I'll prepare that in a separate comment including screenies.

sun’s picture

FileSize
115.16 KB
73.06 KB

So here is the only remaining problem.

BC-node-view.png

BC-node-edit.png

The code that follows after the $non_tab_item retrieval determines the menu_name once again (which works properly here, i.e. "management"), and then builds the menu page data tree by passing that $menu_name.

This tree, however, only contains "Administer" for whatever reason. I currently have absolutely no clue under which circumstances this can happen.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
162.42 KB

So, uhm, yeah. That last remaining issue has nothing to do with breadcrumbs at all.

menu-tree-page-data-on-edit.png

Damien Tournoud’s picture

Status: Needs review » Needs work

Discussed that patch with tha_sun on #drupal. It turns out that the real issue is in the new menu name detection code:

    // Determine if the current page is a link in any of the active menus.
    if ($menu_names) {
      $query = db_select('menu_links', 'ml');
      $query->fields('ml', array('menu_name'));
      $query->condition('ml.link_path', $item['href']);
      $query->condition('ml.menu_name', $menu_names, 'IN');
      $result = $query->execute();
      $found = array();
      foreach ($result as $menu) {
        $found[] = $menu->menu_name;
      }
      // The $menu_names array is ordered, so take the first one that matches.
      $name = current(array_intersect($menu_names, $found));
      if ($name !== FALSE) {
        $tree = menu_tree_page_data($name);
        list($key, $curr) = each($tree);
      }
    }

This condition:

      $query->condition('ml.link_path', $item['href']);

... doesn't match because it is trying to compare a path (like admin/structure/block/manage/%/%) with an href (like admin/structure/block/manage/search/form).

Changing that fixes the active trail detection and as a consequence both the breadcrumb and the menu expansion.

Damien Tournoud’s picture

Alpha quality patch:

- fixes the href vs. path issue in menu_set_active_trail() (the exact logic needs to be discussed, we could also use the same logic as below)
- fixes the parent detection code in menu_tree_page_data(). The idea is that we build a list of candidate paths (the href, the path, the tab_root href, the tab_root path, etc.), and resolve the conflict in PHP. There should only be a couple of results anyway.

sun’s picture

Status: Needs work » Needs review
FileSize
5.55 KB

Awesome debugging session. This is the cut down patch, but we're working on a fix for menu_tree_page_data() now, so that must be merged afterwards.

sun’s picture

+++ includes/menu.inc
@@ -2016,14 +2021,19 @@ function menu_set_active_trail($new_trail = NULL) {
       $query = db_select('menu_links', 'ml');
       $query->fields('ml', array('menu_name'));
-      $query->condition('ml.link_path', $item['href']);
+      $query->condition(db_or()
+        ->condition('ml.link_path', $item['path'])
+        ->condition('ml.link_path', $item['href'])
+      );
       $query->condition('ml.menu_name', $menu_names, 'IN');
+      $query->orderBy('ml.hidden', 'DESC');
       $result = $query->execute();

We additionally discussed that this orderBy('hidden') will break #204077: Allow menu links pointing to dynamic paths, so we want to find a better solution here. Most probably, not in SQL, but PHP.

This review is powered by Dreditor.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Title: breadcrumb vanishes when editing an image style » Breadcrumbs don't work for dynamic paths and local tasks
Status: Needs work » Needs review
FileSize
7.61 KB

So this patch is a merge of those patches. And, YEAH, this fixes all the breadcrumbs!

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
8.78 KB

So this one contains some additional code by Damien - which additionally fixes breadcrumbs for books.

However, those changes removed the notion of "preferred menus". It was the introduced additional condition for $item['path'] that broke breadcrumbs for books. Because the fetched menu names from the DB are intersected with the $menu_names, but keeping the order of $menu_names, in which 'navigation' comes before the book menu. But due to the new db_or() 'href' or 'path', the query finds node/123 AND node/% -- and the latter is invisible in the navigation, but that invisible link in the navigation was preferred over the book menu.

So this patch fixes that, but removes the preferred menus functionality. Which means: When the identical link appears in more than one menu, then an arbitrary one is used.

uhm, that said. Perhaps we can stuff $menu_names into an orderBy() ? :)

sun’s picture

So, erm.... I guess Damien will kill me for this new orderBy() code. It works, but yeah... :-D

Damien Tournoud’s picture

I think we are getting close with something like this.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Seems we have some failing tests here now. :-/

btw, quicksketch came up with a very similar approach some time ago over in #483614: Better breadcrumbs for callbacks, dynamic paths, tabs - though the changes applied here are looking more solid to me, especially, because they do not change APIs.

sun’s picture

Hmmm... is it possible that we all tested with admin_menu being installed, which alters the behavior of local tasks in the menu system?

Apparently, only MENU_CALLBACKs are normally stored in {menu_links}, as outlined in #631550: Stale + improper logic for MENU_VISIBLE_IN_TREE and MENU_VISIBLE_IN_BREADCRUMB

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
12.32 KB

Rerolled, given in offering to the testing bot.

Damien Tournoud’s picture

Actually, we are doing something really wrong in _menu_link_translate(): we try to complete a menu link with to_arg functions (?!):

    $map = explode('/', $item['link_path']);
    if (!empty($item['to_arg_functions'])) {
      _menu_link_map_translate($map, $item['to_arg_functions']);
    }
    $item['href'] = implode('/', $map);

This makes *absolutely* no sense: $map here contains strings, not loaded objects.

Fixed here. But I fear that we will need to come up with a comprehensive set of unit tests for those part of the menu system.

Status: Needs review » Needs work

The last submitted patch, 576290-breadcrumb-logic.patch, failed testing.

mcrittenden’s picture

Subscribe.

Status: Needs work » Needs review

Re-test of 576290-breadcrumb-logic.patch from comment #27 was requested by Damien Tournoud.

Status: Needs review » Needs work

The last submitted patch, 576290-breadcrumb-logic.patch, failed testing.

sun’s picture

I moved that fix for the @todo in menu_save() into #473082: Add custom menu API, where it was introduced.

sun’s picture

Status: Needs work » Needs review
FileSize
13.5 KB

Also note that #631550: Stale + improper logic for MENU_VISIBLE_IN_TREE and MENU_VISIBLE_IN_BREADCRUMB should allow us to finally base the breadcrumb trail on the MENU_VISIBLE_IN_BREADCRUMB constant, which had no real meaning before.

For now, just uploading the last patch, minus that custom menu API fix.

Status: Needs review » Needs work

The last submitted patch, drupal.dynamic-breadcrumbs.34.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
11.86 KB
20.75 KB

This should fix the exceptions in field_test module. However:

breadcrumbs-1.png

Status: Needs review » Needs work

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

sun’s picture

Status: Needs work » Needs review
FileSize
21.12 KB

Sorry.

Status: Needs review » Needs work

The last submitted patch, drupal.dynamic-breadcrumbs.38.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
35.89 KB

Status: Needs review » Needs work

The last submitted patch, drupal.dynamic-breadcrumbs.40.patch, failed testing.

sun’s picture

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

Since I believe that we can vastly simplify this patch, and also remove the hack for skipping local tasks (due to proper usage of MENU_VISIBLE_IN_BREADCRUMB), I expect a very minor, but still a slight API change here.

bdragon’s picture

Subscribing.

Dave Reid’s picture

Marked #692868: Losing breadcrumbs on pages as a duplicate of this issue.

Dave Reid’s picture

Priority: Normal » Critical

I feel like this is a very critical bug as it will cause new users to get lost since we lose all breadcrumbs and context. Only one test failure, seems like its close.

@sun: How is it going on your simplification?

Damien Tournoud’s picture

Status: Needs review » Needs work

Thinking about this, _menu_fill_wildcards() is just a hack. We should use _menu_link_map_translate() (and thus *_to_arg() functions) more consistently, and some of the issues we are trying to fix here are just due to the fact that we don't have a *_to_arg() function for every loader in core.

Nick Lewis’s picture

FileSize
111.99 KB

These changes allow a solid fix for #520106: Allow setting the active menu trail for dynamically-generated menu paths. as well. I just set a node's parent dynamically as the site configuration page! (see image expanded menu & breadcrumbs)

However we need to add a drupal_alter to menu_link_get_prefered as shown below:

function menu_link_get_prefered($href = NULL, $check_access = TRUE) {
// ...
    if ($item['tab_parent']) {
      // 3. The href of the current item tab root (if its exists).
      $path_candidates[] = _menu_fill_wildcards($item['tab_root'], $item['original_map']);
      // 4. The path of the current item tab root (if its exists).
      $path_candidates[] = $item['tab_root'];
    }
    // allow modules to dynamically set an menu parent
    drupal_alter('path_candidates', $path_candidates);
// ...

Here's the code I just used to set node/7's path to the admin site configuration page.

function hook_path_candidates_alter(&$path_candidates) {
  if($node = menu_get_object('node')) {
    if($node->nid == 7) {
      // wtf not?
      $path_candidates[0] = 'admin/config/system/site-information';
    }
  }
}

What say you -- think that drupal_alter can get added into this patch? If so we can mark #520106 as a dup.
*(my spellcheck says its preferred btw)

Crell’s picture

Subscribing.

webchick’s picture

Issue tags: +Needs tests

This patch looks quite enormous, and I'm getting a little turned around by the discussion here.

Can someone summarize what this is actually fixing, and what API changes this brings about, so I can make a judgment call on whether it's D7 or D8 material? Also, I don't see any new test hunks. Since breadcrumb issues have plagued us since the dawn of time, we should add some to document the way this is meant to work so we don't break them again.

Finally, I do not see a chx, pwolanin, or quicksketch on this issue. That's a bug. ;)

Nick Lewis’s picture

Title: Breadcrumbs don't work for dynamic paths and local tasks » Breadcrumbs don't work for dynamic paths & local tasks / no way to assign a menu parent dynamically

There is no way to dynamically assign a menu parent (e.g. a node, a user, a view) in our current system. This patch has cleaned up the process of building menu trails and breadcrumbs enough that I was easily able to add an alter hook to cleanly change an items menu parent. This fixes a long standing regression bug from d6. #520106: Allow setting the active menu trail for dynamically-generated menu paths.. It also fixes the annoying affinity the menu system has to the navigation menu, requiring hacks to make breadcrumbs use the primary-links menu trees (behavior described in #689890: Breadcrumbs don't try hard enough.). I'll have to reopen those bugs if this patch can't get through.

Below two screenshots describe the bug.
In this first screenshot we see how a menu is supposed to look when a node is assigned a parent menu item. (this node is actually in the menu_links table):
http://drupal.org/files/issues/correctbehavior.png

In this second screenshot I use the standard hack for "dynamically" expanding a menu tree from a node page without a record in the menu_links table. See code sample: http://drupal.org/node/520106#comment-2501876.
http://drupal.org/files/issues/pretendingtobetaxonomy.png

The key thing to look at is the difference in the breadcrumbs. In the second screenshot, I've forced the menu tree to expand by pretending to be the taxonomy term's page taxonomy/term/2. So the breadcrumb is correct, but its not what I wanted. The breadcrumb in the first screenshot is what I want. But in d6, I can't get what i want unless I use the hack, and futz with the breadcrumbs as well to get them to behave correctly.

I will reroll this patch, and add the alter hook tonight unless anyone protests.

sun’s picture

Count this as protest. This issue is about fixing system-wide breadcrumbs, not about introducing a new hook. It's hard enough to get this patch done, so let's not add unrelated noise, please.

Nick Lewis’s picture

Title: Breadcrumbs don't work for dynamic paths & local tasks / no way to assign a menu parent dynamically » Breadcrumbs don't work for dynamic paths & local tasks

10-4. New hook goes in after this patch.

Alan D.’s picture

Great work sun. Sanity in the trails is just pure gold. (#40 patch - no OpenID)

Usability increased 150% as context is maintained.

As per chx comment. I was searching for a way to create a menu structure under the user tabs. Currently, all context is lost once you get a couple of layers in. This patch allows you to keep context in a menu like the following: All links are local tasks except the few callbacks noted.

Key: [BC: The breadcrumb path]

 User
   - View (account), ...
   - Albums
     - List
     - Add album
     - %album [callback]
       - View / List album photos [BC: Home › admin  › User albums]
       - Edit [BC: Home › admin  › User albums › Test album 1]
       - Delete [callback]
       - Add photo
       - %photo [callback]
         - View [BC: Home › admin  › User albums › Test album 1]
                   [Old BC: Home]
         - Edit [BC: Home › admin  › User albums › Test album 1 › Test photo 2]
                  [Old BC: Home › Test photo 2]
         - Delete [callback]

It doesn't get all breadcrumbs right though. I've got two bundle admin links pointing to secondary local tasks. First issue (not related) is the non-basic display settings tabs do not show as they are now 3rd level tabs, but easy workaround adding local task to these tabs. The breadcrumbs have lost their context for the Manage fields / Manage display secondary tabs, [BC: Home]. The additional Manage display tabs are [BC: Home › Manage display]. All of the core tabs that I checked were correct, but all core bundle admin links point at the top level local tasks.

joachim’s picture

FileSize
81.88 KB

This is the sort of problem I filed this issue about; I've not tested the patch.

D7 bad breadcrumbs.png

Alan D.’s picture

These are my breadcrumbs for Taxonomy edit, post patch #40

Home » Dashboard » Structure » Taxonomy » My Vocab.

It would be nice to have the edit term link in the admin section as "admin/structure/taxonomy/term/%/edit" rather than "taxonomy/term/%/edit" as this switch from the admin area to the content area is not a nice UI, particularly when you have an admin theme defined.

quicksketch’s picture

I'm trying to figure out (yet again) some title and breadcrumb weirdness, so here we are again. I applied sun's patch and it seems to be working pretty well, though there is a problem specifically with nodes in the breadcrumb. If you visit "node/x/edit", where the node title should be there's just an empty space, so you get a breadcrumb like "Content » " instead of "Content » Node Title". To fix this problem we need to use the menu system to set the title instead of drupal_set_title(), which we should probably avoid anywhere possible. See #737792: Use a "title callback" for node/%node instead of drupal_set_title().

moshe weitzman’s picture

Count me as one who thinks our breadcrumbs are buggy as hell and patches that fix them (like this one) are always eligible for commit. So, lets do this. Subscribe.

andypost’s picture

Subscribe.

drifter’s picture

sub

catch’s picture

It would be nice to have the edit term link in the admin section as "admin/structure/taxonomy/term/%/edit" rather than "taxonomy/term/%/edit"

We don't do that for nodes, why do it for taxonomy terms?

Kiphaas7’s picture

sub

Alan D.’s picture

We don't do that for nodes, why do it for taxonomy terms?

Never really noticed this before, but with an administrative theme this really stood out when as a change of context that could be confusing for newbs. (UI issue) Without tagging, the administrative tasks are in the admin area with the exception of the term list / edit, so the context behind the relationship between the containing vocab and term is lost.

Just an observation. :)

andypost’s picture

@Alan D. Agreed exactly!

I think that checkbox "Use the administration theme when editing or creating content" at admin/appearance should be appended with comment about taxonomy terms or we need just clear comment about what "creating content" means...

EDIT: On clean install I cant view list of tags in vocabulary admin/structure/taxonomy/tags but I can use this as fields in posts and edit at taxonomy/term/4/edit... is there any issues about already?

catch’s picture

Note that users are also edited at user/n/edit, not admin/people/1/edit - either way, if you really want to change this behaviour, could you open a new issue for it? Doesn't have anything to do with this patch.

Nick Lewis’s picture

Re#64 agree.

Re: #55 This patch is not about that issue *however*, in #47 I describe a one line hook (one line drupal_alter addition to menu_link_get_preferered()). In the screenshot of #47 I show what happens in d7 when i run that code: A simple hook makes node/7 and only node of nid 7 think that its actually a child of admin/site-information. So in a sense the message is "even though it wasn't intend, and this patch doesn't concern that specific issue, this patch will fix your issue!". Sign of a good patch imo.

What's going on with this patch anyway? Should someone take a stab on getting this patch to pass tests?

p.s. If you're reading the code in the patch, be sure to pay attention to this function -- it appears to have made the drupal menu api sensible. (note the crazy lines removed, and the easily understood code that replaces them thanks to this little function...)

*
* @param $path
 *   The path, for example node/5. The function will find the corresponding
 *   menu link (node/5 if it exists, or fallback to node/%).
 * @return
 *   A fully translated menu link, or NULL if not matching menu link was
 *   found. The most specific menu link (node/5 prefered over node/%) in the
 *   most prefered menu (as defined by menu_get_active_menu_names()) is returned.
 */
function menu_link_get_prefered($href = NULL, $check_access = TRUE) {
drifter’s picture

Status: Needs work » Needs review
FileSize
30.65 KB

Here goes... this fixes #36 (User name link not showing). Trouble was, to_arg functions were not being called in _menu_link_translate() - this affected the $items['user/%user_uid_only_optional'] menu item, for instance. It would return a href of 'user/%', and would get rejected a few lines down:

    if (strpos($item['href'], '%') !== FALSE) {
      $item['access'] = FALSE;
      return FALSE;
    }

So, re-added the to_arg callbacks so the incantations are now:

    $item['href'] = _menu_fill_wildcards($item['link_path'], arg());
    $map = explode('/', $item['href']);
    if (!empty($item['to_arg_functions'])) {
      _menu_link_map_translate($map, $item['to_arg_functions']);
      $item['href'] = implode('/', $map);
    }

I don't claim I actually understand the menu system, but it made the test pass for me :)

drifter’s picture

FileSize
30.67 KB

Test bot is stuck, retrying... also, corrected spelling of 'prefered' to 'preferred'.

Status: Needs review » Needs work

The last submitted patch, 576290-67.patch, failed testing.

drifter’s picture

FileSize
33.42 KB

Overlooked some rejects for option widget tests in sun's original #40 patch.

drifter’s picture

Status: Needs work » Needs review
Nick Lewis’s picture

Okay guys I consider myself so obsessed with this patch that it doesn't feel like I'm the one to mark it RTBC. (I would have done that 3 months ago if i could).

In summary my views:
1. The menu system becomes understandable
2. #520106 can be closed with a simple one line drupal alter hook. (which works insanely well btw).
3. I work on drupal 40 hours a week and this single bug (which is in drupal 6) is probably my biggest timewaster and hair puller.

Someone please give a nod or voice your concerns.
*This fixes menu trails and breadcrumbs. For real.*

pwolanin’s picture

@nicklewis - "1. The menu system becomes understandable" is surely not true. At the moment it's totally understandable (at the level of code), it just doesn't do what you expect it to do.

So - given that people want breadcrumbs to work as they expect, perhaps we need something like this - I'd certainly agree that the current code totally breaks down when we are dealing with dynamic paths.

Nick Lewis’s picture

@pwolanin -- well, okay, perhaps i exaggerated. Whoever wrote the book module obviously understood why this function was necessary
http://api.drupal.org/api/function/book_build_active_trail/6 - anyone want to explain that one? or why the book module is calling menu_tree_all_data all the time? (more importantly, what is it doing with the results?) Wasn't it sort of a test of the new menu system. I won't deny that i'm an idiot, but I literally am completely lost as to where I find the place where I can cleanly influence the menu system without a hack in the current implementation or completely reverse engineering the book module implementation (which btw, wouldn't help because those menu items aren't editable through the menu system).

In the current code, I've spent *years* trying to figure it out in the current implementation.

Patch applied figured it out in less then 10 minutes.

That's what i mean by "understandable".

chx’s picture

Status: Needs review » Needs work

http://www.webchick.net/patch-reviewers-are-not-clairvoyant

Give me a summary that says: I am on path X, the breadcrumb should be Y but it's Z currently. Do this for several different kinds of X. Apparently one kind is an admin path with a dynamic element. I would be curious for node/1 and taxonomy/term/1 and node/1/edit too.

chx’s picture

Adding tag.

JohnAlbin’s picture

Breadcrumb bugs drive me batty as well. And I'd love for them all to be fixed. But I agree with chx; this issue needs to explain better how to test the patch.

Show me the use cases and the edge cases and I'll do a thorough review. :-)

I did do a review, I just don't know if its thorough, so marking it to RTBC is premature.

When I go to: admin/structure/types/manage/article/display/rss, I see this:

without patch: Home » Article
with patch: Home » Dashboard » Structure » Content types » Article » Manage display (Including two levels of tabs! Nice!)

When I go to: node/1/edit, I see this:

without patch: Home »
with patch: Home » Content »

So those parts work great. What else does this patch do? Wait. Wasn't this the issue Sun described in #5? "Content" (which is the /node list of content) shouldn't be the breadcrumb, the "view" of the node should be the breadcrumb.

JohnAlbin’s picture

Also, if a node has a menu link, its breadcrumb display fine on the "view" tab, but gets replaced with "Home » Content »" when on the "edit" tab.

Nick Lewis’s picture

Yeah, I see that too. That's very interesting @JohnAlbin.

The problem seems to boil down to node, node/%node , node/%node/edit being in the navigation menu (according to menu_links) table...
I'm still investigating but I think the internal dialog of the menu system is something along these lines.

"okay, lets see, this is node/2/edit -- i see that node/2 is in main-menu, but at the same time I also see that node, node/%node, and node/%node/edit are in the navigation menu... hrm, feels like navigation menu has better handle on this page so I'm going to use its breadcrumbs."

...the investigation is continuing. Something fishy is going on, and I think the problem is how node/%node is handled, not how the menu system is handling it...

Nick Lewis’s picture

The problem seems to ONLY be node/%node tasks. All other pages either show the same behavior, or in the case of the complicated pages *fix something that is completely broken.*

NO CHANGE APPARENT

PAGE WITH PATCH | WITHOUT PATCH
user/%user/edit home > username | home > username
taxonomy/%term/edit home > term | home > termname

REGRESSION
PAGE WITH PATCH | WITHOUT PATCH
node/%node/edit content | home > nodetitle

FIXED
PAGE WITH PATCH | WITHOUT PATCH
admin/structure/taxonomy/%vocab/edit dashboard > structure > taxonomy > vocabname | [empty]
admin/structure/types/manage/article/display/search Dashboard » Structure » Content types » Article » Manage display | Article

I have no idea what's causing the issue with nodes. Any ideas?

pwolanin’s picture

options potentially also include putting multiple levels of callbacks intot he menu links table - that would be the preferred use of the API. I thought sun had a different issue to fix it that way?

Nick Lewis’s picture

Ironically, I remembered that my proposal in #47 http://drupal.org/node/576290#comment-2515376 actually had a potential use here. From the discussion above, it seems clear that node tabs are an exception not a rule. The path candidates alter hook is built to handle such exceptions.

My first test fixed the edit page. Here's the gist of the code. (working on a cleaner implementation of course...)

// menu.inc  (line 2131)
 drupal_alter('menu_path_candidates', $path_candidates, $item);

//node.module
/**
 * Implements hook_menu_path_candidates_alter(). maybe find a better?
 * (need a comment here to probably explain why node tabs are a special case)
 */
function node_menu_path_candidates_alter(&$path_candidates, $item) {
  if($item['path'] == 'node/%/edit') {
    // i'm working out a way to apply this behavior only on local task. 
    // arg actually feels like an alright way to figure out the argument in this context...
    $path_candidates[0] = 'node/'.arg(1);
  }
}
klonos’s picture

subscribing...

cha0s’s picture

Patch needs a reroll.

sun’s picture

Assigned: Unassigned » sun
Status: Needs work » Needs review
FileSize
33.46 KB

Plain re-roll. Nothing to review.

Summary... follows, but as test.

Status: Needs review » Needs work

The last submitted patch, drupal.dynamic-breadcrumbs.84.patch, failed testing.

Damien Tournoud’s picture

_menu_fill_wildcards() is a hack that needs to go away. The proper way to do this is call the _menu_link_map_translate() function, and we need to implement a _to_arg() function for each loader in core.

lotyrin’s picture

subscribe

lotyrin’s picture

Status: Needs work » Needs review
FileSize
32.71 KB

I believe this should fix the exceptions in testing. Also rerolled against HEAD.

Damien Tournoud’s picture

Killed _menu_fill_wildcards(). Any modification suggested in #88 not included (I was working on this in parallel).

lotyrin’s picture

Looks like my fixes in #88 didn't fix anything. Ignore them.

Status: Needs review » Needs work

The last submitted patch, 576290-dynamic-breadcrumbs.patch, failed testing.

yoroy’s picture

The randomly generated term name is present.	Other	taxonomy.test	495	TaxonomyTermTestCase->testTermInterface()

Is the one failing test in #89

drifter’s picture

The test is failing because there's no breadcrumb on the term edit page. See attached photo. Without the patch, the term breadcrumb is visible.

drifter’s picture

Status: Needs work » Needs review
FileSize
33.65 KB

OK, so the same thing happens with menus too. Without a patch, editing a menu gives the breadcrumb:

Home » Dashboard » Structure » Menus » Main menu

With the patch:

Home » Dashboard » Structure » Menus

The menu tests pass because the test cases were changed from assertText to assertRaw, so instead of detecting the breadcrumb it now detects the textfield value.

I can change the taxonomy testcases accordingly (patch attached), and tests should pass. But isn't this a regression, shouldn't those breadcrumbs be there?

chx’s picture

Where is the summary? Nick Lewis had one before but with new patches probably new summaries are necessary. Will try to review the code nonehtless

chx’s picture

Thanks for the work.

While I can image this working (hey I can imagine anything working with breadcrumbs for we have no specification of what constitutes "working" here and I insist on "summaries" which really are specifications) , the code could use a lot better and more precise inline comments. Also I am a bit confused about what menu_link_get_preferred returns. If you want a "fully translated menu link" then you need to call _menu_link_translate and return an array. Edit: But instead, this function returns a "translated menu link path" which is fine but that's not what the doxygen says. see next comment.

Throughout the function we have "the path of..." and the "href of...". i would much rather say " // 3. The translated path of the tab root of the current item". What "current item" btw? Why it can't just be " // 3. The translated path of the tab root of this menu item". and for "the path of" what about "the router path of"? We have a "// FIXME: tab_root is not a href, but a wildcard path." which confuses me. The tab_root is a router path of course because that's how the router table is keyed. I am not sure what you want to fix here. You mean that menu_get_item always should translate it's tab root too? I am not sure why it should. File an issue please instead of the fixme to discuss if that's what you meant or tell me what this wants to fix.

And anyways, what's this list? The comment says "We look for the correct menu link by building a list of candidate paths. We pick the most relevant path in the preferred menu." first of all, I would avoid the word "path" in itself like the plague -- use "link path" here. Because if I am not mistaken, this wild pile of translated and untranslated router paths is assembled because we are looking for a menu link path which points to one of them. That's what $query->condition('ml.link_path', $path_candidates, 'IN'); tells me.

I am sure this loop can be written without three continue (I am sure the die is a debug mistake in there) and maybe even without a break although dropping the break is a matter of taste, I have included two break-less variants. Adding back the break is trivial I think.

    foreach ($path_candidates as $path) {
      if (!isset($preferred_links[$href]) && isset($candidates[$path])) {
        foreach ($menu_names as $menu_name) {
          if (!isset($preferred_links[$href]) && isset($candidates[$path][$menu_name])) {
            $candidate_item = $candidates[$path][$menu_name];
            $map = explode('/', $href);
            _menu_translate($candidate_item, $map);
            if ($candidate_item['access']) {
              $preferred_links[$href] = $candidate_item;
            }
          }
        }
      }
    }
    foreach ($path_candidates as $path) {
      foreach ($menu_names as $menu_name) {
        if (!isset($preferred_links[$href]) && isset($candidates[$path]) && isset($candidates[$path][$menu_name])) {
          $candidate_item = $candidates[$path][$menu_name];
          $map = explode('/', $href);
          _menu_translate($candidate_item, $map);
          if ($candidate_item['access']) {
            $preferred_links[$href] = $candidate_item;
          }
        }
      }
    }
chx’s picture

Status: Needs review » Needs work

More about the crucial ""We look for the correct menu link by building a list of candidate paths. We pick the most relevant path in the preferred menu". There are more problems here, because

  1. Compile a list of menu link candidates
  2. Go to the database and find which candidates are really menu links
  3. Process the results and pick the winner.

Which can be translated like "First, a list of paths are collected in order of preference. Then menu links pointing to these paths are retrieved from the database. Finally, the mostrelevant link in the most relevant menu is translated and returned". See, i used "paths' because these are router or links paths -- these are just paths and we look for links pointing to them. Yes. Takes a bit to grok.

Now I see better, that _menu_translate should be _menu_link_translate

plach’s picture

moshe weitzman’s picture

So, how to move forward here? This is critical, with little recent progress.

bangalos’s picture

Summary Of Main Issues.

We are restating the issues that have been brought up in discussion over the past 100 posts. There are 2 main issues listed below. It might help to break them into separate issues in the issue queue.

1. The issue on Taxonomy is that it does not contain enough breadcrumbs [Refer to original post and comment #54]
2. The issue on nodes is that it shows "Content" in the breadcrumbs when you are in node/%node/edit tab. The breadcrumb shows "Home >> Content >>" instead of "Home >> node-title" [Refer to post #56 and #76]

What the patch from Comment #94 does:

Before Patch:
On admin/structure/taxonomy/fruit page , the breadcrumb shows "Home".
On admin/structure/taxonomy/fruit/edit page , the breadcrumb shows "Home >> Fruit".

On node/1 page , the breadcrumb shows "Home" (The parent item of node 1 is )
On node/2 page , the breadcrumb shows "Home >> Foo" (The parent item of node 2 is Foo)

On node/1/edit page , the breadcrumb shows "Home >> Content >>" (Content is link to /node)
On node/2/edit page , the breadcrumb shows "Home >> Content >>" (Content is link to /node)

After Patch:
On admin/structure/taxonomy/fruit page , the breadcrumb shows "Home >> Dashboard >> Structure >> Taxonomy".
On admin/structure/taxonomy/fruit/edit page , the breadcrumb shows "Home >> Dashboard >> Structure >> Taxonomy"

On node/1 page , the breadcrumb shows "Home" (The parent item of node 1 is )
On node/2 page , the breadcrumb shows "Home >> Foo" (The parent item of node 2 is Foo)

On node/1/edit page , the breadcrumb shows "Home >> Content" (Content is link to /node)
On node/2/edit page , the breadcrumb shows "Home >> Content" (Content is link to /node)

===========================================================================

Summary of what this patch needs to do additionally before it can be accepted:

Instead of
On admin/structure/taxonomy/fruit/edit page , the breadcrumb shows "Home >> Dashboard >> Structure >> Taxonomy"
Should Be:
On admin/structure/taxonomy/fruit/edit page , the breadcrumb should show "Home >> Dashboard >> Structure >> Taxonomy >> Fruit"

Instead of
On node/1/edit page , the breadcrumb shows "Home >> Content" (Content is link to /node) (The parent item of node 1 is )
Should Be:
On node/1/edit page , the breadcrumb should show "Home >> Foo" (Foo is the title of node-1)

Instead of
On node/2/edit page , the breadcrumb shows "Home >> Content" (Content is link to /node)(The parent item of node 2 is Foo)
Should Be:
On node/2/edit page , the breadcrumb should show "Home >> Foo >> Bar" (Bar is the title of node-2) (Foo is the title of parent item of node-2 (which is node-1))

YesCT’s picture

nice summary. thanks.

pkiraly’s picture

Hi bangalos,

at my machine, and with freshly installed D7 (2010-06-25) I receved slightly different breadcumbs for node/1/edit and node/2/edit without applying the patch.

node/1/edit page:
your version: "Home >> Content >>" (Content is link to /node)
my version: "Home >> < title of node1 >" (< title... > is link to node/1)

node/2/edit page:
your version: "Home >> Content >>" (Content is link to /node)
my version: "Home >> " (HTML: < a href="/" >Home< /a > » < a href="/node/2" >< /a >)

After applying the patch mine version was the same as yours.

Regards,
Péter

lotyrin’s picture

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

#94: 576290-dynamic-breadcrumbs-2.patch queued for re-testing.

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

The last submitted patch, 576290-dynamic-breadcrumbs-2.patch, failed testing.

lotyrin’s picture

There have been some changes to the menu system since this was last rolled. No longer applies.

philbar’s picture

Issue tags: +beta blocker

tag

drifter’s picture

Status: Needs work » Needs review
FileSize
34.54 KB

Here's a reroll. There have been major changes in #620618: Optimize menu tree building and use it for toolbar (http://drupal.org/node/620618#comment-3014330), I tried to merge it correctly but someone involved in that patch should definetly review it.

An aside: just recently found out that the Devel module also has a "Menu item" debug link in the block, comes in very handy when you need to dive deep into menu router items.

Status: Needs review » Needs work

The last submitted patch, 576290-dynamic-breadcrumbs-3.patch, failed testing.

drifter’s picture

Weird, I ran some tests using both the web interface and run-tests.sh, and it ran fine...

drifter’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -API change, -beta blocker

Status: Needs review » Needs work

The last submitted patch, 576290-dynamic-breadcrumbs-3.patch, failed testing.

gengel’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Needs tests, +API change, +beta blocker

The last submitted patch, 576290-dynamic-breadcrumbs-3.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
33.69 KB

Same patch but without .htaccess hunk

Status: Needs review » Needs work

The last submitted patch, 576290-dynamic-breadcrumbs-3.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
33.97 KB

Fixed last test

dhthwy’s picture

Status: Needs review » Needs work

You guys still haven't properly addressed #96.

Farhang Darzi’s picture

Status: Needs work » Needs review
dhthwy’s picture

Status: Needs review » Needs work
FileSize
10.95 KB
11.5 KB
14.77 KB
12.69 KB
12.75 KB
17.06 KB
21.12 KB

There are different types of breadcrumbs (which can lead to confusion), If anyone is confused which type of breadcrumb Drupal uses then you can visit
http://www.visiblearea.com/cgi-bin/twiki/view/Patterns/Homeward_path for a good overview and examples.

Under the discussion section located at the above link it cites a usability study that found that breadcrumbs are often not used, if at all. This is likely because breadcrumbs are normally more of a supplementary, redundant, navigational aid. However there are areas in Drupal where breadcrumbs become more than just supplemental, such as edit/tab pages.

An example of this is when you're at a page like a forums edit page @ ?q=admin/structure/taxonomy/forums/edit and you want to quickly get back to the taxonomy page @ ?q=admin/structure/taxonomy , or perhaps you're a newbie and you can't remember how you got to taxonomy. Without the breadcrumbs the only way to get to it is to click the stucture link, then taxonomy on the structure page, and if you didn't remember how you got there you might be clicking thru a whole mess of links trying to locate it.

quicksketch does a good job at describing how breadcrumbs should work in Drupal here:
http://drupal.org/node/483614#comment-1834388 and from bangalos: http://drupal.org/node/576290#comment-3081668

And here are some of my findings, with screenshots attached:

Breadcrumb for permissions page (?q=admin/people/permissions) is: Home » Dashboard » People (correct, includes parent hierarchy)

Breadcrumb for a blog entry edit tab (?q=node/11/edit) is: Home » blog entry (incorrect, missing parent hierarchy)

But the breadcrumb for a blog entry page (?q=node/11) is: Home » Blogs » dhthwy's blog (correct, includes parent hierarchy)

The breadcrumb shows correctly for the blog entry @ ?q=node/11 because blog module sets it in hook_view() (however this hook is not invoked for tabs which is why the edit page's breadcrumb trail is wrong). Taxonomy module also sets the breadcrumb (in taxonomy_term_page() menu callback, again doesn't trigger for tabs), as well as the forum module (hook_node_view()).

After seeing these modules set the breadcrumb, it seems like the module is the most qualified to set it, and it doesn't need to do much work whereas the menu system has to work harder to construct a breadcrumb which might not even be correct. So personally I think the menu system should only be used as a last resort - it's already got enough to do.

The patch in this issue (after some adjustments) is only partial fix. It doesn't, for example, fix the breadcrumb on node tabs of type blog, or of type forum, poll, etc. Is there a node-specific hook we can use to set the breadcrumb on tab pages?

Not sure if any of this helps...

jhodgdon’s picture

As another data point, the Search module is having a problem where the page title is changing to "Home" if you do a search. The patch above in #116 fixes this problem.

Attached is a test patch that I would be gratified if you would add to the patch above, which tests this behavior in search.module.

Regarding the complaint in #120 that this is only a partial fix: It's still critical to fix at least this part...

chx’s picture

Status: Needs work » Needs review
sun’s picture

My all-sundayer present for you.

To be merged with second to last patch. Contains 1 unrelated but important hunk that please someone move into a separate issue please.

Status: Needs review » Needs work

The last submitted patch, drupal.breadcrumb-test.123.patch, failed testing.

berenddeboer’s picture

When you list the terms of a vocabulary, the breadcrumb disappears. Is this patch supposed to fix that? Because it does not (tested against alpha 6).

Can someone enlighten me if this is part of this patch or an unrelated issue?

klonos’s picture

I think the last patch in #123 simply adds tests for the patch in #116. As Daniel says in #123:

To be merged with second to last patch.
sun’s picture

Status: Needs work » Needs review
FileSize
11.15 KB

Some of those failures and of course the exceptions were not intended. Fixed in attached patch.

I additionally went through this issue again and double-checked the expectations of this new test case.

Also moved the new test case into menu.test of Menu module. Breadcrumbs are based on menu links, which are currently mixed into router items of the menu system, but I think it's safe to say that menu links, if decoupled, would rather be a concept of Menu module, and thus, even more so, breadcrumbs belong to Menu module (or a dedicated Breadcrumb module), too.

sun’s picture

So let's see whee we are.

jhodgdon’s picture

Did the test from #121 (or its equivalent) get added to the patch? I don't see it there, but maybe I'm missing something.

sun’s picture

FileSize
44.74 KB

Minor test simplification. Quite a lot of test failures remain though:

Most are caused by the expectation that, on a local task (not the default local task), the breadcrumb should also contain the parent link. At least that was the case in earlier versions of this patch and some people chimed in with positive comments on that. For instance, this basically means:

Path            Breadcrumb
node/123        Home » Node's parent link (if any)
node/123/edit   Home » Node's parent link (if any) » Node's (link) title

Since Drupal's tabs/tasks/actions easily allow the user to lose orientation, although still being within the same context (especially on the new Field UI sub-sub-tasks), including the last parent context is a tremendous help for orientation and navigation.

sun’s picture

FileSize
47.06 KB

Merged in #121.

klonos’s picture

One thing though Daniel. Perhaps it is on your mind already, but I thought I'd say it anyways... Some people actually use the breadcrumbs in their sites as a 'You are here' thing (as in maps). This means that they like the actual current node/page/location to be the last 'trail' of the breadcrumb (not as a link though). So it should (optionally) be:

Path            Breadcrumb
node/123        Home » Node's parent link (if any) » Node's title
node/123/edit   Home » Node's parent link (if any) » Node's (link) title » Edit (not a link)

Same thing when people use Pathauto/Sub-path URL Aliases/Token to add the content type in the url...

Path            'pretty' URL                   Breadcrumb
node/123        content-type/node-title        Home » Content type name (link) » Node's title (not a link)
node/123/edit   content-type/node-title/edit   Home » Content type name (link) » Node's title (link) » Edit (not a link)
sun’s picture

@klonos: The current page is not contained in the breadcrumb. That's an entirely different issue, which can still be done through a contributed module. Unlikely to change for D7. For D8, see #481564: Provide breadcrumbs as seperate module with block. (similar to path, menu and search.)

Status: Needs review » Needs work

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

klonos’s picture

Thanx for pointing the issue Daniel ;)

sun’s picture

Status: Needs work » Needs review
FileSize
52.32 KB

Primary problem space being that

1) Regular menu links are not dynamic.

2) Untranslated dynamic router items and links (containing % placeholders) are not included in the breadcrumb. (because the href would contain "%" instead of real values)

3) The menu system provides ARGUMENT_to_arg() hooks to perform replacements for menu links (contrary to full argument loading for router items).

4) However, when those to_arg hooks are called, we only get a "%" passed to the hook and therefore cannot replace. That is, because we do not have a global context aside from $_GET['q'].

5) $_GET['q'] works, but why do we have to implement tons of to_arg hooks that are all doing the same?

Hence, this patch changes _menu_link_translate() to make it automatically try to derive the path argument map from the current router item, if a link happens to share a partial path with the current path.

Furthermore, this patch removes MENU_VISIBLE_IN_BREADCRUMB from MENU_CALLBACKs. Effectively making that type 0 (zero), i.e., not appearing anywhere. I have no idea why callbacks were ever considered to show up in the breadcrumb, but it's against our expectations. But there's also a technical reason: Without removal of that constant, the breadcrumb would contain "Content" when visiting node/% or node/%/edit, since "Content" is the title of the normally hidden menu callback path 'node'. If someone wants a menu callback to appear in the breadcrumb, then it can be registered with bitwise or'ed constants, but by default, this makes no sense.

Speaking of, the breadcrumbs on node/% paths, with or without menu links, are the only breadcrumbs that are still failing currently.

Unfortunately, this took the whole day to figure out, so I can only hope that chx/pwolanin won't start to yell. ;)

Status: Needs review » Needs work

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

dhthwy’s picture

Status: Needs work » Needs review

You can use menu_get_object() in the to_arg hooks and return the nid. From my testing, only nodes appear to need a to_arg hook. But I don't believe this solves the breadcrumb problem for blog/poll/forum/etc type nodes.

dhthwy’s picture

Status: Needs review » Needs work
sun’s picture

Status: Needs work » Needs review
FileSize
52.57 KB

Speaking of, the breadcrumbs on node/% paths, with or without menu links, are the only breadcrumbs that are still failing currently.

wow, erm, a quick peek at node_menu() would have shown that node/% is defined as MENU_CALLBACK :P ;)

sun’s picture

FileSize
52.56 KB

Re-rolled against HEAD.

Status: Needs review » Needs work

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

sun’s picture

Status: Needs work » Needs review
FileSize
54.2 KB

Tricky last bits.

At least one of the test failures (last one) is a currently untested + broken menu module/system functionality - when moving a parent link (from within node form), then child links are not following the parent link, ending up in nirvana. That's equally quite critical, needs to be fixed separately and the tests here will fail until the bug is fixed.

sun’s picture

Issue tags: -Needs tests
FileSize
54.2 KB

Re-rolled against HEAD.

Status: Needs review » Needs work

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

sun’s picture

Status: Needs work » Needs review
FileSize
54.25 KB

Now, the new breadcrumb test completely passes -- except that last assertion I already mentioned, caused by a unrelated menu module/system bug (details in #143).

sun’s picture

Resolving some of the other test failures. (merely removing bogus MENU_CALLBACK assignments throughout core)

sun’s picture

FileSize
61.24 KB

ok, the remaining are very hard to fix for me, because I get a too many connections error for MenuTestCase, and I'm not able to manually replicate the missing title on the AccessDenied (403) page.

Status: Needs review » Needs work

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

quicksketch’s picture

I'm still cheering for you sun! I can't say how much I appreciate your perseverance on this issue. Breadcrumbs have been a pet-peeve of mine for a looong time. I'll see if I can replicate/solve any of these problems with the failing tests and we can try to get it ready for the CPH code sprint.

sun’s picture

Status: Needs work » Needs review
FileSize
62.87 KB

MenuTestCase should be resolved, which leaves us with AccessDeniedTestCase (can't replicate), ForumTestCase, and StatisticsBlockVisitorsTestCase.

And of course, there's still that other menu system bug. Could not find any existing issue, so created #886620: Child menu links are lost in nirvana after moving parent link

Status: Needs review » Needs work

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

sun’s picture

Yay! 5 fails left. So who's able to replicate those?

jhodgdon’s picture

All four of the tests that failed in the test bot in #151 failed for me on my test bot... I will see what's happening and report back soon.

But I also thought it would be useful to give this some human testing. In the notes below, breadcrumb lists are shown in [], individual breadcrumbs are separated by |, and breadcrumbs are given by either just the link title (if it's obvious) or title (path).

GOOD
- If I have added a node to the Main Menu, then the breadcrumb comes from there.
- When editing a node, the breadcrumb is [(breadcrumb when viewing the node) | Title of Node], which seems right.
- Top level items in Admin all have breadcrumb [Home | Administration], and sub-items work fine.
- One of the areas that was bad before this patch was that if you were editing a content type, the breadcrumbs were lacking. They are good now. For instance, if you are on a Manage Fields tab for a content type, the breadcrumb is [Home | Administer | Structure | Content types | (name of content type)], and all the links work fine.
- When I enabled some new modules and got the "you need to enable these modules too" screen, the breadcrumb was [Home | Administer | Modules | List], which actually seems sensible.
- Add new vocabulary - breadcrumb is [Home | Administer | Structure | Taxonomy], and when I edit the terms, add terms, etc. the breadcrumb is this plus the name of the vocabulary (leading to the list terms page). Editing a term pops you out of the admin theme and the breadcrumb is just [Home | term name] which feels odd but is consistent with what happens with add/edit content. (I don't have "stay in admin theme when editing" turned on).
- Forum breadcrumbs seem to be correct [Home | Forum | forum name]
- Blog breadcrumbs seem to be correct [Home | Blogs | username's blog]

BAD
- When I visit a node page like node/1 that has not been added to the menu, the default breadrumb is [Home | Content (node)]. Without this patch it is just [Home]. I think the old behavior is more sensible, especially as on this site, Home is the same as Content. Not sure how to fix this, since by the logic of this path, node/% should be a sub-page of node. But actually this node is not even "promoted to front page" so it wouldn't be displayed on path node, and anyway if path node is equivalent to home, why should it be in the breadcrumb twice? Hmmm... Sounds like a special case...

jhodgdon’s picture

I took a look at the test failures.

a) 403 - "Site information" found
The test is trying to do:

    // Check that we're still on the same page.
    $this->assertText(t('Site information'));

It appears that during the test, the page title is wrong for the site information page. However, when I visit the page myself outside simpletest it looks fine and the page title is correct. See screen shot from the debug output page in the test.... This needs more investigation as to why this is happening in the test. Also, that's not a very great way to test that we are on the right page -- it should test the path instead?

b) Forum tests
These two are the same problem as (a) - the page title is "Home" instead of "Edit Container" or "Edit forum". But when I visit the URLs interactively, the page titles are correct.

c) Breadcrumbs
In this test, it looks like it's adding a node and putting it into the Navigation menu, and then verifying that the breadcrumb comes from the Navigation menu path. Instead, the breadcrumb shown on the screen in the test is just "Home", when you look at the debug output. Not sure why, and I haven't tested interactively to see what's happening.

d) Statistics
This is the same problem as (a) again. The page title is "Home" instead of "IP Address Blocking", as it is when viewed interactively outside simpletest.

Is it possible that all of this is caused by some caching issues within Simpletest?

sun’s picture

Thanks for reviewing, jhodgdon!

When I visit a node page like node/1 that has not been added to the menu, the default breadrumb is [Home | Content (node)].

No, "Content" does not appear. This is even covered by the new breadcrumb tests, and was one of the trickier challenges. ;)

You likely see "Content", because you applied the patch to an existing site. Either re-install with the patch already applied, or install admin_devel module in latest admin_menu-7.x-3.x to "Rebuild system links" from admin_menu's settings page. You're experiencing a different menu system bug that seems to be very hard to fix and also hard to cleanly replicate; more specifically, we're talking about #550254: Menu links are sometimes not properly re-parented (again).

EDIT: Given that you also cannot replicate the test failures (except the known one mentioned above), I'll try to re-install from scratch having the patch already applied, to see whether that makes a difference. (I only rebuilt system links via admin_devel thus far)

jhodgdon’s picture

Ah. Yes I applied the patch to an existing site.

jhodgdon’s picture

Oh, and regarding #155C, I did just verify that on my site interactively, if I add nodes to the Navigation menu in a hierarchy, the breadcrumbs work as they should.

jhodgdon’s picture

I just reinstalled from scratch with the patch already applied.

I can confirm that the site information page has the right title, forum editing pages have right title, ip address blocking has the right title, and nodes don't have "Content" in the breadcrumb by default. When viewed interactively.

jhodgdon’s picture

Actually I think that most tests are showing wrong page titles if I look at the Verbose output link after running a test. It's just those 4 spots where someone is actually testing that the page title is as expected.

sun’s picture

Odd. The only permanent difference I know with regard to PIFR is that it doesn't use clean URLs. However, that shouldn't be relevant here. Breadcrumbs are solely based on menu links.

Does anyone have any clues?

jhodgdon’s picture

I am doing some debugging in the test env, but it is taking a while... Will report back in a couple of hours.

jhodgdon’s picture

So I am debugging using the IP blocking page that is failing in the statistics page.

So far, I have figured out that in menu_set_active_trail(), when running within the simpletest, it is getting the right thing for menu_link_get_preferred() -- i.e. it returns the management menu link as it should.

But then when that function calls menu_tree_page_data('management') it is getting back an empty array for the menu tree, which is why the active trail is not being generated, and hence the page title and breadcrumb trail are missing.

I guess it is a static variable or cache problem? I am continuing to debug.

jhodgdon’s picture

OK, I give up.

It's not caching or static variable issues. menu_tree_data() or _menu_tree_data() is getting corrupted somehow, I think? In any case when I put in debugging statements it looks like it is working fine, but then if you look at _menu_build_tree(), nothing is getting successfully returned from that function.

I have no idea why, and of course it is only happening while you are running simpletest, making debugging nearly impossible.

But I'm pretty sure that is the problem. If you look in _menu_build_tree at the result from _menu_tree_data(), you will find it's empty for the test case that's failing, in spite of having multiple links, and it going through the right steps in the recursion in the _menu_tree_data() calls.

Maybe too many recursion steps? I'm not sure, but it doesn't seem to be working.

I'm also not sure I understand the logic there. menu_tree_data() is building a nested array, but it's organized only by depth, not by which link is the parent of which other link... ??? well, whatever. In simpletest it is not working correctly, and it's odd, and I don't know why.

jhodgdon’s picture

I have real PHP debugging working now (hooray!) and am revving up the debugger to see if I can figure this out.

jhodgdon’s picture

Sigh, spoke too soon. When running the debugger, apparently you don't get the cURL-generated Drupal requests in the debugger, so you only see the calls from SimpleTest, not what's happening inside the simpletest browser. Oh well.

webchick’s picture

Hm. You should be able to put a breakpoint anywhere you want? I guess the one thing to watch out for, if you're using something like Komodo, is the batch API can make debugging pretty confusing. I think I get something like 2-3 "Would you like to run the debugger?" prompts before you get to the "true" test run from SimpleTests.

jhodgdon’s picture

I'm debugging in Eclipse...

I put a breakpoint in the statistics test just before the place where it fails. Then I turn on some more breakpoints. It manages to stop at drupalGet(), but then it just skips right over the curl() call, and I don't get asked if I want to start debugging. That I could handle. It's just not trying to connect to the other apache/php session that gets generated by the curl() call.

webchick’s picture

Oh, I see. Yeah, the cURL call won't trigger a new debugging request, because as far as the debugger is concerned, it's just a PHP function.

I wonder though... if you added ?XDEBUG_SESSION_START=1 to the drupalGet() call if that would do the trick? Hm....

jhodgdon’s picture

http://drupal.org/node/30011 says as much. I'm trying it out.

sun’s picture

Status: Needs work » Needs review
FileSize
62.84 KB

Re-rolled against HEAD. (Home » Dashboard is Home » Administer now)

sun’s picture

FileSize
66.25 KB

ok, it's relatively simple:

If you don't have access to "Administer", you cannot expect a breadcrumb containing "Administer". And due to how the menu system works in general and how menu trees are built in general, and since the breadcrumb actually is just a different representation of a regular menu tree, the breadcrumb cannot recurse any further than "Home", if the user does not have the "access administration pages" permission.

In turn, because that same functionality of the menu system uses the title of the last available link in the breadcrumb as page title, if no other has been set manually, the page title is also "Home".

Effectively, the previous/current breadcrumb behavior was able to recurse into the menu tree - even if the user does not have access - to grab the link for the current page and use that as (invisible) last breadcrumb item, which is then taken over for the page title.

I've to study why and where exactly that special logic happened, and whether it would be possible to incorporate it - or some better alternative - into this patch.

jhodgdon’s picture

Aha! That makes sense as to why the page titles would now be "Home".

And I now have simpletest debugging working (!!), though I think it's not necessary now for this issue. Will be useful for the next one...

Status: Needs review » Needs work

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

sun’s picture

Status: Needs work » Needs review
FileSize
66.77 KB

Just to prove that we're down to 1 now.

While I'm studying this - anyone able to move forward #408482: Menu links do not follow parent when moving, which blocks this patch? (that last test failure will only vanish when that bug is fixed)

Status: Needs review » Needs work

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

moshe weitzman’s picture

If you set up your Xdebug to autostart using xdebug.remote_autostart = 1 then you get a new debug window for each of those curl requests. Its a pain to continue through them, but it can be done. I have done this on Komodo. Not sure if other xdebug clients can spawn new windows like this.

jhodgdon’s picture

moshe: I finally got the debugging working, through the help of ksenzee on IRC & Google & trial/error. Eclipse can do the thing you're describing also, if you set the preferences properly.

So this week I set up Drupal 6/7 on Postgres, Drupal 7 on SQLite, got my two contrib modules with tests to pass all the tests on all of those combinations (plus or minus a few core patches that I was also able to supply/test for Postgres compatibility), and got debugging working, even with SimpleTest. Phew! Now I should really be able to contribute to Drupal. :)

dhthwy’s picture

this issue turned a 180 thanks to sun. you guys are doing an awesome job with this. sorry I couldn't have been more help.

dhthwy’s picture

180 degrees that is ;)

quicksketch’s picture

I've got this down to 1 failing test. The last remaining one hopefully shouldn't be too bad.

chx’s picture

Assigned: sun » chx

I will fix this today.

quicksketch’s picture

Assigned: chx » sun
Status: Needs work » Needs review
Issue tags: -API change, -beta blocker

This should have 1 failing test. We just needed to update one of the tests to logout after logging in to pass the 403 access denied test.

quicksketch’s picture

Assigned: sun » Unassigned
FileSize
67.14 KB
quicksketch’s picture

Assigned: Unassigned » quicksketch
Issue tags: +API change, +beta blocker

Sorry having some network issues at the Drupalcon code sprint today. Retagging.

Status: Needs review » Needs work

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

aspilicious’s picture

I rly want this to get it in, so for people didn't read the entire issue: #408482: Menu links do not follow parent when moving will remove the latest fail.
So support chx in fixing that one, that way we can close 2 criticals at once ;)

quicksketch’s picture

Status: Needs work » Needs review
FileSize
0 bytes

There's some weird issue with a new static variable "menu_link_get_preferred". I'm removing it as a static here to see if it will pass tests, but if it does we won't be able to use this patch because of the inefficiencies it introduces by removing the static variable.

quicksketch’s picture

FileSize
67.48 KB

Arg, more network problems. Here's a patch that actually contains something to test.

Status: Needs review » Needs work

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

quicksketch’s picture

FileSize
76.75 KB

I applied #408482: Menu links do not follow parent when moving and this patch at the same time and all the tests pass!! So indeed it does seem that this issue is dependent on #408482. Once that issue is committed, this patch should pass all tests.

aspilicious’s picture

And that one is in, so testbot should give green light ;)

aspilicious’s picture

Status: Needs work » Needs review
pwolanin’s picture

Status: Needs review » Needs work

can someone explain or fix this?

+        // @todo die() is never reached, obsolete?.
+        if (!$candidate_item['access']) {
+          die();
+          continue;
+        }

quicksketch’s picture

Status: Needs work » Needs review
Issue tags: -API change, -beta blocker

Reroll, should pass everything now. This patch still needs cleanup, including pwolanin's comment in #194. I don't understand what that line is doing either.

quicksketch’s picture

Issue tags: +API change, +beta blocker
FileSize
67.39 KB

Reroll, should pass everything now. This patch still needs cleanup, including pwolanin's comment in #194. I don't understand what that line is doing either.

pwolanin’s picture

Status: Needs review » Needs work

This makes no sense to me - why are we doing:

-    'type' => MENU_CALLBACK,

all over the place.

makara’s picture

Note that the patch also removes MENU_CALLBACK from breadcrumbs (with define('MENU_CALLBACK', 0);).

menu_link_get_preferred() doesn't need to check the access IMO and truth is it is always called after an access check.

pwolanin’s picture

FileSize
51.46 KB

I've removed a lot of the changes that should not be in the patch.

sun’s picture

@pwolanin: Please don't simply remove code that you do not understand. You removed important parts of this patch.

MENU_CALLBACKs are currently defined to appear in the breadcrumb, but that's horribly wrong. MENU_CALLBACKs are straight callbacks with nothing else. Defining a path that *should appear* in a hierarchy as MENU_CALLBACK is plain wrong.

@quicksketch: While I love that we made tests pass, we still have to add new tests for the (un)expected behavior that made this patch not pass tests earlier, i.e., the expected behavior for breadcrumbs, when the current page cannot have a breadcrumb, because there is no menu tree/trail to the current page, since the current user does not have permissions to access pages higher up in the trail.

sun’s picture

Assigned: quicksketch » sun
Status: Needs work » Needs review
FileSize
68.32 KB

Back to #196. Added tests for the additionally (currently) expected behavior, which should fail as of now.

sun’s picture

FileSize
68.33 KB

- Fixed phpDoc for MENU_CALLBACK.

- Removed that obsolete code path mentioned in #194.

sun’s picture

FileSize
69.72 KB

MEH! Who introduced assertResponse() but no corresponding assertNoResponse() ?!

sun’s picture

    // @todo Do we really expect no breadcrumb on 403? If there are menus, why
    //   not a breadcrumb pointing to Home?
sun’s picture

FileSize
69.87 KB

What a difference a single parenthesis can make. Bit masks complicate things quite badly.

sun’s picture

FileSize
71.46 KB

Alright, we fixed all currently asserted expectations. However, we should resolve all @todos contained in the tests, as every single one may reveal unexpected behaviors and require larger changes. Attached patch already seems to prove that -- breadcrumbs seem to fail for taxonomy terms having menu links.

sun’s picture

+++ includes/menu.inc	22 Aug 2010 18:06:42 -0000
@@ -799,13 +813,34 @@ function _menu_link_translate(&$item) {
+    // Complete the path of the menu link with elements from the current path,
+    // if it contains dynamic placeholders (%).
     $map = explode('/', $item['link_path']);
+    if (strpos($item['link_path'], '%') !== FALSE) {
+      // Invoke registered to_arg callbacks.
+      if (!empty($item['to_arg_functions'])) {
+        _menu_link_map_translate($map, $item['to_arg_functions']);
+      }
+      // Or try to derive the path argument map from the current router item,
+      // if this $item's path is within the router item's path. This means that
+      // if we are on the current path 'foo/%/bar/%/baz', then menu_get_item()
+      // will have translated the menu router item for the current path, and we
+      // can take over the argument map for a link like 'foo/%/bar'. This
+      // inheritance is primarily used for breadcrumb links.
+      elseif ($current_path = menu_get_item()) {
+        if (strpos($current_path['path'], $item['link_path']) === 0) {
+          $count = count($map);
+          $map = array_slice($current_path['original_map'], 0, $count);
+          $item['original_map'] = $map;
+          if (isset($current_path['map'])) {
+            $item['map'] = array_slice($current_path['map'], 0, $count);
+          }
+        }
+      }
     }
     $item['href'] = implode('/', $map);

It seems we just revealed yet another menu system bug:

As of now, Drupal core does not implement any to_arg callbacks to translate dynamic arguments for menu links. As explained in the phpDoc quoted above, this patch introduced an automatic population of dynamic arguments based on the current menu router item.

This change was one part of the puzzle to solve the problem of not being able to translate higher/parent breadcrumb links on deep links, such as admin/structure/types/%/fields/%/edit -- i.e., the topic of this very issue, no breadcrumbs on dynamic paths.

It works. But, as mentioned before, breadcrumbs are just a different representation of menu link trees, we also get the behavior in those now, which means:

1) Add a taxonomy term. It gets tid 1.

2) Add a menu link for that taxonomy term, i.e., to taxonomy/term/1.

3) Visit taxonomy/term/1.

Expected result: Proper breadcrumbs, proper page title, and whatnot.

Actual result: == Expected result.

However: When being on the page taxonomy/term/1, the menu router item contains a valid argument map for taxonomy/term/%. Taxonomy module itself registered the generic dynamic path taxonomy/term/%, and since we actually have a menu router item sharing exactly that path, we are now getting a "new" menu link that *does not exist*.

This actually means two things at once:

1) Menu link tree rendering code elsewhere does not properly check for the internal menu system constant MENU_VISIBLE_IN_TREE. Only links set to be visible in a tree should be visible in a menu link tree. (Do I even have to spell that out? :P)

2) Regardless of 1), we likely want to move that dynamic to_arg translation based on the current router item in a function that only affects trails/breadcrumbs, though I'm not sure which one that could be.

Powered by Dreditor.

Status: Needs review » Needs work

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

sun’s picture

Status: Needs work » Needs review
FileSize
71.88 KB

Added assertions for the bug described in #207 to make sure we don't introduce this bug in whatever we do. This is likely my last re-roll for tonight.

pwolanin’s picture

Status: Needs review » Needs work

@sun - the parts I removed did not seem to make sense - for example, making callbacks into MENU_NORMAL_ITEM. I don't understand you argument that callbacks should not be in the breadcrumb - it seems the whole point of this issue is to make them visible.

Also, there were code formatting changes in files that were otherwise not touched - those should not be in this patch (e.g. statistics.test).

Also, there seemed to be bad bitwise tests and other problems.

sun’s picture

@pwolanin:

i) Yes, that's the whole point, which I already tried to explain in #200, and which has been even more clarified in the latest patch(es): MENU_CALLBACKs are not supposed to appear and participate anywhere. Not in menu trees, not in the breadcrumb, and they also do not need a record in {menu_links}. However, without this patch, they do participate in breadcrumbs and also have menu link records. Just because a path contains dynamic placeholder argumentes (%) doesn't make it a menu callback. Type MENU_CALLBACK is removed from all router items that are not menu callbacks, but simply paths that happen to have dynamic placeholder arguments in them. They want and should appear in breadcrumbs and everywhere else (whereas there is no technical core capability yet that's able to render a menu tree containing dynamic placeholders -- let me quickly add that there is one in contrib). So by reverting those changes you're effectively reverting the intention of this issue/patch: We need to differ between entirely invisible menu callbacks and paths that happen to contain dynamic placeholder arguments. They are not the same. Likewise, the definition of MENU_CALLBACK is wrong, because it defines them to be visible in breadcrumbs.

ii) You're mistaken. It's not only code formatting.

iii) Could you elaborate a bit more on this? I'm unable to see bad bitwise tests in this patch. Before removing any changes that may or may not be bogus, I'd like to hear valid concerns.

pwolanin’s picture

@sun - can you give me an example of a MENU_CALLBACK that should *not* appear in the breadcrumb?

quicksketch’s picture

I think sun's comments make sense here. Currently everywhere we have a dynamic menu path (say node/%/delete), we are currently using MENU_CALLBACK because it's not visible in the menu anywhere. With this patch, we now have a new way of using MENU_NORMAL_ITEM. Essentially you can break it down into this:

MENU_CALLBACK: No breadcrumb, no entry into the menu hierarchy, not displayed in menus.
MENU_LOCAL_TASK: Has breadcrumb, entry in the menu hierarchy, not displayed in menus (displayed instead as tabs).
MENU_NORMAL_ITEM: Has breadcrumb, entry in the menu hierarchy, displayed in menus if possible i.e does not have a dynamic path.

So node/%/delete used to be a MENU_CALLBACK, now it's a MENU_NORMAL_ITEM because we want it to support the breadcrumb. It's not displayed in the menu to end-users because it contains a dynamic path and it would be impractical to do so (however like sun notes, it may be possible to add this support through a contrib module).

This paradigm change explains why callbacks are now define('MENU_CALLBACK', 0), because they have no entry anywhere in the menu tree and have no support for breadcrumbs. Where we previously were using MENU_CALLBACK for dynamic paths, we'll now be using either MENU_CALLBACK or MENU_NORMAL_ITEM, depending on if we want Drupal to assume a hierarchical menu structure or not.

joachim’s picture

Issue tags: +Needs documentation

> With this patch, we now have a new way of using MENU_NORMAL_ITEM.

That'll need documenting ;)

pwolanin’s picture

I don't think that makes any sense. give me an example of a MENU_CALLBACK that should *not* appear in the breadcrumb.

sun’s picture

FileSize
7.31 KB

@pwolanin: Sure. Plenty, even. See full list of (actually correct) menu callbacks throughout core attached.

quicksketch's explanation in #213 pretty much cuts it. As of now, it seems like we simply added 'type' => MENU_CALLBACK to any router item that has a % in it, regardless of whether that makes sense at all or not. A MENU_CALLBACK is a menu router item only, nothing else. You can have a look at how admin_menu 7.x-3.x builds and renders dynamic link trees.

quicksketch’s picture

I think it makes sense that we use MENU_NORMAL_ITEM for most items in the menu. Why would things like "admin/structure/types/manage" be a MENU_NORMAL_ITEM while "admin/structure/types/manage/article/fields/%/fields" be a MENU_CALLBACK? It's like sun says, we're making things MENU_CALLBACK for no reason other than it contains a wildcard.

sun’s picture

Status: Needs work » Needs review
Issue tags: -Needs documentation
FileSize
76.87 KB

Breadcrumbs are based on menu trees, but menu trees are built through lots of helper functions recursing individually into the tree, passing only minimal arguments around. Thus, limiting translation of dynamic path arguments in links to a certain invocation only is quite a challenge.

Most likely I should have added $translate as $link['translate'] property starting from one of the tree building helpers, similar to the already existing $link['options']['alter'] property, but I realized that only now, and I'm slowly running out of steam for today.

Status: Needs review » Needs work

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

sun’s picture

Status: Needs work » Needs review
FileSize
80.92 KB

More tests, more asserted expectations.

pwolanin’s picture

conceptually, I think of a callback as belonging in the breadcrumb, but NOT in the visible tree - I still don't agree with this change:

-define('MENU_CALLBACK', MENU_VISIBLE_IN_BREADCRUMB);
+define('MENU_CALLBACK', 0);

If we want a router type that's totally decouple from the menu tree we should define something new.

quicksketch’s picture

@pwolanin: How would you justify #217? Where we decide to use a different menu type explicitly because it contains a wildcard? At this point I'd be happy moving forward with sun's excellent patch and not get into a debate over naming conventions (creating a new menu item type or using just using MENU_NORMAL_ITEM). Functionality-wise it's not going to make any difference what we call the menu item type and I think that reusing the existing one rather than making up a new one is going to be more acceptable at this point.

sun’s picture

conceptually, I think of a callback as belonging in the breadcrumb, but NOT in the visible tree

So what is the concept behind menu callbacks? As far as I understand its usage throughout Drupal core and contributed modules, menu callbacks are and should be used as plain router callbacks, without any other further logic attached, which is also what the phpDoc for MENU_CALLBACK exactly states:

+++ includes/menu.inc	23 Aug 2010 19:36:51 -0000
  * Menu type -- A hidden, internal callback, typically used for API calls.
  *
  * Callbacks simply register a path so that the correct function is fired
- * when the URL is accessed. They are not shown in the menu.
+ * when the URL is accessed. They do not appear in menus or breadcrumbs.
  */
-define('MENU_CALLBACK', MENU_VISIBLE_IN_BREADCRUMB);
+define('MENU_CALLBACK', 0);

A hidden, internal callback, typically used for API calls. That's it.

I cannot see any other legit usage or concept for menu callbacks. It's entirely not clear to me, why we think or thought that they perhaps should show up in breadcrumbs, given their callback purpose. The definition as MENU_VISIBLE_IN_BREADCRUMB becomes even more weird in combination with its usage for dynamic router paths, which were technically not able to appear in breadcrumbs at all. Thus, the one and only logic attached to this router item type was technically impossible for the majority of router paths it was used for. That sounds like a conceptual design and usage failure to me, which should be corrected, and which is corrected in this patch.

As far as I can evaluate this, it just seems like another unintended and hard-coded presumption in the menu system, which potentially was never fully reviewed and analyzed, similar to other menu system constants we had to finally fix for Drupal 7: #631550: Stale + improper logic for MENU_VISIBLE_IN_TREE and MENU_VISIBLE_IN_BREADCRUMB

drifter’s picture

So, are there any side-effects to making paths containing wildcards a MENU_NORMAL_ITEM instead of a MENU_CALLBACK? I.E. is the following true:

path without wildcard + MENU_NORMAL_ITEM = MENU_VISIBLE_IN_TREE | MENU_VISIBLE_IN_BREADCRUMB
path with wildcard + MENU_NORMAL_ITEM effectively = MENU_VISIBLE_IN_BREADCRUMB

(If I'm understanding the problem correctly)

If there are no side effects, then this is a welcome change. MENU_CALLBACK always meant "just an API callback" for me, with no presence in menus, breadcrumbs etc.

sun’s picture

re #224:
Not exactly. Removing MENU_CALLBACK means that a router item becomes MENU_NORMAL_ITEM, which is MENU_VISIBLE_IN_TREE | MENU_VISIBLE_IN_BREADCRUMB, and technically makes the items show up in menu link trees and breadcrumbs. In theory, regardless of whether the router item's path contains dynamic placeholder arguments.

In practice, however, as of now, Drupal core's menu system does not support menu links using dynamic placeholder arguments in menu trees. But that's an entirely different shortcoming, which may be fixed in D8 or beyond.

What matters is that the router items being changed in this patch are actually intended to be in a visible tree hierarchy. We want to see the hierarchy in breadcrumbs. And if we could, we'd also want to see them in menu trees (but we cannot). In the end, the conceptual issue is as simple as mentioned before: Just because those paths contain dynamic arguments doesn't automatically make them menu callbacks.

Additionally, by defining MENU_CALLBACK as real, internal, and hidden menu router callbacks, we get a tiny performance gain, as those router items are no longer needlessly stored and maintained in {menu_links}.

quicksketch’s picture

Small review:

     // Make sure the current page is in the trail (needed for the page title),
     // but exclude tabs and the front page.
+    if ($trail[$last]['href'] != $preferred_link['href'] && ($preferred_link['type'] & MENU_VISIBLE_IN_BREADCRUMB) == MENU_VISIBLE_IN_BREADCRUMB && !drupal_is_front_page()) {
+      $trail[] = $preferred_link;
     }

This really took me quite a while to figure out. ($preferred_link['type'] & MENU_VISIBLE_IN_BREADCRUMB) == MENU_VISIBLE_IN_BREADCRUMB essentially says "if the link type needs to be shown in the breadcrumb". The code comment above could say as much instead of the bit about excluding tabs.

-    $this->drupalGet('test-entity/' . $entity->ftid .'/edit');
+    $this->drupalGet('test-entity/manage/' . $entity->ftid .'/edit');

Completely minor, but while doing these changes we should fix the concatenation of .'/edit'.

+  form_state_values_clean($form_state);
   $variables = $form_state['values'];
 
   // Remove everything that's been saved already - whatever's left is assumed
@@ -333,8 +334,6 @@ function node_type_form_submit($form, &$
     }
   }
 
-  unset($variables['form_token'], $variables['op'], $variables['submit'], $variables['delete'], $variables['reset'], $variables['form_id'], $variables['form_build_id']);
-

This is definitely a good change, but probably not meant to be included in this issue.

sun’s picture

FileSize
81.08 KB

Incorporated #226.

pwolanin’s picture

This looks incorrect:

 ($preferred_link['type'] & MENU_VISIBLE_IN_BREADCRUMB) == MENU_VISIBLE_IN_BREADCRUMB && !drupal_is_front_page()) {

if you are doing a bitwise operation, why do you compare to the original? Ah - maybe it's because you define MENU_CALLBACK as 0?

Either way, this is not good. MENU_CALLBACK shoudl not be zero, and as above, I don't really feel that I'd want to sign off on this with the current redefinition of MENU_CALLBACK.

sun’s picture

@pwolanin: Please understand that we need to know *why* MENU_CALLBACK should not be zero, or respectively, *why* menu callbacks should appear in breadcrumbs. Could you explain that, if possible? No one else seems to be able to understand the current definition. Instead, most seem agree with the reasoning provided to correct the definition. So to actually have a discussion, there are have to be real counter-arguments. Merely stating "this is not good" does not allow us to move forward.

quicksketch’s picture

Like I said I'm not super sharp on bit-wise math, but I think this:

if (($preferred_link['type'] & MENU_VISIBLE_IN_BREADCRUMB) == MENU_VISIBLE_IN_BREADCRUMB)

Should simply be this:

if ($preferred_link['type'] & MENU_VISIBLE_IN_BREADCRUMB)
Alan D.’s picture

#228 I think that pwolanin may be referring to that having MENU_CALLBACK = 0 means that an unassigned item results in an assigned behavior (whatever that is now in the menu). So the check !($item & MENU_CALLBACK) would lead to a possible false match, (if that matters)

#230 - 00100 & 11111 should equal 00100 from memory

Great work guys. Loving the results :)

quicksketch’s picture

#230 - 00100 & 11111 should equal 00100 from memory

Yep that's right, so basically if you're just trying to check if a menu type should be shown in a breadcrumb, you just use ($preferred_link['type'] & MENU_VISIBLE_IN_BREADCRUMB), since if the breadcrumb bit is not 1, then that conditional would return 0. If it does support breadcrumbs it would be 00100, or just assume that the entire operation is TRUE, so the == MENU_VISIBLE_IN_BREADCRUMB is not necessary.

pwolanin’s picture

Status: Needs review » Needs work

I thought quicksketch was going to comment regarding the conversation we had.

There are pages - like the node delete page - which should have a working breadcrumb, but would never appear as a menu link. So this patch is wrong because we lose that functionality. This change for MENU_CALLBACK is incorrect.

Let's define a new constant like 'MENU_ENDPOINT' OR 'MENU_SERVICE' or something else (I don't care) for the use case, e.g., of ajax or Service module calls that will never be accessed by the end user and has value 0.

sun’s picture

There are pages - like the node delete page - which should have a working breadcrumb, but would never appear as a menu link.

That's not true. Contextual links are very small, partial menu trees, in which such links may appear. It's actually one of my goals for D7 contrib and D8 core to revamp, simplify, and speed up contextual links by properly building partial link trees.

In the case of node/%/delete, we don't even talk about a MENU_CALLBACK, but a local task: http://api.drupal.org/api/function/node_menu/7

Introducing a new constant that would be what MENU_CALLBACK is already - even stated in its documentation - a simple, straight, and hidden callback without any other attached logic - does not make sense to me. "Menu callback" is exactly to the point.

If there would be a new constant for the remaining edge-cases, then that would entirely duplicate MENU_VISIBLE_IN_BREADCRUMB, but if absolutely required, we'd have to call it MENU_BREADCRUMB or similar.

sun’s picture

Status: Needs work » Needs review

Counter question: Please show us the actual "menu callbacks" in core that ought to have breadcrumbs after removing that constant from router items that just happen to have a dynamic argument in their paths.

Please also take into account that - while core might be unable to do so - contrib is definitely able to render dynamic link trees.

In other words: The provided reasoning is not sufficient. And it's quite different to what the actual documentation of MENU_CALLBACK states.

pwolanin’s picture

This is a potential problem:

     // Generate a cache ID (cid) specific for this page.
-    $cid = 'links:' . $menu_name . ':page-cid:' . $item['href'] . ':' . $GLOBALS['language']->language . ':' . (int) $item['access'] . ':' . (int) $max_depth;
+    $cid = 'links:' . $menu_name . ':page-cid:' . $item['href'] . ':' . $GLOBALS['language']->language . ':' . (int) $item['access'] . ':' . (int) $max_depth . ':' . (int) $translate;

We are now building and caching the links twice per page?

pwolanin’s picture

I think that double caching is not at all needed. I altered the code to avoid it.

This is the diff relative to the last patch:

@@ -1168,17 +1168,19 @@ function menu_tree_page_data($menu_name, $max_depth = NULL, $translate = FALSE)
           }
           $tree_parameters['expanded'] = $parents;
           $tree_parameters['active_trail'] = $active_link;
-          $tree_parameters['translate'] = $translate;
         }
         // Otherwise, only show the top-level menu items when access is denied.
         else {
           $tree_parameters['expanded'] = array(0);
         }
-
         // Cache the tree building parameters using the page-specific cid.
         cache_set($cid, $tree_parameters, 'cache_menu');
       }
-
+      // $tree_parameters['active_trail'] will only be set if we we able to
+      // access the router item. Set 'translate' here to avoid double caching.
+      if (isset($tree_parameters['active_trail'])) {
+        $tree_parameters['translate'] = $translate;
+      }
       // Build the tree using the parameters; the resulting tree will be cached
       // by _menu_build_tree().
       $tree[$cid] = menu_build_tre

Also fixing this mismatch of the function parameters to the doxygen:

-function menu_link_get_preferred($href = NULL) {
+function menu_link_get_preferred($path = NULL) {

Fixing this use of bitwise operator:

@@ -2191,7 +2191,7 @@ function menu_set_active_trail($new_trail = NULL) {
     // if the link's type allows it to be shown in the breadcrumb. Also exclude
     // it if we are on the front page.
     $last = count($trail) - 1;
-    if ($trail[$last]['href'] != $preferred_link['href'] && ($preferred_link['type'] & MENU_VISIBLE_IN_BREADCRUMB) == MENU_VISIBLE_IN_BREADCRUMB &&
+    if ($trail[$last]['href'] != $preferred_link['href'] && ($preferred_link['type'] & MENU_VISIBLE_IN_BREADCRUMB) && !drupal_is_front_page()) {
       $trail[] = $preferred_link;
     }
   }

I'm still not totally convinced about the change to MENU_CALLBACK, but I think these changes get use closer to a polished patch.

Damien Tournoud’s picture

Please always use (value & CONSTANT == CONSTANT). It is only equivalent to value & CONSTANT) if constant is actually a single bit. Having only one way to do that helps for consistency and self-documentation.

I agree with Peter here that menu items defined as MENU_CALLBACK (like node/%node) should by default be visible in breadcrumbs. I don't understand the rationale behind the change.

pwolanin’s picture

@Damien Tournoud - no where else in menu.inc are we using (value & CONSTANT == CONSTANT). If we want to make that consistent, maybe we should do it in another issue. Also - all the menu constants are used in the way that they should be bitwise unique.

quicksketch’s picture

Please show us the actual "menu callbacks" in core that ought to have breadcrumbs after removing that constant from router items that just happen to have a dynamic argument in their paths.

In my conversation with Peter, we recognized that there is no longer any existing type for an item that is:

  1. Not visible in the menu hierarchy
  2. Does have a breadcrumb

So I had thought that somewhere in core we needed this use-case. Specifically I had been thinking of paths like "admin/structure/menu/add", in which we want that combination of properties. As it turned out, I couldn't find *any* callbacks that needed to be in the breadcrumb but that shouldn't be in the menu tree that aren't already accounted for. I had thought that "admin/structure/menu/add" would be a MENU_CALLBACK but it's actually a MENU_LOCAL_ACTION (which has similar properties). Upon practical examination, I could find no place where we need an item to be visible in the breadcrumb but not in the hierarchy that is not an action.

So in short, I'm left again agreeing with sun in that I think this approach is fine and there are very few situations where you want MENU_CALLBACK to be visible in the breadcrumb. In an extreme edge-case if you needed something to be in the breadcrumb, you could just set 'type' => MENU_VISIBLE_IN_BREADCRUMB, though it wouldn't be philosophically pure, it would work to cover that 1% situation.

quicksketch’s picture

I agree with Peter here that menu items defined as MENU_CALLBACK (like node/%node) should by default be visible in breadcrumbs. I don't understand the rationale behind the change.

Paths such as node/%node would no longer be MENU_CALLBACK. The idea here is that MENU_CALLBACK would be used exclusively for situations in which the path has no relationship to menu tree at all (such as AJAX callbacks or REST API callbacks). Paths that should be in the breadcrumb will almost always be MENU_NORMAL_ITEM. Items that don't need the breadcrumb or hierarchy at all we use MENU_CALLBACK, and we get a tiny performance improvement because we don't need to bother building out menu links for these items that don't care about hierarchy anyway.

pwolanin’s picture

This change looks potentially wrong:

           // Use array_values() so that the indices are numeric.
-          $parents = $active_link = array_unique(array_values($active_link));
+          $parents = $active_link = array_values(array_unique($active_link));

why were the function calls reversed?

makara’s picture

#242: It is not wrong since $active_link is changed by the patch. But I don't think we need array_values() anymore cause the indices are already numeric, and the array_unique() is almost useless (though doesn't hurt).

pwolanin’s picture

patch still seems to fail for a path like ?q=admin/structure/menu/manage/management The BC just says "Home" even though this is a link in the tree.

sun’s picture

FileSize
83.05 KB

Fixed

- #244 Menu module router item type definitions. Added tests for that.

- #242 array_*() functions needless.

- #238 bitwise exact comparison of constants. There have been enough issues that clearly clarified that API functions in menu.inc have to compare with internal constants and always test for the exact bits. We don't have to do everything at once, and the most important functions are corrected with this patch and others that already went in).

Remaining issues:

- #236 'translate' property potentially leading to double caching. To really prevent double caching, we need to remove the new $translate argument from menu_tree_page_data() and automatically set 'translate' if 'active_trail' is set and we ask for the trail to the current menu router item's path. That, however, would also try to translate (any potentially existing) dynamic links in a potential menu tree, if it happens to contain a link to the current router item's path.

pwolanin’s picture

Status: Needs review » Needs work

@sun - did you not use my patch as a basis? We aren't going to get this issue finished if you are doing independent re-rolls. We should probably use a common github repo.

Also, items marked as MENU_CALLBACK are still going to get saved to the {menu_links} table.

drunken monkey’s picture

Status: Needs work » Needs review

Hm, you fixed Peter's bug in #244 by making admin/structure/menu/manage/management a MENU_NORMAL_ITEM – but is it really correct that MENU_CALLBACK pages shouldn't show a breadcrumb (other than "Home")? I thought that it should show a correct, normal breadcrumbs on those pages and just exclude it from breadcrumbs displayed for child items. Isn't that the case?
If I change the item's type back to MENU_CALLBACK, admin/structure/menu/manage/management shows just "Home", but admin/structure/menu/manage/management/add shows a breadcrumb even including the link to the parent "Management" item. To me it seems that this is exactly the wrong way round.

makara’s picture

Status: Needs review » Needs work

regarding to the translate thing, I suggest:

1. instead of doing this in menu_link_get_preferred()

+        $map = explode('/', $path);
+        _menu_translate($candidate_item, $map);

we do

+        $candidate_item = array_merge(menu_get_item($path), $candidate_item);

because the function menu_link_get_preferred() is always called after menu_get_item() which also calls _menu_translate().

2. simplify the query inside menu_link_get_preferred() if possible.

3. cache only the query result.

sun’s picture

Status: Needs work » Needs review
FileSize
101.01 KB

@pwolanin: As discussed, last patch is based on yours.

@drunken monkey: No, you got it entirely wrong. I suspect you didn't read any of the follow-ups since ~#136 and also not most recent clarifications by quicksketch and me. A MENU_CALLBACK is an API callback end point without any corresponding menu link or breadcrumb, because such callbacks do not need anything like that, they don't work within a page context like a regular menu link would do.

However, @pwolanin just pointed out that the manually stored menu links for admin/structure/menu/manage/navigation and the like - despite joining on {menu_links}.router_path = {menu_router}.path leading to a type of MENU_CALLBACK - should result in a proper breadcrumb. Although that makes no practical sense (it's illogical to link to an API callback end point and to expect something like a page containing a breadcrumb there in the first place), I guess I'm going to add a test for this, if absolutely required. If possible, however, I'd prefer to fix that total edge-case in a follow-up issue.

re #248 and the 'translate' parameter:

Just discussed this with @pwolanin. We basically want to re-use the already built + cached menu tree, but _menu_tree_check_access() entirely removes/skips links containing dynamic arguments from the cached tree, so we are currently not able to re-use the cached tree and run _menu_link_translate($link, $translate = TRUE) again on all links, as that no longer contains links with dynamic arguments.

One solution would be to make _menu_tree_check_access() to keep dynamic links and have subsequent building and rendering functions account for the possibility of inaccessible links in the tree. That, however, bears the question whether it wouldn't be faster to just build a new tree, containing only the active trail, and only the active trail (i.e., incorporating the patch from #327918-43: Optimize menu_tree_page_data() performance).

Attached patch fixes all MENU_CALLBACK definitions throughout core.

pwolanin’s picture

FileSize
79.86 KB

Not sure this fixes anything for real, but fixes the static caching. need to figure out how to avoid calling menu_build_tree() twice per page.

drunken monkey’s picture

@sun: I did read the clarifying comments (though probably not all 250), it just sounded to me that callbacks shouldn't appear in breadcrumbs, not show no breadcrumbs. And your explanation just now seems to say the same, to me.

> A MENU_CALLBACK is an API callback end point without any corresponding menu link or breadcrumb
Doesn't this say that such a menu item shouldn't be contained in a breadcrumb? This would make sense to me, as you also wouldn't want to show, e.g., a link to an AJAX callback in a breadcrumb.

But if this is indeed intended behaviour than I just misunderstood and my objections are of course void. Sorry for that.

sun’s picture

FileSize
104.17 KB

Merged #250, but instead of trying to recursively invoke menu_build_tree(), I went ahead and merged #327918: Optimize menu_tree_page_data() performance into this patch. Interestingly, the changes from that patch for $only_active_trail are almost identical to the previous $translate changes of this patch, so merging was quite simple.

This means that menu_set_active_trail() builds a separate, but very tiny link "tree" just for the breadcrumb. As mentioned in #249, hypothetically comparing the possible solutions, I think this should be the faster one.

Status: Needs review » Needs work

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

sun’s picture

Status: Needs work » Needs review
FileSize
105.12 KB

Sorry, forgot to update one last remaining instance of 'translate' into 'only_active_trail'.

Just to make one point clear: If we are going to reach 300 here, then I'll replace this issue with a new one.

Status: Needs review » Needs work

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

drifter’s picture

Status: Needs work » Needs review
FileSize
104.91 KB

/user is in fact a MENU_CALLBACK; otherwise it will show in the Navigation menu as "User account". The search config test was tripping on that one since it was looking for the "User" string as a search tab.

Reverted "/user" to being a MENU_CALLBACK.

c960657’s picture

I tested the patch and generally it looks good. Nice work!

I notices two changes, though, that may not be intentional:

  1. With the patch, the breadcrumb for node/123 becomes “Home » Content” (if the node has not been added to a menu). I would expect it to be just “Home”, because the path node isn't really a parent page for all nodes.
  2. The breadcrumb for node/123/view is different from that of node/123, even though it is exactly the same page.
drifter’s picture

FileSize
105.36 KB

@c960657: have you installed Drupal _after_ applying the patch? The breadcrumb for node/123 does become just "Home" if you do that.

As for #2, true, node/123/view will include the node title (as it is a child of node/123). Is this a problem in practice though? The node "View" tab points just to node/123.

But, it's a MENU_DEFAULT_LOCAL_TASK. Should those be visible in the breadcrumb? I think not, they are really just dummy paths pointing to the parent, right? Not 100% sure about this, but removing VISIBLE_IN_BREADCRUMBS from the default task.

Status: Needs review » Needs work

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

c960657’s picture

You are right about #1. Sorry for causing confusion.

WRT #2 I don't think it is a very big deal, but I don't think it makes sense to have node/123 in the breadcrumb for node/123/view, because that would link to exactly the same page except on another URL. But I don't know why anybody would link to node/123/view in the first place instead of just node/123. BTW I initially noticed this issue on admin/structure/types/manage/article/fields/field_foo/edit vs. admin/structure/types/manage/article/fields/field_foo. You are taken to the former when you are creating a field, but when you edit the same field later on, you are taken to the latter.

tstoeckler’s picture

1. in #257 could be handled by adding a special-case in menu_set_active_trail() for MENU_DEFAULT_LOCAL_TASKs to build the breadcrumb of their parent. I don't know if that special case is actually worth it, though. I'd vote no.

#260 sounds like a bug on its own (minor, I guess, but still).

tstoeckler’s picture

sun’s picture

Status: Needs work » Needs review
FileSize
106.61 KB

Thanks for manually testing this patch.

Not including the parent link (tab_root) for default local tasks, but for default local tasks only, is quite a challenge though. Although it's uncommon, the menu system also has to support a second level of local tasks below the default local task. Which means that we cannot simply solve this through a simple comparison. For the same reason, it is wrong to remove MENU_VISIBLE_IN_BREADCRUMB from MENU_DEFAULT_LOCAL_TASK. For now, I've added a quick assertion for node/%/view only, but I'm going to have to add a testing menu tree structure in menu_test.module to properly test this breadcrumb scenario.

Aside from that, merging #327918: Optimize menu_tree_page_data() performance into this patch makes more and more sense in the meantime. It may even possible to generate "partial" breadcrumbs for weird access permission scenarios we were facing between #100 and #130; i.e., we might be able to skip higher, inaccessible links and only render the deeper accessible links in the active trail.

Status: Needs review » Needs work

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

drifter’s picture

OK, so removing breadcrumbs for all default local tasks is difficult. Never mind then, I think this is a minor nit. Most of the time links point to the default local task's parent anyway. I think you can remove the assertion for node/XXX/view, we shouldn't need to special case that.

sun’s picture

Status: Needs work » Needs review
FileSize
111.22 KB

wow. That was quite a challenge to figure out. Impossible without tests.

sun’s picture

FileSize
111.42 KB

Clusterfuck. Various page titles are broken now. I'd really like to know why on earth the page title is based on an invisible, last link in the breadcrumb... never really understood that weird relation between two completely irrelevant things.

Attached patch enhances assertBreadcrumb() to optionally assert a page title, too. It's possible that we need to move the additional array_pop() + lengthy explanation from set_active_trail() into get_breadcrumb().

sun’s picture

FileSize
111.45 KB

And further studying the code of menu_get_active_breadcrumb() revealed that this magic trail-to-page-title behavior has always been an issue, since the conditions for the existing array_pop() were entirely undocumented.

Thus, we finally have docs for that condition now. This patch should pass again.

pwolanin’s picture

@sun - that bit about the page title is one of the special nearly undocumented features of the D6 menu system! You can change a page title via the UI by re-naming the menu link for the page.

I'm not sure that makes any sense, but it seemed like a good idea at the time.

sun’s picture

Heh. Sure, I understand the relationship to a menu link. But I've no idea why that menu link needs to be crammed as last item into an array of breadcrumb links, and only ever be contained in there, so that it can be removed right before the breadcrumb trail is output... ;)

Anyway, that behavior is not changed in here. This patch ought to be ready. But it seems we have zero tests for expanded menu links, which happen to be completely broken with this patch applied.

pwolanin’s picture

Status: Needs review » Needs work

It needs to be the last item since there that was the hack I could think of to connect the current page to the rest of the hierarchy...

If expanded is broken with this patch then it needs to be back to cnw? I'd be totally happy to rip the expanded thing from core, but too late now I think.

drifter’s picture

What's broken with expanded menu links? With the patch applied, I set the "Add new content" link as always expanded - and it expands as expected, "Basic page" and "Article" are show on every page. So what's wrong?

sun’s picture

Sorry, I was mistaken. It's not the 'expanded' functionality that is broken. It's rather that (regular) menu trees are not depth-limited to the 'active_trail', so the assumption of the current menu rendering functions that an item with 'has_children' and $tree[below] must be an expanded link does no not work. In other words, the entire menu tree is passed to rendering, regardless of whether we actually want to show deeper links in the tree or not. While I like that behavior for a multitude of reasons (hint, hint, empty categories are not hidden, hint, hint), I'm pretty sure that pwolanin doesn't, also for a multitude of reasons. That change was not intentional though, so something is clearly wrong. To reproduce it, you need to create two nodes (or taxonomy terms or whatever) and put them into a parent-child menu link hierarchy. Access any page, and you'll always see the child link (as if the expanded option was enabled for the parent link, hence my initial suspicion). I've just debugged myself to sleep, but unfortunately can't provide any more insight, as I don't really see a difference to the old code if you forget about the other 110 KB that's unrelated to the functionality.

drifter’s picture

FileSize
4.64 KB

The 'expanded' brokennes was introduced in patch #263 - it worked fine until then. Attaching a diff of patch #258 and #263 - couldn't localize the issue further than that yet.

drifter’s picture

Status: Needs work » Needs review
FileSize
111.35 KB

Tracked it down to this bit at around line 1170:

+            $tree_parameters['expanded'] = $parents;
           }
-          $tree_parameters['expanded'] = $parents;

I put the $tree_parameters['expanded'] assignment back outside of the if statement. This fixes the problem - sun, please review if this is the correct way to solve it.

Also, modified a test to match new dblog terminology (#752048) - "Recent log messages" instead of "Recent log entries".

sun’s picture

Status: Needs review » Needs work

Good job, drifter! Thanks for figuring that out :)

However, stepping manually through the verbose Breadcrumbs test debug output makes crystal clear that we have to add menu tree output test assertions to this patch. Even though it's totally unrelated. Trust me, I'm tired of once again having to deal with technically unrelated menu system functionality in a wonderful focused patch like this.

I'm going to work on a assertion helper function similar to assertBreadcrumb() and some initial assertions now. Since we would have to duplicate the entire new Breadcrumbs test case to assert the menu tree output, we will have to mix these assertions into the Breadcrumbs test case.

What drives me nuts most is that it seems like all of the existing menu tests could as well be shredded. WTF are all of those test lines testing for, if it's not bare essentials like this? Seriously.

sun’s picture

Status: Needs work » Needs review
FileSize
115.52 KB

Here we go. Plenty of new assertions. And plenty of test failures.

Probably not going to work further on this today, so if anyone wants to try to resolve some of those failures, feel free to go ahead.

Status: Needs review » Needs work

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

sun’s picture

Status: Needs work » Needs review
FileSize
115.66 KB

Fixed 3 bogus assertions, the remaining are real bugs (which also seem to exist in HEAD).

Status: Needs review » Needs work

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

andypost’s picture

Forum breadcrumbs are depends on this patch too #148145: "Forums" title is not localized

catch’s picture

I'm really not keen on the API change in regards to MENU_NORMAL_ITEM / MENU_CALLBACK, although I guess the only contrib fallout is they don't get breadcrumbs for things defined with MENU_CALLBACK.

Adding needs benchmarks tag since when this passes we need to confirm the menu tree changes aren't a regression, although this is more of a 'needs profiling' really.

chx’s picture

Status: Needs work » Needs review
FileSize
109.81 KB

Here is one bugfix: if you somehow go to a default task url (node/1234/view) then the active link functionality breaks.

Status: Needs review » Needs work

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

sun’s picture

Status: Needs work » Needs review
FileSize
119.83 KB

Thanks, chx!

Here we go. This one should pass.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Quick! someone commit this before we figure out we want more or it breaks.

sun’s picture

@Dries/@webchick:

#576290 by sun, Damien Tournoud, drifter, quicksketch, chx: Fixed breadcrumbs.

webchick’s picture

Could we get some benchmarks here, please?

As well as a summary?

sun’s picture

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 Minimal installation profile.
  • 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.

I'm unable to do benchmarks on my computer, so I hope someone else can do that.

webchick’s picture

Wow, that's a bad-ass summary! Thanks!!

"The MENU_CALLBACK constant should be removed from all menu router items that previously defined it just because the path contained a dynamic argument."

Makes sense. What will be the consequence if this isn't done? I guess your breadcrumbs will vanish on you?

I'm booked up with phone calls until this evening PDT, but I'll take a closer look at this then.

sun’s picture

The effect of not removing MENU_CALLBACK 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).

moshe weitzman’s picture

My benchmarks are a bit worrisome. I'd like for someone else to confirm or refute them. I got a few connect failures in my runs so hopefully others get better results. I am seeing 6% slowdown in requests per second with this patch (page delivery time similarly increased). I opened up 'administer image styles' to anon users and requested http://mm/fr/admin/config/media/image-styles/edit/thumbnail/effects/1. I did a drush cc all between each run.

WITHOUT PATCH
~/htd/fr$ ab -c1 -n500 http://mm/fr/admin/config/media/image-styles/edit/thumbnail/effects/1
This is ApacheBench, Version 2.3 <$Revision: 655654 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking mm (be patient)
Completed 100 requests
Completed 200 requests
Completed 300 requests
Completed 400 requests
Completed 500 requests
Finished 500 requests


Server Software:        Apache/2.2.16
Server Hostname:        mm
Server Port:            80

Document Path:          /fr/admin/config/media/image-styles/edit/thumbnail/effects/1
Document Length:        6725 bytes

Concurrency Level:      1
Time taken for tests:   49.668 seconds
Complete requests:      500
Failed requests:        8
   (Connect: 8, Receive: 0, Length: 0, Exceptions: 0)
Write errors:           0
Total transferred:      3591500 bytes
HTML transferred:       3362500 bytes
Requests per second:    10.07 [#/sec] (mean)
Time per request:       99.336 [ms] (mean)
Time per request:       99.336 [ms] (mean, across all concurrent requests)
Transfer rate:          70.62 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:       31   49  20.1     42     136
Processing:     0   50  21.6     55     303
Waiting:        0   50  21.6     55     303
Total:         87   99  14.5     98     375

Percentage of the requests served within a certain time (ms)
  50%     98
  66%    100
  75%    101
  80%    102
  90%    105
  95%    116
  98%    126
  99%    135
 100%    375 (longest request)







---------------------
WITH PATCH
~/htd/fr$ ab -c1 -n500 http://mm/fr/admin/config/media/image-styles/edit/thumbnail/effects/1
This is ApacheBench, Version 2.3 <$Revision: 655654 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking mm (be patient)
Completed 100 requests
Completed 200 requests
Completed 300 requests
Completed 400 requests
Completed 500 requests
Finished 500 requests


Server Software:        Apache/2.2.16
Server Hostname:        mm
Server Port:            80

Document Path:          /fr/admin/config/media/image-styles/edit/thumbnail/effects/1
Document Length:        6725 bytes

Concurrency Level:      1
Time taken for tests:   52.997 seconds
Complete requests:      500
Failed requests:        4
   (Connect: 4, Receive: 0, Length: 0, Exceptions: 0)
Write errors:           0
Total transferred:      3591500 bytes
HTML transferred:       3362500 bytes
Requests per second:    9.43 [#/sec] (mean)
Time per request:       105.994 [ms] (mean)
Time per request:       105.994 [ms] (mean, across all concurrent requests)
Transfer rate:          66.18 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:       29   43  19.0     40     314
Processing:     0   63  15.5     66      79
Waiting:        0   63  15.5     66      79
Total:         96  106  10.1    106     314

Percentage of the requests served within a certain time (ms)
  50%    106
  66%    107
  75%    108
  80%    109
  90%    110
  95%    111
  98%    112
  99%    113
 100%    314 (longest request)

chx’s picture

Status: Reviewed & tested by the community » Needs work
This is ApacheBench, Version 2.3 <$Revision: 655654 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking localhost (be patient).....done


Server Software:        Apache/2.2.14
Server Hostname:        localhost
Server Port:            80

Document Path:          /7/admin/config/media/image-styles/edit/thumbnail/effects/1
Document Length:        7440 bytes

Concurrency Level:      1
Time taken for tests:   2.939 seconds
Complete requests:      100
Failed requests:        0
Write errors:           0
Total transferred:      788800 bytes
HTML transferred:       744000 bytes
Requests per second:    34.02 [#/sec] (mean)
Time per request:       29.393 [ms] (mean)
Time per request:       29.393 [ms] (mean, across all concurrent requests)
Transfer rate:          262.07 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    24   29   6.0     27      65
Waiting:       24   29   6.0     27      65
Total:         24   29   6.0     27      65

Percentage of the requests served within a certain time (ms)
  50%     27
  66%     29
  75%     31
  80%     32
  90%     34
  95%     41
  98%     52
  99%     65
 100%     65 (longest request)

With patch:


This is ApacheBench, Version 2.3 <$Revision: 655654 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking localhost (be patient).....done


Server Software:        Apache/2.2.14
Server Hostname:        localhost
Server Port:            80

Document Path:          /7/admin/config/media/image-styles/edit/thumbnail/effects/1
Document Length:        7440 bytes

Concurrency Level:      1
Time taken for tests:   3.412 seconds
Complete requests:      100
Failed requests:        0
Write errors:           0
Total transferred:      788800 bytes
HTML transferred:       744000 bytes
Requests per second:    29.31 [#/sec] (mean)
Time per request:       34.117 [ms] (mean)
Time per request:       34.117 [ms] (mean, across all concurrent requests)
Transfer rate:          225.79 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    30   34   4.5     33      58
Waiting:       30   34   4.5     32      58
Total:         30   34   4.5     33      58

Percentage of the requests served within a certain time (ms)
  50%     33
  66%     34
  75%     35
  80%     35
  90%     39
  95%     40
  98%     57
  99%     58
 100%     58 (longest request)

We need to figure out why the patch slows down Drupal this badly (note: this is with xcache. My SD is too big without xcache to be useful. Same for Moshe's results, btw: those numbers mean little as the standard deviation is just too large compared to the mean)

sun’s picture

Status: Needs work » Needs review
FileSize
112.56 KB

Reverted $only_active_trail, which is likely the cause for the performance drop. Was able to make it almost work without that, but luckily the tests are precise enough to leave us with 2-3 failures that most likely cannot be resolved so easily.

I've added a load of in-code explanation to this patch, to hopefully provide some clues to others in here.

Status: Needs review » Needs work

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

sun’s picture

Status: Needs work » Needs review
FileSize
111.78 KB

This one gets us back to something more sane.

Status: Needs review » Needs work

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

moshe weitzman’s picture

I am seeing no improvement since my last benchmarks. I suspect that some xhprof profiling would help isolate the slowness. Not sure I can get to that.

WITHOUT PATCH
~/htd/fr$ ab -c1 -n500 http://mm/fr/admin/config/media/image-styles/edit/thumbnail/effects/1
This is ApacheBench, Version 2.3 <$Revision: 655654 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking mm (be patient)
Completed 100 requests
Completed 200 requests
Completed 300 requests
Completed 400 requests
Completed 500 requests
Finished 500 requests


Server Software:        Apache/2.2.16
Server Hostname:        mm
Server Port:            80

Document Path:          /fr/admin/config/media/image-styles/edit/thumbnail/effects/1
Document Length:        6725 bytes

Concurrency Level:      1
Time taken for tests:   48.419 seconds
Complete requests:      500
Failed requests:        6
   (Connect: 6, Receive: 0, Length: 0, Exceptions: 0)
Write errors:           0
Total transferred:      3591500 bytes
HTML transferred:       3362500 bytes
Requests per second:    10.33 [#/sec] (mean)
Time per request:       96.838 [ms] (mean)
Time per request:       96.838 [ms] (mean, across all concurrent requests)
Transfer rate:          72.44 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:       30   43  12.7     41     111
Processing:     0   54  15.3     54     207
Waiting:        0   54  15.3     54     207
Total:         87   97   8.7     96     246

Percentage of the requests served within a certain time (ms)
  50%     96
  66%     99
  75%     99
  80%    100
  90%    102
  95%    105
  98%    111
  99%    126
 100%    246 (longest request)



WITH PATCH
~/htd/fr$ ab -c1 -n500 http://mm/fr/admin/config/media/image-styles/edit/thumbnail/effects/1
This is ApacheBench, Version 2.3 <$Revision: 655654 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking mm (be patient)
Completed 100 requests
Completed 200 requests
Completed 300 requests
Completed 400 requests
Completed 500 requests
Finished 500 requests


Server Software:        Apache/2.2.16
Server Hostname:        mm
Server Port:            80

Document Path:          /fr/admin/config/media/image-styles/edit/thumbnail/effects/1
Document Length:        6725 bytes

Concurrency Level:      1
Time taken for tests:   53.821 seconds
Complete requests:      500
Failed requests:        6
   (Connect: 6, Receive: 0, Length: 0, Exceptions: 0)
Write errors:           0
Total transferred:      3591500 bytes
HTML transferred:       3362500 bytes
Requests per second:    9.29 [#/sec] (mean)
Time per request:       107.642 [ms] (mean)
Time per request:       107.642 [ms] (mean, across all concurrent requests)
Transfer rate:          65.17 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:       31   45  18.3     41     156
Processing:     0   62  19.0     64     227
Waiting:        0   62  19.0     64     227
Total:         96  108  11.2    106     299

Percentage of the requests served within a certain time (ms)
  50%    106
  66%    109
  75%    110
  80%    110
  90%    113
  95%    119
  98%    131
  99%    141
 100%    299 (longest request)



pwolanin’s picture

Priority: Critical » Major

@sun - I was going to comment last night that looks like we were back to building that data 2x in the latest patch. Perhaps part of the problem is that in the _menu_link_translate(&$item) we are using the 'access' flag for both actual access and to suppress visibility for dynamic paths.

Perhaps we can tweak _menu_tree_check_access() and related code so that we always build the links in the active trail for the breadcrumb? I am imagining part of the performance hit is that we are calling the access checks on a lot of links that will never be visible in the menu block.

I also feel like this needs at this point to be moved to "major" not "critical" in terms of releasing D7. Given that we are about at 300 - let's open a new issue with the recent summary and patch.

philbar’s picture

Priority: Major » Critical

Beta blocker = Critical

We want to iron out all the UI Changes before the Beta launch.

webchick’s picture

Priority: Critical » Major
Issue tags: -beta blocker

This isn't a beta blocker. The current situation exists in D6.

philbar’s picture

Works for me!

Can someone with permissions update the Community Initiative page?

We need to remove the following, which are no longer beta blockers:
#576290: Breadcrumbs don't work for dynamic paths & local tasks
#706842: Improve comments for the taxonomy upgrade path

sun’s picture

Status: Needs work » Closed (duplicate)
sun’s picture

#907690: Breadcrumbs don't work for dynamic paths & local tasks #2 now contains a patch (based on #295) that is likely to pass all tests without performance penalty.

klonos’s picture

...last post is spam.

jhodgdon’s picture

Thanks... The right thing to do is to file an issue in the drupal.org webmasters queue to report this. I will do that...

klonos’s picture

I knew that ;) ... it's gone now.