Menu & Breadcrumb Conditions & Reactions use menu selection but process based on path. Can cause counter intuitive results!

I've seen this issue as well.

If the same path exists twice in the menu table, you only get the first one, when using menu's lookup functions. menu_get_item() uses db_fetch_array, and it just returns the first value it found...

Lots of users double up menu items because it makes sense to them visually, or because they expect to place their menus where, possibly in combination with other unrelated menu items...unrelated according to how some other menu in their site might be structured.

The same is true of breadcrumbs, which rely on menu lookups too.

I think this is a problem more generally in Drupal - Modules that rely upon menu module functions (as they should) don't have any way to address looking for more than one instance of a path in the menu system.

That's probably a 'works as designed' situation, since the menu system is meant to both handle the display of hierarchical menus to users and the mapping of functions to urls which ought to be unique.

But to end users and non-developer site builders this is hugely frustrating. What can we do to make setting 'active-menu' and 'breadcrumb' settings actually stick in these kinds of cases?

I'm pretty sure there are issues around the portability of menu exports in the features module that will prevent an implementation that uses the mlid field to explicitly catch which menu item you want...but that would be the first approach I'd imagine to get what we want from setting a menu as active with Context...

Thoughts?

Can context provide a dropdown for menu_name AND a selection for link_path, then look up the item using both values?

I just stumbled upon this issue in a multilingual site with different primary links menu per language. All menus contain a menu item for the same single-page view. Though I have created different contexts for each language, the breadcrumb shows the item name of the latest menu item created for each language. I had to use a workaround (either create separate views for each language or turn on menu translation. But still this is incorrect behavior IMO.
Subscribing.

This seems like a huge bug to me. My site has a footer menu which duplicates some of the links found in the primary navigation, so of course that is the menu Context is considering active, even though I selected the primary links menu item when I created the reaction.

I suppose Context could store the menu name, in addition to the link path, but that will also break if the same item is listed more than once in a single menu (which I have done before).

The best solution seems like it would be storing the unique mlid, instead of the link path. @dsayswhat, the menu link currently being stored by Context is no less site-dependent than the mlid. Right now a context on my site is trying to activate "node/9" which would be just as meaningless on another website as if it tried to activate "mlid 2098".

Sub. I've been having some issues about it.

I agree with @jstoller about storing the mlid instead of the path. It is very common with larger and more complex sites to put the same path in more than one menu. If the mlid is stored, then all is good.

[update] I just got the breadcrumb to work properly by making the following changes. Perhaps someone can test other scenarios. I would be willing to make a proper patch if the code (below) works and the patch would be accepted (I'm not sure if the maintainers want the mlid stored instead of the path though it makes sense to me to do that). In order to make it backwards compatible, though, then either the query needs to check both mlid and link_path/href *or* there needs to be an "update" in the install file that converts all existing paths for the contexts into mlids (by just grabbing whichever the first one). The latter would be more solid but requires more work and more testing.

1) edit context_reaction_menu.inc and update:

//$identifier = $link['link_path'];
$identifier = $link['mlid'];

2) edit context_reaction_breadcrumb.inc and update:

//$result = db_query("SELECT p1, p2, p3, p4, p5, p6, p7, p8 FROM {menu_links} WHERE hidden = 0 AND link_path = '%s'", $path);
$result = db_query("SELECT p1, p2, p3, p4, p5, p6, p7, p8 FROM {menu_links} WHERE hidden = 0 AND mlid = '%s'", $path);

3) edit context_condition_menu.inc and update:

//$identifier = $link['link_path'];
$identifier = $link['mlid'];

and

