menu_get_active_breadcrumb breaks breadcrumbs in primary links

I don't think the current page is suppoosed to be in the menu array. it is delivered as the page title ... the lack of an 'active' attribute might be a bug though.

This is a regular old bug (if at all). downgrading.

the current page is not in the active trail - this is by design. There are other issues about this.

The l() function adds the ".active" class to a link for the current page.

For what you want, it might be easier to use this: http://api.drupal.org/api/function/menu_tree_output/6

you could get the Primary links - and then either show them all, or truncate at the top level, etc.

I can confirm this, I think. When you have link 1 -> link 2 -> link 3 in the navigation menu and you're viewing link 3, menu_get_active_trail() returns them all (which seems correct, as menu_get_active_breadcrumb() removes the current page from the breadcrumb later). But when the links are in the Primary Links menu, it only returns the front page and the current page, and not the rest of the trail. The breadcrumb in this case contains only a link to the home page. This would seem to suggest that the same problem exists for all menus other than Navigation.
[edited to add attachments]

This isn't critical.

The active trail is normally generated based ONLY on the Navigation menu - thus the behavior you describe the the correct and intended behavior. It would be nonsensical to generate the active trail based on multiple menus - a link to the current page might be several menus.

If you want this function to generate the trail based on the primary links menu, perhaps you could call menu_set_active_menu_name() first.

you'd have to set this fairly early to make it work - probably in a module in hook_init. Where in the code are you trying to set it?

theme level is almost certainly too late. However - your code might be faster if you use http://api.drupal.org/api/function/menu_tree_page_data/6 since the result for the Primary Links menu will have already been generated and cached.

Generating the active trail based on a single menu certainly makes sense... but that menu should REALLY be configurable. As a use case scenario just look at this site. Having the active trail generated from the Navigation menu simply doesn't make sense for a lot of sites out there.

Sadly it seems unlikely this will be fixed by the release of Drupal 6. This is a flaw that is going to be the source of significant confusion.

Edit: But then again, changing the menu the active trail is generated from to anything other than the Navigation menu would cause problems with generating breadcrumbs for the rest of the drupal system. Hmm... I'm not sure this has been thought through very well.

Suppose following Menu's

[Navigation]
link 1 = node/1
- link 2 = node/2
- - link 3 = node/3

[Primary Link]
link 4 = node/4
- link 5 = node/5
- - link 6 = node/3 (ie.. same as link 3)

In the examples below i'm displaying breadcrumbs, because they visually map with the get_active_trail array.

1. Action : Clicking Link 2.
Expected breadcrumb : Home > link 1
Resulting breadcrumb in D5 : Home > link 1
Resulting breadcrumb in D6 : Home > link 1

2. Action : Clicking Link 3.
My Expected breadcrumb : Home > link 1 > link 2
Resulting breadcrumb in D5 : Home > link 1 > link 2
Resulting breadcrumb in D6 : Home > link 1 > link 2

3. Action : Clicking Link 5.
My Expected breadcrumb : Home > link 4
Resulting breadcrumb in D5 : Home > link 4
Resulting breadcrumb in D6 : Home

4. Action : Clicking Link 6.
My Expected breadcrumb : Home > link 4 > link 5
Resulting breadcrumb in D5 : Home > link 1 > link 2
Resulting breadcrumb in D6 : Home

In my opinion.. D5 was more consistent with my expectations.. Only when there was a ambiguity, D5 chooses the first breadcrumb that was found. D6 can only handle breadcrumb for items in the Navigation Menu.
All this is the result of http://api.drupal.org/api/function/menu_get_active_menu_name/6 which ALWAYS returns 'navigation' unless the http://api.drupal.org/api/function/menu_set_active_menu_name/6 has been called before.

You could call this 'by design' but i would like to see the judgment for this design.
The feature request in #14 just moves the problem to another menu getting more *attention* than all the others.

Let me repeat what I was saying over in #176176: Breadcrumbs only show Home

To me, this seems like a pretty big regression compared to D5.

Almost all websites are built by putting pages in the primary links menu, not by putting them in the navigation menu. While having menu_get_active_trail() return the trail via the navigation menu may be "by design". Having menu_get_active_breadcrumb() use that same "navigation-menu-based" trail is a regression.

