Menu Breadcrumb setting active menu to undesirable menus

This is how I think it could work: iterate through matching menu items (which means quite some overhead without menu weights) and use the first item with positive access permissions. Patch is completely untested.

I agree that the sloppy select is the problem, but this module could run into the same problem with book module, or any other module using the menu_links table. Important note: LINK PATHS ARE NOT UNIQUE.

If you want the link saved by the system module for this path (which should be unique), change the query to specify that:

$link    = menu_link_load(db_result(db_query("SELECT mlid FROM {menu_links} WHERE link_path = '%s' AND hidden = 0 AND module = 'system'", $item['href'])));

Also note, you probably want hidden = 0, since that column can have values, 1, 0, or -1.

Note- this looks like the same problem code that appears in the Local Menu module : http://drupal.org/node/296534#comment-998251

An alternative/additioanl approach would be to make sure you only set as the active menu one of the menus managed by menu module. They are listed in table {menu_custom}.

Hi,

@sun, pwolanin: Have we actually taken the time to consider what should be the correct behaviour before jumping into implementing code?

If a link (ie. node/add/story) appears in two menus (ie. navigation and admin_menu), what should the breadcrumb be?

Should it be // home // create content ?

Or should it be // home // content management // create content ?

Or should it just randomly pick between the two? :)

It's ambiguous, and I don't think menu_breadcrumb nor necessarily the Drupal menu system was designed for this ... which would mean something is fundamentally broken.

First of all, sorry for my late reply. I've been occupied with my work lately and had/have little spare time to check the issues queue.

@smk-ka and @Proct0t:
I tried the fix mentioned by smk-ka, and think it's good to check the access attribute since it only gets set and not used by the breadcrumb menu. However, there are cases like Proct0t correctly stated where the user has access but the 'bug' still occurs. For example, using the code with the patch with the Administration Menu module, going to the user login page still gives troubles.

@pwolanin, @bengtan and @Proct0t:
Indeed link paths are not unique and ambiguous. Unfortunately I would say by it's nature it's impossible to tell to which menu a menu item belongs (without proper context or some convention).

To partially solve these problems, I would propose the following for the Menu Breadcrumb module :

1. Check all possible menu links (not just the first one) and check them for access and the possibly the hidden flag. For this the proposed patch and code provided in this thread can be used.
2. An option where a user can enter/select the names of the menus which Menu Breadcumb must ignore.
3. An option where a user can set an order/preference of menus which Menu Breadcrumb will use and prefer.

These solutions might not be perfect, but imho it's better to have a workable fix than a bug for months until a better solution is present.

I didn't have too much time to delve into the problem so maybe the proposed solutions may not be right or I might be missing something. So any comments are more than welcome.

I think it would be smart to add the ability to exclude certain menus from use. The Navigation (Administer) menu, specifically, is such a common and obvious case that you might even like to disable it by default. Good suggestions!

Hi - has there been any further development on this issue? Thanks :) Scott

I've created a patch that lets you ignore certain menus. I can't login to the CVS server from the office, so the diff was done using Perforce. If should still be valid(ish).

oops! There was a typo in the patch.

@gdevlugt I like the feature set you mentioned. Limiting Factor's patch only tackles #2, but it works great!

I marked admin_menu, devel, and navigation as “Ignore menus” and that lets Drupal core handle the breadcrumbs for the navigation menu (so no more favicon in the navigation breadcrumb :-p) and /user breadcrumbs works now too. woo-hoo!

I re-rolled the patch in CVS, but made no changes otherwise.

I'm marking this RTBC since it seems to do #2 well. We still need code for the #1 and #3 bits.

