Thanks so much for getting this module started, and addressing some of the shortcoming of the alternative "jump" menu modules out there.

This module seems to suffer from a problem that I've found to be common with quite a few "menu rendering" modules out there. Namely, it will not properly handle menus that use the same href more than once. A common scenario where this will cause problems is when admins match the first child menu item to its parent (to ensure that the branch root is always accessible when browsing just one branch -- e.g. with something like "menu block").

This problem is commonly linked to the fact that the module will abstract the menu as an array, and will key the array by the href for a menu item. If the href is repeated elsewhere in the menu, this will overwrite the first entry, possibly breaking any hierarchical structures. Of course, this module has a very good reason to abstract a menu as an array with href keys, as this is is what the ctools 'jump-menu' feature requires. Regardless, it poses a problem that I think could be somewhat common.

I'm working on a potential solution to get around this and hope to post a patch soon.

CommentFileSizeAuthor
#2 jump_menu-allow_multiple_href_in_menu-1510294-2.patch1.54 KBrjacobs
#1 jump-menu-allow_multiple_href_in_menu-1510294-1.patch1.54 KBrjacobs

Comments

rjacobs’s picture

rjacobs commented
Status: Active » Needs review
StatusFileSize
new jump-menu-allow_multiple_href_in_menu-1510294-1.patch1.54 KB

The attached patch checks each branch to see if a child has a href that matches a parent. If it finds one is then sets the parent to be a simple optgroup. This is akin to the support that already exists for special_menu_items.

rjacobs’s picture

rjacobs commented
StatusFileSize
new jump_menu-allow_multiple_href_in_menu-1510294-2.patch1.54 KB

Sorry, the patch in #1 had a typo in an if statement. New patch attached.

doublejosh’s picture

doublejosh commented

Great. I was worried you end up in the pitfall that Form API doesn't allow you to have duplicate options in a select.
Ran into that and had to add an asterisk which was stripped off within the submit handler, hassle. #1048134: Repeat "most used" country list first with separator.

I have a few other items stacked up, so this is likely to make the next release relatively soon.

doublejosh’s picture

doublejosh commented

Just tested this out id D7 only to realize you're dealing with a D6 problem. This bug doesn't appear to exist in D7, or perhaps only happens in certain cases. BTW: Only a 25% of the sites using this module are D6 and it's staying fairly flat. I see that you're patch it also about making it an optgroup, confirming that wasn't the whole point, but rather the fix for a bug.

Perhaps you can help me find an example if it's really a problem in both versions?

In order to see it I added a "Create" menu item within "Navigation" with the path of "node/add" and placed it above "Add Content" link, then moved the content type add menu items in line with Add Content. Menu become like so...

Create
- Add Content
- Article
- Page
- Test Type

... and this worked just fine. Is that an indicator that it's a version issue?

doublejosh’s picture

doublejosh commented
Status: Needs review » Postponed (maintainer needs more info)
rjacobs’s picture

rjacobs commented

Hi Josh,

I'm thinking this issue is specific to D6. Was the case where you tried to replicate it under D7?

In the D7 version it looks like an item is added to the $target array (that's later passed to ctools to render the menu) with:

// Create a normal option.
// $t[ url($item['link']['href']) ] = t($title); // Prior to special render path.
$t[] = array(
  'value' => url($item['link']['href']),
  'title' => t($title),
  '#attributes' => array('class' => 'd-' . $d['current']),
);

But in the D6 version it's just:

// Create a normal option.
$t[ url($item['link']['href']) ] = t($title);

So under D7 the targets are keyed via a generic array index, but in D6 they are keyed by the menu item's href value. So in D6 you can't have duplicate hrefs in the same jump menu, but in D7 you can.

I'm guessing the structure of the $targets argument has changed between D6 and D7 in the following call?:

drupal_get_form('ctools_jump_menu', $targets, $options);
rjacobs’s picture

rjacobs commented
Status: Postponed (maintainer needs more info) » Needs review

Just moving the status (assuming my reply in #6 was sufficient).

doublejosh’s picture

doublejosh commented
Status: Needs review » Needs work

Yeah, "tested this out id D7 only to realize you're dealing with a D6 problem."

Thank you for tracking the cause down.
The code was changed to allow for depth classes, and I suppose accidentally made it allow for duplicate hrefs. I should probably just backport that whole feature.

rjacobs’s picture

rjacobs commented

Ah, yes. If it is possible to key the $targets array by something other than the href then this would indeed work, and would be a more robust solution than my workaround. If that's possible under D6 it would be great to see it implemented!

doublejosh’s picture

doublejosh commented

BTW: This isn't directly possible in the D6 or D7 core form API. It keys options on value. I had to create an alternate rendering path, which is what I'll need to backport and adapt for D6.

doublejosh’s picture

doublejosh commented
Version: 6.x-1.2 » 7.x-1.4
Status: Needs work » Closed (fixed)

Won't tackle this for D6. Focusing on getting this upgraded for D8.

BTW: This feature ended up causing all kinds of issues because it took over option output...
#1512550: Better Jump Menus module interfering with all Drupal forms
#1824804: Jump menu breaks block save on admin/structure/block/list page [SOLVED]