Regression: Breadcrumb doesn't always include all active items

In working with aggregator this morning, I noticed it is also hit by this bug -- when one edits a feed, the breadcrumbs do not display correctly (see attached screenshot).

This patch no longer applied, so I rerolled it, and it solved the breadcrumb issue. I'm not qualified enough [yet] to really speak to the mechanics here, so I'll defer that to wiser souls among us.

Related issue.

Bumping this up to the top of the list in the hope that a menu or breadcrumb guru will see it and review.

Rerolling, since the previous patch had a good bit of fuzz.

As a note, I just tested this again, and the aggregator breadcrumbs (when editing a feed) are certainly incomplete without it (but are ok with it). There may be other similar instances in core as well.

The patch in #4 applied to HEAD with a bit of offset so I re-rolled it.

The breadcrumbs are still incomplete when editing feeds in Aggregator (and possibly other places), and this patch still fixes it.

I hit this bug too.

All items with a wildcard in them are not showing up in $curr['below'] because they are being removed by _menu_tree_check_access after $item['access'] has been set to FALSE in _menu_link_translate. The latter code (menu.inc:621) is accompanied with the following comment:
// Note - skip callbacks without real values for their arguments.

I think the core of the problem is that a router item is passed into _menu_link_translate while that function expects a link item. Or _menu_link_translate is simply unwilling to deal with a link_path with a wildcard in it, while it should better deal with it since they do need to appear in breadcrumbs.

This patch will run the item through _menu_translate if it's in the active trail and contains wildcards, instead of simply abandoning it. It appears this will fix this bug.

I'm not certain this solution is any good.

maybe this is only a problem for node links? There is some special-case code for them for access checking to improve performance.

pwolanin: no, this is not only a problem for node links. Please read the initial bug report.

ah, right, well for local tasks this is a known problem, though I'm not sure about the patch code.

At the least, I think you need to use _menu_translate($item, $map, TRUE); as is done here: http://api.drupal.org/api/function/menu_local_tasks/6

    * notice: Undefined index: path in /Users/Shared/www/drupal6/includes/menu.inc on line 565.
    * notice: Undefined index: number_parts in /Users/Shared/www/drupal6/includes/menu.inc on line 566.

on node/2/edit or admin/settings/filters/1

pwolanin: just to clarify, this problem is not only happening with local tasks. It's happening with everything that has a wildcard as one of it's ancestors.

I'll just set the status back to active until a menu-guru takes a look at this.

Maybe this issue should be marked as critical, since it would be bad, I think, if something simple as breadcrumbs don't fully work when 6.0 is released, while they did work in 5.x.

Marked http://drupal.org/node/220084 as a duplicate. Perhaps http://drupal.org/node/184955 and http://drupal.org/node/139015 are duplicates too?

I said there already that "breadcrumb does not always include all active items" does not make your site horribly broken, so does not mandate a critical flag. I'd be surprised if the breadcrumb would be such an important part of any website.

Confirmed that this is still a problem. I don't think it quite qualifies as a critical though.

Gábor: if the breadcrumb is inconsistent then users might get confused when navigating the site. Additionally, sometimes the breadcrumb allows you to navigate back to a page that is not always linked in the navigation menu.

But indeed: critical, no.

Patch in #5 fixed two different breadcrumb issues for me.
Please give it an another review, and set it to RTBC.

Doesn't this patch in #5 throw a warning?

if there is nothing below, the 'below' element is set to FALSE: http://api.drupal.org/api/function/_menu_tree_data/6

Do you see the error:

PHP Warning:  Variable passed to each() is not an array or object

@pwolanin I did not see this warning yet. Do you know any page which should throw the warning?

Patch in #5 fixed these breadcrumbs for me:
(note: The described examples are in simplified form and may have functional differences with my actual tests)

issue1: callback 2 level deeper, shows correct breadcrumb now

  $items['test'] = array(
    'title' => 'Test',
    'type' => MENU_NORMAL_ITEM,
  );

  $items['test/edit/%'] = array(
    'type' => MENU_CALLBACK,
  );