Blargh. I spoke too soon. The previous patch (in #11 and #12) doesn't work since there is a weirdness with in_array() and zero values. See http://www.php.net/manual/en/function.in-array.php#86695

This patch uses in_array() in STRICT mode and works well. Please review.

THANKS!

work perfect for me!

Good catch, wrapping the variable_get('menu_breadcrumb_ignored_menus', array()) in an array_filter will also work, since it removes NULL values from the array.

Trivialized parts of this patch.

However, I'm a bit afraid that users need to know that they have to change menu breadcrumb's settings in order to fix this annoying issue (when admin menu is installed).

Good point, Daniel.

I’m just thinking out loud here…

Maybe, by default, we're going to have to hard-code admin_menu into the exclude list. And/or have some way for other modules to automatically add their menus into the exclude list.

Maybe the admin_menu.module could form_alter() to check the "admin_menu" box in the "Ignored menus" list on admin/settings/menu_breadcrumb?

I like the idea of having admin_menu disabled by default.

If you used form_alter it wouldn't take effect until the first time you administered menu_breadbrumb. If you really wanted to do it you'd need to define a hook that let all modules declare if they would like to their to be part of menu_breadcrumb. Personally though I don't think this would make much sense though.

Why not just drop a drupal_set_message into the hook_install prompting the user to go to admin/settings/menu_breadcrumb and choose their menus there?

Well, let's do it then.

Worked for me!

Hmm, I could be missing something - but after applying the patch (updated to HEAD in order to do so), I now don't see Menu Breadcrumbs working at all.

The site I'm testing this on still has to be upgraded from D6.6 to 6.8. Could this be part of the problem? Otherwise I'm not sure what could be... I ran the updates, and flushed caches.

Does this make any sense at all? Thanks for any clarification

(I could optionally apply the patch to 6.x-1.1 if it would still apply... I'm just thinking using HEAD on a production site may not be a good idea anyway?)

Follow-up: I double-checked on a clean install of D6-8 and alternately tried patching against Menu Breadcrumb 6--1-1. The 'menu breadcrumb' functionality doesn't seem to be working at all with the patch applied. I tried with and without excluding the admin_menu in the patched MB configurations. As soon as I reverse the patch, menu breadcrumbs works again (of course in that case the issue is still there, conflicting with admin_menu).

Please let me know if you have it working, because it's possible I may be missing a step somewhere? Or if there's anything else I can do to help troubleshoot?

hum, wow, thats a lot of patches... as said before, the whole admin-menu (not the module, i mean the core-menu) is subject to exclude. For instance: i have garland as administration theme and a self-made as normal theme. when using menu_breadcrumb without excluding paths starting with "admin/*", i get garland theme when i try to configure blocks for the other theme.

furthermore, "user/*" becomes ugly, too. I suggest there should be something like block-visibility (having the opportunity to exclude/include certain paths).

FYI: The patch in #19 seems to do the trick for me. Is this going to be in the next version any time soon?

Thanks!

@scottrigby: The patch in #19 worked well for me when run against 6.x-1.x-dev (with a date of 2009-Feb-10). I did not try it against theofficial release.

Thanks for the work on this everyone.

Patch from #19 works perfectly for me on official realease 6.x-1.1.
Thank you for patch.

I'm a bit stunned that it's working for some people, and not for others.

Having tracked down the bug, it shouldn't be working for anyone!

We had:

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

which was removed through the patch. But the patch also uses the $use_menu variable (which was removed) in the added code here:

    while($mlid = db_result($result)) {
      $menu_link = menu_link_load($mlid);
      if (!in_array($menu_link['menu_name'], $ignored_menus, TRUE)) {
        menu_set_active_menu_name($use_menu);
        break;
      }
    }

So, of course, the easy fix:

    while($mlid = db_result($result)) {
      $menu_link = menu_link_load($mlid);
      if (!in_array($menu_link['menu_name'], $ignored_menus, TRUE)) {
        menu_set_active_menu_name($menu_link['menu_name']);
        break;
      }
    }

Works for me :)
I've included this change in the attached patch, if you'd like to confirm.

Cheers!

@webrascal: I can confirm that your patch also does the trick for me, just like the old one did. I didn't look into what's going on and why the first patch worked even though you say it shouldn't, so I don't have an opinion about that aspect at this point.

Thanks svdoord for the check.

I've changed the status to reviewed pending further reviews to make sure it's ok.

I'll pop back in a couple of weeks and close it if all seems well by then.

Thank you, svdoord, for the patch.

This evening, I ran into the same conflict between admin_menu and menu_breadcrumbs. Searched for a few minutes to find this issue queue.

I curled the patch, looked up how to patch a file in the fine handbooks that Drupal offers, and made my first module patch. Works perfectly. The extra menu exclusion options are helpful.

Drupal's strength is this engaged community.

I'm collecting all modules into a common code base to assemble a robust CMS for higher education use at:
http://web.fredonia.edu/d6

Thank you, all.

Hi jwoolson,

I'm glad it worked for you. I cannot take credit for this patch though, this one was made by others :-)

By the way: is there any word on when this will end up in a release?

Thanks!

subscribe

The patch posted with #27 worked perfectly for me. Thanks much for the fix!

+1 patch #27, it works for me.

I can also vouch for patch #27, it would be nice if it made it into the next version (if there ever is one).

+1 for #27 which appears to have fixed the issue for us also

@webrascal, are there other issues open which are blocking release of an updated menu_breadcrumb with this fix?

+1 for #27 seems to work just fine.

Yes this works as well for me! Thanks for the all the patch work everyone!

marked #417052: issue on /user page. related to admin_menu as a duplicate of this issue.

I applied the patch i #27 and it works just fine in my so far limited testing. Maybe it's time to commit this?

I doubt the maintainer needs any more people to weigh in on their approval of this patch. ;) At this point all we can do is either wait until gdevlugt has time to commit this, or until someone requests to co-maintain this module to take some of the load off for him.