Clearly, if your breadcrumb is just a "Home" link, it is a broken breadcrumb.

Possible solutions: menu_get_active_breadcrumb() could be expanded to check the primary links trail if the navigation-based trail is too short. Or we could look to see which menu the path belongs to and use that for the breadcrumb and have some sort of arbitration when it belongs to multiple menus.

Here's a patch that checks if the navigation-menu-based breadcrumbs are just the Home link, and, if so, grabs the breadcrumbs from the Primary links menu.

It feels hackish since I need to add a reset param to menu_set_active_trail(), but the patch is pretty short. All simpletests still run successfully after applying it ("4890 passes, 0 fails, 0 exceptions").

And, speaking of “hackish”…

Reseting the active trail is no good since what we really need is the ability to cache the active trail per menu. Once we calculate the active trail for a particular menu name, I have no reasons for wanting to reset and re-calculate it.

So, this patch changes menu_set_active_trail() and menu_get_active_trail() to add a new $active_menu_name parameter. menu_set_active_trail() now caches the active trail per menu name. And the logic in menu_get_active_breadcrumb() becomes much simpler (compared to the previous patch.)

yes, I had been thinking something along these lines. However, if you are proposing such a change for D7, it seems the active trail code might just have a hierarchy of menus it checks to see if it can find one where the current page is present?

You could actually get all such menus with one query, and maybe have an admin setting to pick the order/hierarchy in which menus would be used to define the active trail.

Also, I think there is already a similar issue open by beginner.

yes, I had been thinking something along these lines. However, if you are proposing such a change for D7, it seems the active trail code might just have a hierarchy of menus it checks to see if it can find one where the current page is present?

Playing with some non-primary links menus in D5 and checking out the breadcrumbs, I see that you are correct about D5's behaviour. It uses the menu tree that the page's menu is defined in, not just primary links. I'll need to fix that in the patch.

You could actually get all such menus with one query, and maybe have an admin setting to pick the order/hierarchy in which menus would be used to define the active trail.

Yes, drag-and-drop ordering of menus to pick breadcrumbs from would be cool. :-) But I'm actually just trying to fix the bug in D6 and not do a "feature request" for D7.

And beginner's #73834: Allow multiple breadcrumbs on a page and #150854: fix breadcrumbs arbitration bug are definitely separate issues from this one.

Albion,

Awesome work. I finally stumbled upon this thread after looking for resources regarding this issue for a long time. I don't get why earlier posts seem to think this a minor issue or that it acts as intended with the navigation menu.

Most sites create their main content links as primary links and use the navigation menu strictly for user profiling information and other "admin" type modules. Breadcrumbs are also a critical component of design today in most sites. Absolute key issue, and I need to get this working myself!

Going to be looking at your patches and hopefully provides a temporary solution.

Reverting version. This needs to get fixed in the latest code (D7) and then backported to D6.

Straddle, if you need a fix now, you can also look at http://drupal.org/project/menu_breadcrumb

Hello JohnAlbin

Thanks for your great patch: http://drupal.org/files/issues/breadcrumbs-primary-links_0.patch

I'm using Drupal 6.4 and I think, that your patch should be immediately added to Drupal 6.5.

I had a huge problem with wildcard menus in the primary navigation:

menu-item: "author/browse" was working: Breadcrumb Home->Author->Browse

menu-item: "author/browse/%" was not working: Breadcrumb: Home->%

After applying your patch and using the menu_breadcrumb module, it works like a charm! Thanks alot!

Revert vandalism. The patch itself looks good, but I would like to have chx' review this.

Guys how do I implement the patch?

The patch unfortunately only fix the breadcrumb. The menu active trail is still not correct for menu items with wildcards.

I need the active trail to highlight the menu-items, that are in the active menu trail.

If I use a menu-item with a wildcard like "author/browse/%" and the parent item "author/browse" is in the primary links, the menu trail is not highlighted. But i don't now why...

But how do I implement this particular patch?

Nobody?

@manan

I followed the description on http://drupal.org/patch/apply. I'm using Eclipse that is providing a GUI for applying patches.

@pwolanin:

The active trail is normally generated based ONLY on the Navigation menu

This does NOT make sense to me, and is not the way 5.10 works. What difference does it make to the end user which menus got them to where they are? All they want is a way back. This is not happening in 6.4 - they are lost.

Another side effect of this problem, I believe, is that an admin overridden menu title is not used for the page title when the menu item comes from the Primary links. For example, take any module-generated menu item and change its title. That title shows as the page title. Now move that item to Primary links and all of a sudden the module-specified title is back rather than the admin-specified title, even though the admin's title shows in the primary links. Additionally, the breadcrumbs shows the module-specified title, so the trail back to where they came from is broken.

IMHO, this is just wrong and could very well discourage people from upgrading to 6.x.

I asked chx to quickly review this on the IRC the other day [on September, 6 around 19:15 GMT+2]:

DamZ: chx: could you give a look at http://drupal.org/node/184955 ? The patch itself looks good, but you master the menu system more than anyone.
chx: DamZ: i will ask pwolanin :) looks good at first sight.

chx: i will ask pwolanin :) looks good at first sight.

heh. See pwolanin response in #20 above. :-)

Setting this back to CNW, based on my note in #21.

Playing with some non-primary links menus in D5 and checking out the breadcrumbs, I see that you are correct about D5's behaviour. It uses the menu tree that the page's menu is defined in, not just primary links. I'll need to fix that in the patch.

I should get some time later this week to fix this patch.

Well, I am certainly hoping to see this in 6.5. I consider this a serious bug. I know of one contrib that has already had an issue opened against it when this is really a core issue. I have many sites where these menu items are moved to Primary links and would hate to have lots of confused users.

I added the following to hook_init.

  if (arg(0) == 'weblinks') {
    $active_trail = menu_set_active_trail();
    foreach ($active_trail as $key => $item) {
      if ($item['path'] == 'weblinks') {
        $active_trail[$key]['title'] = _weblinks_get_menu_title();
      }
    }
    menu_set_active_trail($active_trail);
...
function _weblinks_get_menu_title() {
  return db_result(db_query("SELECT link_title FROM {menu_links} WHERE link_path = 'weblinks'"));
}

I'm thinking that maybe menu_get_item should JOIN on {menu_links} for the title.

@NancyDru - patches welcome. I don't use the BC much when I navigate, and getting it to work at all in D6 took many hours of my time.

did not mean to set to CNR.

@Peter: now that I understand the problem better than I would like to have to, I can think about a patch. I don't use BC much myself, but I have users who rely heavily on them. I appreciate your efforts on getting them to work and I really like the change to the way they are built (although that took hours of my time to straighten out).

I think this is simpler and seems to work for everything I've tested. BTW, it is against 6.4, not 7.

didn't have "includes\".

one more time...

Okay, now it is against 7.x-dev. The only difference for 6.4 is the line number (312).

@NancyDru - um, are you sure that's the right patch. you should absolutely not be changing anything in that function to deal with a BC problem.

What might be more helpful than a patch right now is if someone could explain clearly how the BC worked (or was supposed to work) in D5, and pinpoint where this differs in D6.

Yes, that's the right patch. Like I said I developed it on 6.4, not 7.

Actually I came to this issue for a different reason. That's that the page titles don't reflect the admin-specified title when the menu item is moved out of the Navigation menu, for example to Primary Links. However, I believe both symptoms have the same cause, and that is that the router table does not contain the admin-specified title; it is only in the links table. AFAICT, the lowest point to "fix" these symptoms is that function. And it seems to work fine for me and tested on several modules and menu combinations that were wrong before.

And, yes, if you think this is a separate issue, I will be happy to open a new one.

@NancyDru - I agree that the issues are connected, but I think the patch above is totally wrong. If it worked, it's only by chance that you had unique links or some other happenstance.

It clearly fixes the page title problem. But I am willing to see what else develops.

subscribing

subscribing

Subscribing.

Subscribing. I'm having issues related to breadcrumbs / active trail in primary links on all of my drupal pages...

The BC aspect, at least, was fixed by: http://drupal.org/node/273137

Its seems there is a module that fixes it : http://drupal.org/project/menu_breadcrumb

Haven't tried it yet though