issue2: callback under menu_suggested_item shows the title (Test) in breadcrumb
It would be better for me if I could change menu_suggested_item to callback, but in that case the breadcrumb does not show up.

  $items['test'] = array(
    'title' => 'Test',
    'type' => MENU_SUGGESTED_ITEM,
  );

  $items['test/%'] = array(
    'type' => MENU_CALLBACK,
  );

---
issue3: I am still struggling with other problem

  $items['test'] = array(
    'title' => 'Test',
    'type' => MENU_SUGGESTED_ITEM,
  );

  $items['test/%wcard1/add'] = array(
    'type' => MENU_LOCAL_TASK,
  );

  $items['test/%wcard1/edit/%wcard2'] = array(
    'type' => MENU_CALLBACK,
  );

test/%wcard1/add -> shows correct breadcrumb "Test > %wcard1 (changed to correct title)"
test/%wcard1/edit/%wcard2 -> shows breadcrumb "Home" (I thought it should work, because issue1 and issue2 works..)

maybe something like this? Otherwise I think you are trying to run each() on a variable that == FALSE;

The last change in the patch seems logical, but can't test it because I haven't seen the warning..

Is there any chance that there will be a solution for issue3? I am trying to create the correct menu structure for my module, which is without collisions (see issue #220407: Fix collisions in admin menu). This seems like a good structure for me, but the breadcrumb and the actual menu item is totally lost.

Subscribing. This effects the Services module in Drupal 6.

I stumbled on this bug with a different use case

I created a module that used a wildcard, and as long as the module was in the navigation menu, everything was fine, but when I moved it to a custom menu, the breadcrumb was incorrect.

It seems that the menu_get_active_menu_name() returned a wrong menu.

I fixed it by adding the following lines in menu_set_active_trail( ... )

// Retrieve the menu item using the root path after wildcard replacement.
$root_item = menu_get_item(implode('/', $parts));
if ($root_item && $root_item['access']) {
$item = $root_item;
}
}

+ $menu_link = menu_link_load(db_result(db_query("SELECT mlid FROM {menu_links} WHERE link_path = '%s'", $item['path'])));
+ menu_set_active_menu_name($menu_link['menu_name']);

$tree = menu_tree_page_data(menu_get_active_menu_name());
list($key, $curr) = each($tree);

while ($curr) {
// Terminate the loop when we find the current path in the active trail.

I encountered both of these problems too.

My solution was a combination of merely changing the if ($curr['below']... to if (is_array($curr['below']...

And I also made use the very helpful suggestion from mayco, which allows this to work on menus other than navigation.

However if I go to a page like admin/content/node-type/book/delete, there are no breadcrumb links, only Home.

So something is still not quite right. Here is my final patch, which solves my particular problem, but FYI it is against 6.2, not HEAD.

subscribing. this really needs to be fixed.

allows this to work on menus other than navigation

there is also a module with the same functionality as your patch..
http://drupal.org/project/menu_breadcrumb

can we commit the patch in #21, and open another issues for the remaining problems?
that would be a huge help for me..

We'll have to commit it to the Drupal 7 branch first, and then backport it to 6.

patch #21 for Drupal 7

patch #21 for Drupal 6

I think this is the best solution code-wise, and is a very minor change. I re-rolled the patch for 7.x to improve the code comment.

Same patch applies to Drupal 6 with offset.

Patch in #32 fixes the problem I reported in http://drupal.org/node/258660 for D6. +1 from me.

I've committed this to CVS HEAD. I leave it to Gabor to decide whether this needs to be backported to Drupal 6, and if the current patch qualifies. Thanks all!

IMO, this is a bug in Drupal 6, and this patch should qualify as a bug fix. In my opinion, of course.

Thanks Dries. This is a bug and by no means a feature - so I think it should go in D6 also.

Thanks. The latest patch applied with a small offset to D6. Looked good, committed.

menu_get_active_menu_name

seems to allways return navigation. I think its a related bug so i post it here drupal version 6.15