#27 works for me.

I know very little php or mysql, and my question is this:

If I wanted to set the menu I want the breadcrumbs to use by hand, how do I do that?

I have no ethical problems with hand-hacking a module if someone could tell me where to put the name of the menu in men_breadcrumbs.module.

If it matters, the menu I want the breadcrumbs to use is Primary Menu.

For the record: I'm also using the Local Menu module, I hope that doesn't make things messier.

EDIT: I found http://drupal.org/node/364363

It still doesn't work, but that's probably a Local Menu issue then.

EDIT: not Local Menu, not Menu block, I'm at a loss.

My menu has a basic structure

Home
>Vocab
>>Term
>>>Nodetitles

Front page: I get the word "content" (not a link). Maybe this used to be a Vocab name, I'm not sure.
Vocab page: it works Home > Vocab, last one unlinked as it should be.
Term page I get "Home" (linked) instead of Home > Vocab > Term
Node page: it works Home > Vocabulary > Term > Nodetitle last one unlinked as it should be.

So right now I get a 50% hit/miss rate :(

Thanx webrascal!

Sorry about the delay guys, been a bit tied up with various projects. Glad that it works for those who posted.

I'll look into getting this rolled up as well as other issues as requested as soon as I can.

Patch from #27 worked for me.

#27 worked for me.

#27 working well at my site, thanks!

was surprised today to find this issue still open (and still collecting "+1 patch works" comments like a very sticky thing)

@webrascal, in #46 you sound like you're planning to roll a release of this, but I can't see that you're a maintainer of this module (yet!)

will the current maintainer be giving you access to release an update with this fix applied?

i'm wondering how i can best help get this one sorted, eg by offering to co-maintain or by working on other issues which might be blocking the release of a fix for this issue.

wrt the patch in #27.

I think that $form['menu_breadbrumb_ignored_menus'] in menu_breadcrumb_admin_settings_form() should filter out some options.

'admin_menu' and 'devel' are always ignored by menu_breadcrumb_init(), so they should be definitely removed from the list.

And as 'navigation' is the Drupal 6 default, it doesn't seem valid to offer to exclude those menu items, because if menu_breadcrumb_init() is unable to find an alternative, the navigation menu will be used. (edit: Ignore that. If a menu item appears both in Navigation and another menu as well, it's still useful to be able to exclude Navigation from the search list, to ensure the item in the other menu is used.)