//if ($menu_name = db_result(db_query("SELECT menu_name FROM {menu_links} WHERE link_path = '%s'", $item['href']))) {
if ($menu_name = db_result(db_query("SELECT menu_name FROM {menu_links} WHERE mlid = '%s'", $item['mlid']))) {

Kristen

This looks great! I'd love a patch and give a big +1 to its inclusion in the module.

A couple observations: It seems like $active_paths should be changed to $active_menus and $path to $mlid, for consistency. And in the above queries you give the mlid as a string, but shouldn't it be an integer? ("mlid = %d", not "mlid = '%s'")

I don't have so many contexts using this reaction that I couldn't manually update them, but ultimately I think an update function in the install file is the way to go. Checking both the path and mlid doesn't seem like a good long-term solution.

I too ran into this problem with a site I am building. However, I am using the Drupal 7 version of context. I rolled a patch against 7.x-3.0-beta1, and it appears to work.

Weird... I am checking back in on this and see there are new comments but I don't remember getting notified.

So, for some reason, I got a bee in my bonnet last night to try to create a patch for this. It turns out that it is not so simple because the get_active_paths() is used in a few places that were not obvious how to translate into using the mlid.

I have a patch to start with and it may be sufficient.

1) it changes the key for the selected path from

[link_path]

to

[link_path]|[mlid]

so it retains both needed data.

2) added a get_active_mlids to accompany the get_active_paths function

3) change the breadcrumb reaction to use the mlid

4) there is an update in install that will upgrade your context values to match the new format in (1)

The part I'm not sure of is what to do with the menu condition/reaction so I have left as is. Maybe they don't need to be changed at all and can continue to use the get_active_paths.

Please test and report if it is working or not.

Thanks,
Kristen

I tried the patch but it made no difference for me. The breadcrumbs are still pointing to the wrong paths.

@pfrenssen - did you run update.php after applying the patch?

I don't remember unfortunately. Possibly I didn't.

Patch seems to work.
Except for the following problem with the update in the install file.

--- 363,423 ----
+   // find the matching mlids
+   $mlids = array();
+   $results = db_query("SELECT mlid, link_path FROM {menu_links} WHERE link_path in (".db_placeholders($paths, 'text').") AND module = 'menu'", $paths);
+   while ($row = db_fetch_object($results)) {
+     $mlids[$row->link_path] = $row->mlid;
+   }

This needs to check whether or not $paths is empty. If it is you currently get an sql error.

sub

Ran into this problem as well, subscribe.

Exactly what I'm needing fixed in Acquia Commons - subscribing!

Hi there,

The patch works so now I have breadcrumbs on my context but now the menu trails have stopped working and I can't get it to write the active-trail on the main menu. I am using menu blocks and it's impossible to display the menu block if there isn't a trail on the menu, any ideas??

@leenyx:
Have you tried the Context: Menu Block module? That is supposed to do what you are asking.

Same problem here with D7 using Menu Firstchild items.

In the context menu selectbox, I've got many options pointing to the same path, which in my case is <firstchild>. When the context is stored, it grabs the first menu item pointing to <firstchild>.

+1 for storing the mlid instead of the path.

+1 for storing mlid instead of the path. Using as well.

Any news on this issue? Considering the issue is more than a year old and still not fixed ...

Bug confirmed on D7. :(

Rerolled patch from #9 which applies cleanly at 151f925

Patch in #25 works for me.

However, an update hook would likely be needed to convert the stored path to the mlid.

This is a problem in the Menu Condition as well as the Reaction. Patch #25 only addresses the Reactions.

The D7 patches are out of date now as the menu reaction code has all changed.

Here is a quick one I made that I think works but I haven't tested very thoroughly.

One thing I do notice is that it won't fix everyone's problems.
For example if you are using the special menu items module or something and have a bunch of menu items that have the same path you will likely still have issues with setting the active trail. That is not really context's problem though and relates to how drupal sets active trail so I'm not sure if there is an easy, non-hacky fix (I'm currently working on a hackier fix for myself for this purpose. - I will post back here if it works.)

Anyway here's what I have if people want to test, make changes or whatever.

It still doesn't have an update hook.

Nope my hack isn't going to get around it.
If you have the same path for multiple menu items you are always going to have potential problems with active state.

Here is a patch that implements mlids for menu conditions.