So I think the $options array should perhaps be changed from drupal_map_assoc(menu_get_names()) to drupal_map_assoc(array_diff(menu_get_names(), array('admin_menu', 'devel')))

Patch works for me too.
(Though I wasn't able to reproduce on a test site...)

I have a problem with this module in a multilingual site, because two primary menus (for english and japanese) targeting the same content, then, the breadcumb always show a breadcumbs in japanese. I make a little changed in #27 code for fix it. I thinks it is a hack at the moment.

$primary = variable_get('menu_primary_links_source', 'primary-links');
  if (variable_get('menu_breadcrumb_determine_menu', 1)) {
    $ignored_menus = array_merge(variable_get('menu_breadbrumb_ignored_menus', array()), array('admin_menu', 'devel'));
    $menu_item = menu_get_item();
    $result = db_query("SELECT mlid FROM {menu_links} WHERE link_path = '%s'", $menu_item['href']);
    while($mlid = db_result($result)) {
      $menu_link = menu_link_load($mlid);
      if ($menu_link['menu_name'] == $primary) {
        menu_set_active_menu_name($menu_link['menu_name']);
        break;
      }
      // @see http://www.php.net/manual/en/function.in-array.php#86695
      if (!in_array($menu_link['menu_name'], $ignored_menus, TRUE) && !$seted) {
        menu_set_active_menu_name($menu_link['menu_name']);
        $seted = true;
      }
    }

  }

This code only break iteration if the link was found in primary links for the current language, otherwise find the first link occurrence in menu_links table.

My apologize for my english.

+1 for patch in #27, works for me.

-------------------
Armband kopen

Another +1 for #27 and to say thanks! I love Open Source!

It seems that maintainer of this module is not anymore active in the community. If there is somebody who is ready to take care maybe he can contact current maintainer or post issue to become maintainer.

#27 is actually slightly deficient, I've concluded.

The problem with having a black-list of menus to ignore is that new menus can be generated dynamically.

In particular, the book module creates a menu for every single book, to store its page hierarchy.

If you didn't want that to affect menu_breadcrumb, you would need to edit the menu_breadcrumb settings every time you created a book.

Here's a new patch (against 6.x-1.x-dev, 2009-Feb-11) which enable you to choose whether the selected list of menus should act as a black list or a white list.

Defaults match the default behaviour of #27, however I've changed the 'ignore list' persistent variable name, so you'll want to reconfigure the module if you had already customised the settings with an ignore list.

To ensure that menu_breadcrumb always tries to use one specific menu, just select that menu (only) and choose the white-list option.

@anantagati - I've opened an issue at http://drupal.org/node/539240 regarding this module's potentially abandoned status.

I'm sure we'd all welcome the return of the module maintainer, but failing that anyone who is keen to maintain or co-maintain this module is welcome to throw their hat in the ring via that issue.

Attaching pre-rolled version of version patched with jweowu's patch from #58 in the hope that it will encourage others to test until we have a module maintainer.

Patch #58 works for me.

Sadly with:

Modules
administration menu 6.x-1.5
menu attributes 6.x-1.4
devel 6.x-1.16
menu breadcrumb 6.x-1.x-dev patch

Going to add user displays a funny breadcrumb..

Home › 1 / 1 Current anonymous / authenticated users › 1 / 1 <img src="/sites/all/modules/admin_menu/images/icon_users.png" width="16" height="15" alt="Current anonymous / authenticated users" title="Current anonymous / authenticated users" />

Can anyone else confirm this?

As the thread discusses I have the patched built and have both admin_menu and devel set in the black list.

thoughts?

Hi emptyvoid,

I set up a new Drupal 6.13 site (default Garland theme) with the same contrib modules & versions you've listed, and using the copy of menu_breadcrumb which xurizaemon attached in #60. I enabled all of them except for menu attributes at first (in case that was a conflict), and then tried again with menu attributes enabled, but didn't see any issues in either case when visiting admin/user/user/create.

Perhaps another module is conflicting with the breadcrumb code I will attempt to apply the patched version of the module again..

When I reverted back to the initial 1.1 release the issue went away.

I will investigate further and post my results should I figure out what is causing the conflict.

Thanks,

Patch #58 works great on the final stable 1.1 release. Thanks!

"Patch #58 works great on the final stable 1.1 release."

Same here. Thanks!

"Patch #58 works great on the final stable 1.1 release."

Same here too. Thanks!

The patch at #58 solves the issue for me as well.

Please please please... Let aaron's post be the LAST 'patch works for me' post. The problem isn't that the maintainer needs more +1's for the patch, the problem is that the maintainer is either too busy on other more important projects and will get to it when they can, or that this module is abandoned. I would suggest someone open a new issue to request for CVS access. Until then, please don't post on this thread unless you've found a bug related to the patches above, or are submitting a different patch.

I already would have abused my privileges to commit this patch, but I did not do so, because I do not use this module and the proposed code to fix this issue looks a bit strange to me (apart from the coding style issues).

To move on, someone of you (who uses this module) should apply for CVS access to become a co-maintainer (to do this: create an issue in this queue and contact the maintainer in parallel).

@sun, @q0rban, thanks for the input. http://drupal.org/node/539240 is an issue which attempts to follow the instructions at http://drupal.org/node/251466 for obtaining CVS access to an apparently abandoned project. That page says to wait two weeks before moving the issue to the webmasters queue for them to reassign the project, so there are (only) a few days to go.

aaron and xurizaemon (me) are prepared to take on co-maintainership of this module, would be happy to hear from others who are keen (incl. current maintainer of course).

Updated patch - reformatted fields to make UI a bit clearer (I hope), swapped use of ( a && b or c && d ) for more common ( ( a && b ) || ( c && d ) ) to clarify precedence.

Last patch worksforme. Subscribing...

Regard the possibility of a new maintainer... I've also opened #560464: Consider merging with menutrails

Thanks!

committed in http://drupal.org/cvs?commit=257682

thanks all for your patches and patience! as of today, this module has two new maintainers (aaron and xurizaemon)

jweowu, I am happy enough with the usage of the 6.x-1.x-dev release which contains this fix (or, lack of complaints from the sites using that release!) to push out an official update now. But I see that you've opened #556552: Menu weights, black/white list, and memory of menu selection with some related issues spun off from there.

Are there any issues raised in that queue (with regard to weighting the menus) which would suggest that the blacklist/whitelist feature shouldn't make it into an official release? Happy to hold off if you think a better fix is on the cards.

#595282: Menu weights is a factor, because that changes the structure of the 'menu_breadcrumb_menus' variable.

If you make an official release, we'll need to add a hook_update_N() to convert existing variables to the new format.

If you think the menu weights will take a while to evaluate, then it might be worth releasing what's committed now (which does resolve a notable problem, after all), and writing that update code.

Any chance of a release with this fix?

It's been several months since this was committed, yet obviously it's still showing up as a problem when downloading the current release.

This patch was committed to -dev, so installing that build is an option.

I can't speak for the maintainers, but as there hasn't been a stable release with this so far, I suspect that it may be passed over in favour of #595282: Menu weights.

The latter is now looking for people to review it, so it might speed things along if people could try that out?

Yes, I should have said as much earlier in this thread. I'll be reviewing #595282 in the next day or two, and probably release a stable with that if it's good to go. Testing that patch and posting feedback will help this process, so if you'd like to help get the new release out, please do so!

subscribe

Why would you subscribe to a closed issue that was deprecated in favour of a different (and also closed!) issue?

The fix for this has been committed and is available in the latest -dev build. You would do better to try that, and provide feedback.

Had the issue and has gone away with latest dev version. Will provide further feedback if anything (bad) arises.

this issue is now gone with the latest non-dev version. 6x-1.2

Thanks plasticlax. This issue was resolved by #595282: Menu weights and that patch was included in 6.x-1.2.

I didn't see this issue queue before but could this be related to this open issue http://drupal.org/node/1414900

EDIT:

Sorry, but I realized that my breadcrumb problem is caused by conflict with menu trails. Just turned off that module.