Problem/Motivation

  • Custom (non-core) menus do not receive an active trail.
  • Custom menu items consequently never expand properly to reveal child pages, which is a regression from D6.
  • Custom menus also cannot provide breadcrumbs, despite that D7 expands this functionality to menus other than the primary navigation. (While this was by design according to DamZ and pwolanin, the previous point is a clear regression.)

Steps to reproduce

  1. Create a custom menu
  2. Create two nodes, Foo and Bar, and add them to the custom menu with Bar as a child of Foo.
  3. Put the menu block for the custom menu in a sidebar.
  4. Click on Foo. Bar is not displayed.
  5. Visit Bar. The menu item for Foo is not active or expanded.
  • Menu structure

    custom_menu.png

  • Visiting Foo

    custom_menu_on_foo.png

  • Visiting Bar:

    custom_menu_on_bar.png

Proposed resolution

  • Ensure that custom menus are included in the active menus variable so that their items can be identified as part of the active trail.
  • Add an optional argument to menu_link_get_preferred() to allow the active trail to be identified per menu.
  • Statically cache the matched links for the active trail per menu in menu_link_get_preferred(), with the highest-priority match stored in a special __preferred__ key.

Patch in #205 implements this solution. Screenshots with the patch applied follow.

  • Visiting Foo

    foo_after_patch.png

  • Visiting Bar:

    bar_after_patch.png

Remaining tasks

Replace the __preferred__ key with a constant (see #236 #237).

  • The patch includes an automated test that verifies that the custom menu expands properly.
  • The patch's performance implications have been evaluated. It adds the following additional queries (see #180):
  • The changed query in menu_link_get_preferred() (with a condition removed) still properly uses an available index and does not do any table scans (see #187):
    +----+-------------+-------+--------+---------------+-----------+---------+-------------------+------+-------------+
    | id | select_type | table | type   | possible_keys | key       | key_len | ref               | rows | Extra       |
    +----+-------------+-------+--------+---------------+-----------+---------+-------------------+------+-------------+
    |  1 | SIMPLE      | ml    | ref    | path_menu     | path_menu | 386     | const             |    1 | Using where |
    |  1 | SIMPLE      | m     | eq_ref | PRIMARY       | PRIMARY   | 767     | d8.ml.router_path |    1 |             |
    +----+-------------+-------+--------+---------------+-----------+---------+-------------------+------+-------------+
    2 rows in set (0.00 sec)
  • A list of all queries that still use a condition on {menu_links}.menu_name is in #198, indicating that this index should not be removed even though it is no longer used by the particular function in this patch.

Related issue

Notes for contributed modules

User interface changes

  • Custom menus will now have active trails.
  • Items in custom menus will expand properly to reveal child pages.
  • As a side effect, custom menus can now provide breadcrumbs.

API changes

  • Optional argument $selected_menu is added to menu_link_get_preferred().

Original report by becw

Custom menus will never appear in the active menu trail--this means that custom menus will never be used to generate breadcrumbs, and menu blocks for custom menus will not expand as you navigate into them. This applies to any menu that is not in menu_list_system_menus() (navigation, management, user-menu, and main-menu).

To duplicate:

  1. Go to admin/structure/menu and add a new menu.
  2. Add the new menu block to the sidebar so you can see the menu's active state.
  3. Create a node and place it in the menu, using the 'menu settings' on the node form.
  4. Create a second node and place it in the menu, with the first node as its parent item.
  5. View the second node. Note that the menu is not expanded. If you dig in, you'll find that the active menu trail goes through the default 'navigation' menu (see menu_get_active_trail()) and in fact the custom menu isn't even considered an 'active' menu according to menu_get_active_menu_names().

menu_get_active_menu_names() seems to actually be the culprit here. To determine the candidate menus, it uses a Drupal variable, 'menu_default_active_menus', with a default value of menu_list_system_menus() (a hard-coded list of core menus). The 'menu_default_active_menus' variable is never set. It is not referenced anywhere else in Drupal core.

If Drupal is going to keep track of menus to build the active trail from in 'menu_default_active_menus', then it needs to get updated whenever menus are added or removed, so that user-created menus are available. Patch attached.

CommentFileSizeAuthor
#337 Superfish6_2012.JPG93.37 KBCowTownFarmBoy
#337 Superfish26_2012.JPG92.64 KBCowTownFarmBoy
#337 Superfish36_2012.JPG65.3 KBCowTownFarmBoy
#298 menu.zip51.02 KBSummit
#286 print-2.png467.16 KBlinno
#286 print-3.png567.89 KBlinno
#286 print-4.png556.25 KBlinno
#273 menu.png3.84 KBxjm
#262 d7-942782-262.patch11.3 KBxjm
#262 interdiff-248-262.txt1.54 KBxjm
#257 d7-942782-257.patch10.21 KBxjm
#257 interdiff.txt452 bytesxjm
#248 custom-active-trail-942782-248.patch10.26 KBxjm
#248 interdiff-205-248.txt1.78 KBxjm
#243 custom-active-trail-942782-242.patch10.21 KBxjm
#243 interdiff-205-242.txt1.9 KBxjm
#241 custom-active-trail-942782-241.patch10.21 KBxjm
#241 interdiff-205-241.txt1.91 KBxjm
#234 custom_menu.png3.72 KBxjm
#234 custom_menu_on_foo.png2.92 KBxjm
#234 custom_menu_on_bar.png3.02 KBxjm
#234 foo_after_patch.png3.3 KBxjm
#234 bar_after_patch.png3.37 KBxjm
#212 per-menu-preferred-links-942782-212-D7.patch10.62 KBTor Arne Thune
#205 per-menu-preferred-links-942782-205-test-only.patch2.46 KBTor Arne Thune
#205 per-menu-preferred-links-942782-205.patch9.58 KBTor Arne Thune
#196 expandedCustomMenu.png5.29 KBTor Arne Thune
#185 per_menu_preferred_links-942782-186.patch7.59 KBpillarsdotnet
#184 per_menu_preferred_links-942782-185-incomplete.patch6.93 KBpillarsdotnet
#184 per_menu_preferred_links-942782-includes--menu.inc_.rej_.txt569 bytespillarsdotnet
#172 per_menu_preferred_links-942782-172-d7.patch8.73 KBpillarsdotnet
#171 per_menu_preferred_links-942782-171-d7.patch0 bytespillarsdotnet
#170 custom_menus_receive_active_trail-942782-170-d7.patch8.58 KBcolan
#167 per_menu_preferred_links-942782-167-d7.patch8.79 KBmelon
#160 per_menu_preferred_links-942782-160-d7.patch8.79 KBpillarsdotnet
#160 per_menu_preferred_links-942782-160.patch7.52 KBpillarsdotnet
#146 per_menu_preferred_links-942782-146-d7.patch8.71 KBdstol
#145 per_menu_preferred_links-942782-145-d7.patch8.71 KBpillarsdotnet
#133 per_menu_preferred_links-942782-133-d7.patch8.1 KBJames Andres
#129 per_menu_preferred_links-942782-129.patch6.94 KBjn2
#101 per_menu_preferred_links-942782-101.patch8.73 KBpillarsdotnet
#101 per_menu_preferred_links-942782-101-d7.patch8.77 KBpillarsdotnet
#88 942782-88-per-menu-preferred-link.patch8.11 KBjrchamp
#70 942782-70-per-menu-preferred-link.patch8.02 KBjrchamp
#43 942782-43-per-menu-preferred-link.patch8.22 KBJohnAlbin
#42 942782-41-custom-menus-expanding.patch5.55 KBtimhilliard
#41 942782-custom-menus-expanding.patch5.56 KBtimhilliard
#39 942782-39-per-menu-preferred-link.patch7.45 KBJohnAlbin
#22 942782-per-menu-preferred-link.patch3.06 KBDamien Tournoud
#17 custom-menu-d7-regression.png48.76 KBJohnAlbin
#14 Bild 13.png27.67 KBcriz
#6 942782-6-custom_mens.patch2.33 KBbecw
#4 942782-4-custom_menus.patch2.66 KBbecw
#2 942782-3-custom_menus.patch1.41 KBbecw
custom_menus.patch1.41 KBbecw
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, custom_menus.patch, failed testing.

becw’s picture

Status: Needs work » Needs review
FileSize
1.41 KB

Fixed a typo.

JohnAlbin’s picture

Status: Needs review » Needs work

Nice find, Bec! This is a huge problem. I spent a few hours troubleshooting this same problem and didn't find the solution.

This patch is almost what we need to do. We need to replace menu_list_system_menus() with menu_get_menus().

Bec and I talked at length about the odd-looking menu_default_active_menus variable before I finally remembered the use-case. Contrib needs to be able to set the order of the menus, so that it can specify which menus should be checked first for active trails. So that variable absolutely needs to documented.

becw’s picture

Status: Needs work » Needs review
FileSize
2.66 KB

Ok, I've re-rolled the patch:
* now uses menu_get_menus() in place of menu_list_system_menus()
* added a comment documenting the 'menu_default_active_menus' variable
* only set the 'menu_default_active_menus' variable on menu creation (not update) or set on menu deletion only when the menu is present in 'menu_default_active_menus'.

Status: Needs review » Needs work

The last submitted patch, 942782-4-custom_menus.patch, failed testing.

becw’s picture

Status: Needs work » Needs review
FileSize
2.33 KB

Using a function from menu.module in menu.inc causes problems (thanks, testing!). I've re-rolled the patch so that it doesn't do that.

criz’s picture

Priority: Critical » Major

Great, stumbled over the same issue: http://drupal.org/node/950034

Patch from #6 fixes it and works for me. Documentation of this variable is important for sure.

criz’s picture

Priority: Major » Critical
Damien Tournoud’s picture

Category: bug » feature

Not critical and not a bug.

In Drupal 6, only the navigation menu generated an active trail.

In Drupal 7, this was extended to all the menu specified in the 'menu_default_active_menus' variable. There is no UI for that, and the custom menus are not automatically added there.

Adding an UI to control this variable or automatically adding custom menus there is a feature request.

criz’s picture

Okay, but if this (automatically adding custom menus to variable 'menu_default_active_menus') is not supported then the UI in menu settings (admin/structure/menu/settings) is misleading IMHO. At least the helptext would have to be more precise.
Helptext says:

An advanced option allows you to use the same source for both Main links (currently ...) and Secondary links: if your source menu has two levels of hierarchy, the top level menu links will appear in the Main links, and the children of the active link will appear in the Secondary links.

But if people do this with a custom menu it would not work! They won't know why and think about a bug (me too).

Damien Tournoud’s picture

#950034: Using the same source for main/secondary with a custom menu doesn't work doesn't feel related. In Drupal 6 no menu trail was generated for non-navigation menu, and the hierarchical primary/secondary menu was working. Something changed somewhere else.

criz’s picture

So #950034: Using the same source for main/secondary with a custom menu doesn't work should be reopened? Please reopen if you think so.

Patch from #6 seems to fix both issues btw. No idea if in the correct way...

becw’s picture

Category: feature » bug

re #9: When you create a custom menu in Drupal 6, add several nested items to it, and add the corresponding menu block to some region, the menu will expand as you navigate into it (here's a screenie: http://skitch.com/becw/dh6au ). Drupal recognizes when the page you're currently on is in the custom menu, and can tell you the parent and child menu items from that menu. In Drupal 7, this does not happen. This is a bug--not critical, but definitely not the intended behavior of custom menus. Maybe I'm misusing "active trail"?

criz’s picture

FileSize
27.67 KB

This bug seems not to appear with alpha 7. See http://drupal.org/node/950034#comment-3618858

sun’s picture

Status: Needs review » Closed (duplicate)
JohnAlbin’s picture

Priority: Major » Critical
Status: Closed (duplicate) » Needs work

If #950034: Using the same source for main/secondary with a custom menu doesn't work's patch fixes this issue too, fantastic, but that is a giant patch and I don't easily see how it fixes this issue.

This is a tiny, isolated issue that can be solved easily.

In Drupal 6, only the navigation menu generated an active trail.

Actually, that's not accurate. In Drupal 6, only the navigation menu generated a breadcrumb. All core and custom menu trees properly received an active trail to the active item.

That makes this a critical regression. :-(

JohnAlbin’s picture

Title: Custom menus never appear in active trail » Custom menus never receive an active trail
FileSize
48.76 KB

Per chx's request, I've reproduced the problem and taken a screenshot. This is a user-facing problem.

In the screenshot, I created a simple tree in the Main menu:

  • Front
  • About
    • Contact

I then created a custom menu and created the exact same simple tree. Then I placed core's menu blocks for both menus in a sidebar and visited the "About" page.

Both menus should be expanded exactly the same. Which is the way Drupal 6 works.

mfer’s picture

subscribing.

webchick’s picture

Priority: Critical » Major

I can see this is nasty if you run into it, but I don't see how it is critical.

sun’s picture

Both menus should be expanded exactly the same.

I'm not sure whether this expectation is entirely correct. As Damien already explained:

In Drupal 7, this was extended to all the menu specified in the 'menu_default_active_menus' variable. There is no UI for that, and the custom menus are not automatically added there.

In addition, we have menu_link_get_preferred() now, which is supposed to find and determine the active link for the current page, within active menus.

Given all that, I was under the impression that the screenshot in #17 for D7 basically is what we expect in D7 - only one menu (in the active menus) is expanded. The remaining problem is that custom menus will never be expanded, because they are never added to the list of active menus. But even if custom menus were added to the active menus, if there are identical link structures in two menus (as in the given example), then the actual behavior of the active trail, breadcrumbs, and expanded menus depends on whether the custom menu has been appended or prepended to the list of active menus, so the weight/order of active menus is important.

That's why I marked this as duplicate of #950034: Using the same source for main/secondary with a custom menu doesn't work. I may be wrong though, so feel free to correct me.

mfer’s picture

There error goes beyond what JohnAlbin shows to encompass even more. Say I create a menu foo with an item bar. Under bar I put the item baz. Both bar and baz point to internal pages. When I visit either bar or baz the active trail is not working. The active item will work but not the active trail.

Basically, active trails only work for menus defined in menu_default_active_menus. If that is not set than only the menus defined in menu_list_system_menus.

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
3.06 KB

So actually, the real issue / discrepancy between Drupal 6 and Drupal 7 is that we had an active trail per menu in Drupal 6, while we only have one active trail in Drupal 7.

We can easily change that. Gist of a patch here.

sun’s picture

+++ includes/menu.inc
@@ -1197,7 +1197,7 @@ function menu_tree_page_data($menu_name, $max_depth = NULL, $only_active_trail =
           // Find a menu link corresponding to the current path.
-          if ($active_link = menu_link_get_preferred()) {
+          if ($active_link = menu_link_get_preferred(NULL, $menu_name)) {
             // The active link may only be taken into account to build the
             // active trail, if it resides in the requested menu. Otherwise,
             // we'd needlessly re-run _menu_build_tree() queries for every menu

Doesn't this contradict the entire idea of menu_link_get_preferred()?

It's supposed to find the preferred menu link for the current page, taking all active menus into account.

So this change _always_ limits that search to the menu that is currently built, which means that the returned preferred link may not be the real, globally preferred link.

Powered by Dreditor.

Damien Tournoud’s picture

So this change _always_ limits that search to the menu that is currently built, which means that the returned preferred link may not be the real, globally preferred link.

No, but does it really matters? The global, unique, preferred link is still used to build the breadcrumb.

Status: Needs review » Needs work

The last submitted patch, 942782-per-menu-preferred-link.patch, failed testing.

mfer’s picture

Maybe I'm missing something... what was wrong with Becs patch in #6? It corrects the problem by adding all the menus to the same store that the core ones do for the active trail. That means my custom menu is equal in active trails to the main menu.

Damien Tournoud’s picture

@mfer: the list of menus that get to participate in the breadcrumb is an ordered one. The fact that not all menus participate in the breadcrumb is a conscious design decision. See my #9 for context.

Moreover, Bec patch in #6 only solves part of the problem. It will not fix John's example in #17 for example.

JohnAlbin’s picture

I think my screenshot and description on how to easily see the bug has confused people. This bug has little to do with menu links that exist in multiple menus. That was just a convenient way to show that the default menus work and custom menus don't work.

Here's another way to reproduce the bug (this is just a detailed version of mfer's above):

  1. Create a custom menu
  2. Create 2 new nodes and put them in the custom menu with one as a child link of the other.
  3. Put the menu block of the custom menu in a sidebar.
  4. Try to navigate to the child menu item by clicking on the parent menu item. Guess what? You can't! The parent menu item will show a triangle indicating that it has children links, but the menu will never expand to show those links.

Why? Because an active trail is never added to custom menus.

Also, if you use a custom menu as the source for both the main and secondary links in the menu settings form, the secondary links will never appear. Same reason. (@sun that's why the bug underlying in #950034: Using the same source for main/secondary with a custom menu doesn't work is a dupe of this one.)

@webchick From the Priority description page (emphasis mine):

Critical bugs either render a system unusable (not being able to create content or upgrade between versions, blocks not displaying, and the like), or expose security vulnerabilities. These bugs are to be fixed immediately.

In the example above, I used core's menu blocks. But if I were to instead use the Menu block module and configure it to show 2nd or lower level links of a custom menu, those blocks will never appear. i.e. critically broken.

However, I'm not about to get in a priority-switching argument with a core committer. Especially when we don't have an RTBC patch. :-D Bec's patch works great, but doesn't deal with the upgrade path from D6. D6-created custom menus will still be broken in D7.

But even if custom menus were added to the active menus, if there are identical link structures in two menus (as in the given example), then the actual behavior of the active trail, breadcrumbs, and expanded menus depends on whether the custom menu has been appended or prepended to the list of active menus, so the weight/order of active menus is important.

Yes, that's what the code comments in menu_link_get_preferred() already say. “Retrieve a list of menu names, ordered by preference.” Drupal core provides no UI to re-arrange the returned array of menu_get_active_menu_names(), but we left that for contrib, right? (or possibly with the feature-request-ish patch in #950034: Using the same source for main/secondary with a custom menu doesn't work) Bec's patch just appends custom menus to the end of the array, which is the sensible solution. (That's how the UI in D6's menu_breadcrumb module worked, btw.)

Moreover, Bec patch in #6 only solves part of the problem. It will not fix John's example in #17 for example.

Ugh. Actually, you're right. But I consider #17 an edge case.

I think we need to combine Bec's patch with an approach similar to Damien's. And then add an upgrade path for D6 (and 7.0 sites).

wickedskaman’s picture

I have a the same issue with a different use case.

1. Activate system contact form
2. Create menu item in main_menu through GUI pointing to /contact
3. When viewing the contact page the contact menu item created has no active-trail

pixelwhip’s picture

The ability to style the active trail of custom menus would greatly improve their usability for end users. Subscribing.

JohnAlbin’s picture

wickedskaman's point is also correct.

Because many modules (like Contact and Search) put their own menu links in the “Navigation” menu, and since the Navigation and Management and User menus all appear before the Main menu in the list of preferred menus, if a user just adds a new link to the Main menu for an existing module-provided link, the main menu will not expand or get an active trail when on that path.

amanaplan’s picture

subscribing

amanaplan’s picture

usaussie’s picture

subscribing.

timhilliard’s picture

subscribing

anavarre’s picture

Subscribing

BenK’s picture

Subscribing

geerlingguy’s picture

Subscribe.

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
7.45 KB

Ok. This patch combines Bec's patch in #6 with Damien's patch in #22. I also optimized menu_link_get_preferred() because in Damien's version it did a db query per menu tree displayed on the page. This patch does one db query per page to find all the paths for all menus.

Also, I added an upgrade path.

Status: Needs review » Needs work

The last submitted patch, 942782-39-per-menu-preferred-link.patch, failed testing.

timhilliard’s picture

Hi guys, I hope I'm not throwing a cat amongst the pigeons here but I have re-rolled the patch that I created for the duplicate issue #968878: Custom menus do not expand past top level menu items in blocks using the latest dev version. The reason I think this bug is occurring is because of the following piece of code in menu.inc

$parents = $active_trail;

This piece of code can be found in the function menu_tree_page_data. The reason I think this is wrong is that it is setting the active trail to the parents when parents should really be all the parents for a path and not just from the active trail. My patch allows the active trail to function as it did previously and generate the breadcrumbs as normal whilst allowing all the menus to expand properly if they are not in the active trail.

timhilliard’s picture

whoops, error with last patch.

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
8.22 KB

The reason I think this is wrong is that it is setting the active trail to the parents when parents should really be all the parents for a path and not just from the active trail. My patch allows the active trail to function as it did previously and generate the breadcrumbs as normal whilst allowing all the menus to expand properly if they are not in the active trail.

I think there's a problem of terminology here. Tim, you are using “active trail” to mean the the preferred active trail that is used for breadcrumbs. But in Drupal 6, “active trail” just meant the trail to the active item in a particular menu. Each menu got its own active trail. And one of those active trails was used as the breadcrumb.

The problem isn't that menu_tree_page_data() is using the active trail for the parents, its that menu_tree_page_data() is fed the wrong active trail for the menu it is supposed to build.

I'm with Damien here. We need to alter menu_link_get_preferred() to give it an extra parameter of $menu_name.

Doesn't this contradict the entire idea of menu_link_get_preferred()?

It's supposed to find the preferred menu link for the current page, taking all active menus into account.

So this change _always_ limits that search to the menu that is currently built, which means that the returned preferred link may not be the real, globally preferred link.

I don't believe it does contradict the purpose of the function. If you don't give menu_link_get_preferred() any params, it will return the active item at the end of the active trail of the preferred menu. If you instead call menu_link_get_preferred(NULL, 'menu-name'), it will return the active item at the end of the active trail of the requested menu.

Here's another go at the patch in #39. Needs simpletests added. NR for the testbot.

timhilliard’s picture

@JohnAlbin you're totally right, I was having trouble interpreting the terminology correctly. Thanks for the clarification. I would also say fair enough in regards to the usage of menu_link_get_preferred, however there is one case that would have a problem with this implementation and that is when there is two of the same link in the same menu. This will mean that when getting menu_link_get_preferred it will return only the active link from the active trail and not all of the active links in the menu. Although this is not a common situation I think it should still probably be catered for.

JohnAlbin’s picture

This will mean that when getting menu_link_get_preferred it will return only the active link from the active trail and not all of the active links in the menu. Although this is not a common situation I think it should still probably be catered for.

That's totally true. But Drupal 6 (and earlier) didn't do that either. Which means catering for that is a feature request for Drupal 8. :-\

Hmm… I wonder if I could write a contrib module to do that. I bet I could. I seem to have plenty of menu_* contrib modules. ;-)

JohnAlbin’s picture

The simpletests for this will only really be possible once the breadcrumb test refactoring is committed over in #520106: Allow setting the active menu trail for dynamically-generated menu paths.. In the meantime, the rest of the patch could use some eyeball reviews.

W32’s picture

Subscribing

W32’s picture

Standard "Primary link" and "Secondary link" menu blocks are not working too, after update site from D6 to D7.

Damien Tournoud’s picture

I am still -1 on the change in modules/menu/menu.module.

The new version of menu_get_preferred_link() looks ok to me, but it could use further review / clean up. For example this feels insufficient and unnecessary:

+    $preferred_links[$path]['__preferred__'] = FALSE;
+    foreach ($menu_names as $menu) {
+      $preferred_links[$path][$menu] = FALSE;
+    }

We should rather change the return value to return isset($preferred_links[$path][$menu_name]) ? $preferred_links[$path][$menu_name] : FALSE;.

sun’s picture

Status: Needs review » Needs work
killer_tilapia’s picture

Subscribing

jn2’s picture

subscribing

Shmee’s picture

subscribing

Patch in #43 works for me. And everything looks good.

Shmee’s picture

Anybody got any new ideas on this? I need this fixed before I go live on my new site, and I am hoping to do that before the end of the month. Like I said in the previous post the patch in #43 looks pretty good to me, but if it is not the right approach then we should look in to it more. Anybody got an idea on what the correct method would be?

becw’s picture

@Shmee -- For now, JohnAlbin has incorporated this fix into the Menu Block module, so installing Menu Block is a supported workaround for this issue. He wrote about it here: http://palantir.net/blog/use-menus-drupal-7-you-need-menu-block-module

Shmee’s picture

Thanks a lot man! This worked great.

NewZeal’s picture

Version: 7.x-dev » 7.0

This problem occurs in the 7.0 release as well. To illustrate, when creating a custom menu link the tickbox "Show as expanded" becomes redundant because lower menu items will only display when this box is checked. To further complicate matters, when you check all menu items as expanded for a menu tree, then you do not get any active trail.

Alan D.’s picture

Version: 7.0 » 7.x-dev

Fixes go against dev => 7.1 (hopefully)

prezidentchimp’s picture

Count me in as someone else who is looking forward to this menu problem being taken care of.

dropcube’s picture

Subscribing.

jlab’s picture

Subscribing. This is making it difficult to start moving our existing sites over to D7 since most of our sites uses the source for secondary links as the primary links. With the primary links displayed at the top.

Will try and use the menu_block as a work around for now.

Monzer Emam’s picture

Subscribing

wbrain’s picture

subscribing

anydigital’s picture

Status: Needs work » Needs review
pthite’s picture

subscribing

anydigital’s picture

subscribing

jrchamp’s picture

It looks like DamZ's comment #49 will help cleanup the patch on #43. The current method of filling the array with FALSE seems to make the patch a lot bigger (and more confusing) than it needs to be.

Dmitriy.trt’s picture

subscribing

carn1x’s picture

subscribing

jrchamp’s picture

This patch attempts to include the cleanup referenced in #49. I am unable to comment on the changes in the other files, so they are included without modification. Please review.

markabur’s picture

I just tested #70 and it fixes the problem I was having, which is similar to the screenshot in #17.

Main menu:

About Us
- Subpage
Portfolio
- Taxonomy term A
- Taxonomy term B

Portfolio menu:

Taxonomy term A
- Taxonomy subterm a1
- Taxonomy subterm a2
Taxonomy term B
- Taxonomy subterm b1
- Taxonomy subterm b2

Before the patch, visiting the Taxonomy term A page didn't cause its menu item to expand, unless I deleted the Taxonomy term A link from the Main menu. After the patch I can keep the link in Main menu and the Portfolio menu expands properly.

paragkenia’s picture

Subscribe

sbatyrov’s picture

custom_menus.patch queued for re-testing.

torrance123’s picture

custom_menus.patch queued for re-testing.

torrance123’s picture

The patch in comment 70 works for me with no problems. Thanks for your work!

aspilicious’s picture

Subscribe, got bitten by tha bug :)

bmarcotte’s picture

Thank you so much for fixing this! I installed the patch in #70.

I am seeing a quirk tho. It seems to works fine unless one of the links is to the page: user. i.e.:

Structure (admin/structure)
User (user)
Add Taxonomy (admin/structure/taxonomy/add)

Clicking on structure brings up the admin/structure with an active-trail. -Good
Clicking on User brings up the user page but there is no active-trail on neither Structure nor User -Bad
Clicking on Add Taxonomy brings up admin/structure/taxonomy/add page, with the trail descending as it should, picking up all three, Structure, User and Add Taxonomy. -Good

I tried moving User to the bottom of the list, and then again, when clicking on user there was no active-trail on any of the three - Bad.

I have no idea why the user page would stop the trail, but I figured I should pass on the observation.
Thanks again,
Bob

skeates’s picture

Sorry new to patching, but I can't seem to figure out how to run this. I'm working on the assumption it is a core patch and that I should be applying it from my root drupal folder with patch -p0 < patch_name.patch.

If so I keep getting a request for which file to patch and this is were I am stuck. If it needs to be applied from a specific folder could some one point me in the right direction and then I assume I just use patch < patch_name.patch from that folder.

jrchamp’s picture

@skeates: The patch is against drupal.git 7.x. Git compares virtual folders a and b, so you'll probably use the following to skip the a/b directories: patch -p1 < patch_name.patch

skeates’s picture

Thank you that worked a charm. Seem to be learning new things every day.

vito_swat’s picture

subscribe

andros’s picture

Subscribe

pillarsdotnet’s picture

Status: Needs review » Needs work

Review of #70, which still applies cleanly to 8.x:

+    if (!in_array($menu_name, $active_menus) && (strpos($menu_name, 'menu-') === 0)) {

Please consider changing to:

+    if (!in_array($menu_name, $active_menus) && (!strncmp($menu_name, 'menu-', 5))) {
@@ -2206,6 +2206,12 @@ function theme_menu_local_tasks(&$variables) {
 
 /**
  * Set (or get) the active menu for the current page - determines the active trail.
+ *
+ * @return
+ *   An array of menu machine names, in order of preference. The
+ *   'menu_default_active_menus' variable may be used to assert a menu order
+ *   different from the order of creation, or to prevent a particular menu from
+ *   being used at all in the active trail.
  */
 function menu_set_active_menu_names($menu_names = NULL) {
   $active = &drupal_static(__FUNCTION__);

I think you need an example of how to set the menu_default_active_menus variable, or at least an indication of what structure it must have. The user shouldn't have to reverse-engineer the code in order to discover whether to set $conf['menu_default_active_menus'] = 'navigation,main-menu'; or $conf['menu_default_active_menus'] = array('navigation', 'main-menu').

The documentation should also explain why setting this variable may be useful. That is, it should explain how setting this variable would affect the visible appearance of the site.

-  if (!isset($preferred_links[$path])) {
-    $preferred_links[$path] = FALSE;
+  if (!isset($selected_menu)) {
+    // Use an illegal menu name as the key for the preferred menu link.
+    $selected_menu = '__preferred__';
+  }
+    if ($selected_menu !== '') {
+      // If we want a specific menu, put it at the front of the list.
+      array_unshift($menu_names, $selected_menu);
+    }

Why don't you set $selected_menu to the empty string, since that is the value you check for later? Surely an empty string is also illegal as a menu name.

+              $preferred_links[$path][$menu_name] = $candidate_item;
+              if (empty($preferred_links[$path]['__preferred__'])) {
+                // Store the most specific link.
+                $preferred_links[$path]['__preferred__'] = $candidate_item;
+              }

Again, why not just use an empty string?

-  return $preferred_links[$path];
+  return isset($preferred_links[$path][$selected_menu]) ? $preferred_links[$path][$\
selected_menu] : FALSE;

That should be NULL rather than FALSE to match the doxygen comments at the top of the function:

 * @return                                                                           
 *   A fully translated menu link, or NULL if not matching menu link was             
 *   found. The most specific menu link ('node/5' preferred over 'node/%') in        
 *   the most preferred menu (as defined by menu_get_active_menu_names()) is         
 *   returned.                                                                       
pillarsdotnet’s picture

Version: 7.x-dev » 8.x-dev
jrchamp’s picture

I have rerolled #70, taking into account the suggestions from #86 with the following notes:
* strpos performs better in my tests than strncmp and is easier to read
* '__preferred__' seems like better self documenting code than the empty string
* the previous version returned FALSE, seems like the comments are wrong

bmarcotte mentioned in #80 that the User page (and perhaps other pages) are not working with the patch in #70 and I would be surprised if the minor changes in this reroll resolve the issue.

Finally, given that is not my patch and that I was primarily involved so that this issue could hopefully be fixed in 7.x (and not 8.x as this issue is now tagged), I would prefer if someone with a better understanding of the menu and active trail system would continue this work.

bowersox’s picture

Why was this moved to 8.x-dev? I assume the reason is that this fix will apply to 8.x and then be back-ported to be in the 7.1 release (hopefully).

This is a really nasty bug that will hold up sites adopting D7. It's important to fix it soon in 7.x.

pillarsdotnet’s picture

Issue tags: +Needs backport to D7

Tagging

Greg Varga’s picture

Subscribe

Tor Arne Thune’s picture

Status: Needs work » Needs review

The new patch in #88 needs some review.

Tommy Kaneko’s picture

SEE EDIT NOTE

I am not sure that I have the same issue as this one. Please let me know if you think it sounds like the same issue / is different. I am using superfish module, but I am getting problems without it too.

The menus I have are being built beyond the first level (parents), but the children disappear again on node edit/save - let me explain.

To recreate my issue:
- build a custom menu tree with children
- make sure that the custom menu tree is expanded and visible. You can do this by rebuilding the menu (hopefully), by reordering the children elements in the menu edit page and saving.
- insert a node, specifying a menu item to be inserted/updated within the custom menu tree, from within the node edit form.

All sibling menu items of the updated/inserted menu item will now have disappeared, and the parent will be collapsed. All other children seem to be visible.

I have tried all of the patches here without success. I'll be investigating this further, but it would reassure me if I find that someone else has had the same issue.

EDIT: It turns out that this was more a problem with a completely different module: taxonomy_menu . So this is a red herring. Ignore what was said above!

pillarsdotnet’s picture

@#88

strpos performs better in my tests than strncmp and is easier to read

I didn't believe you until I did my own benchmarks. I'll have to disagree with the "easier to read" but you're correct about the performance. See http://php.net/manual/function.strncmp.php#104047

Matt Townsend’s picture

Is there any news on getting this fix into 7.x?

pillarsdotnet’s picture

Is there any news on getting this fix into 7.x?

I see that kind of unhelpful comment all the time. Nonetheless, based on a thorough review of this issue, here are the obstacles that may prevent this patch from being applied to 8.x (and subsequently 7.x):

  • It has a priority of "major", so bugs marked "critical" will take priority over this issue.
  • It is currently marked "needs review". Other issues marked "reviewed and tested by the community" will take priority over this issue.
  • It doesn't have tests, and according to John Albin, cannot have tests until a new feature is added. Therefore, this issue may be blocked until #520106 is applied to 8.x and 7.x
  • It has been reported that the currently proposed patch does not fix the reported problem in all cases.
  • The author of the latest revision of this patch has expressed doubts about its readiness, and is unwilling to work on it further.

You can help by:

  • Resolving or more clearly identifying the problem reported by bmarcotte.
  • Taking ownership of this issue or finding another competent developer to do so.
Murz’s picture

Got this issue too, subscribe.

Murz’s picture

I have test patch from #88 comment on Drupal 7.2 and it works successfully on my sites, without issues, so maybe time to apply it into HEAD?

r_honey’s picture

subscribing...

jlab’s picture

I have also tested the patch from #88 on my D7 sites and the menus is now working as expected.

pillarsdotnet’s picture

#88 re-rolled for current d8 and d7 using git format-patch. No code changes.

star-szr’s picture

@#96 - Some of us are newbies and are not familiar with the seemingly nebulous process of getting patches integrated into core. If this process is explained somewhere it's not very well publicized in my opinion. Thank you for the very thorough post.

gagarine’s picture

With the patch from #88 on D7.2 menu_block with link of 2nd level never show up. Don't know if it should be fixed in menu_block or here. I revert the patch and every things works again (after clearing the cache).

lee20’s picture

Subscribing... and apologizing for the pollution.

lee20’s picture

Issue tags: -Needs backport to D7

Arg.. not sure why it's saying I added that tag... autofill? If the tag from #104 isn't supposed to be here please remove it.

pillarsdotnet’s picture

Issue tags: +Needs backport to D7
stewart.adam’s picture

Subscribing

timcosgrove’s picture

Subscribing. We need custom menus to behave as one would expect. Will hammer at this and mark it RBTC if it seems sound.

mabuweb’s picture

Patch in #88 doesn't work for me in D7.4

derMatze’s picture

Subscribing

MustangGB’s picture

Tested #101 in D7.4
Doesn't work for <front>

Tor Arne Thune’s picture

Version: 7.x-dev » 8.x-dev

All bugs need to be fixed in the current development version (8.x) before they can be fixed in 7.x. Backport policy. This doesn't mean that we have to wait for the release of Drupal 8.0 to get this backported, fortunately, so a backport to 7.x would probably soon follow once it's marked 'reviewed and tested by community', because the code-base of 8.x and 7.x are practically identical at this point.

EDIT: Uh, this post was a comment to #112 which disappeared.

kerios83’s picture

I'm using domain access, i18n and menu attributes. I have created new menu - My Page Menu and added some links there. Sublinks don't show up... This is serious. This problem occurs even in 7.4 ! Im gona try http://drupal.org/project/menu_block now cause it seems to be the only solution.

bitcore’s picture

Patch posted in #101 is working for me. Drupal 7.4. Patch applied by hand. Cleared cache in admin->config->dev and custom menus began expanding and functioning as expected like the main menu does.
Thank you pillarsdotnet.

mabuweb’s picture

Patch posted in #101 is works for me too in Drupal 7.4. if applied manually.
Thanks

kerios83’s picture

Yep I can confirm that #101 - per_menu_preferred_links-942782-101-d7.patch works fine.

Shmee’s picture

@kerios83

That is what I had to do.

sonar_un’s picture

Subscribe, can't wait until this gets committed into D7

MustangGB’s picture

Status: Needs review » Needs work

As I mentioned in #111 patch at #101 doesn't work for <front>

Yanivs’s picture

Installed D7 patch from #101 (per_menu_preferred_links-942782-101-d7.patch) manually on D7.4 and it works.

Please note that after manual patching you need to update the database by going to:

www.your-site-name.com/update.php

joostvdl’s picture

Default Patch from #101 results in :

patching file includes/menu.inc
patching file modules/menu/menu.install
patching file modules/menu/menu.module
Hunk #1 succeeded at 268 (offset 1 line).
Hunk #2 succeeded at 314 (offset 1 line).

Further it works. Not tested the option.

dstol’s picture

Subscribing as this affects D7 taxonomy menu.

kerios83’s picture

Ok, after installing drupal 7.7 menus still don't work as it should. Is there any patch for drupal 7.7 so menu sublinks (child links) are properly displayed if u click on parent link ?

spamator12’s picture

#101 does NOT work with 7.7, I'm using latest i18n with DA and having BIG problem with drupal MENU. This is actually a CRITICAL issue cause without some core hacking/custom module installing u can't get to the site content... I hope it will get FIXED soon cause now you have to use 7.4 witch #101 patch to get your site working.

aspilicious’s picture

What does not work? Doesn't it get applied (the patch) or do you get errors or warnings?

spamator12’s picture

@aspilicious drupal custom menu don't work as it should I think. If you got menu like:

main menu item1
-sublink1
-sublink2
main menu item2
-sublink3
-sublink4

and if u click on main menu item, sublinks won't show up. It's simple vertical menu. With this http://drupal.org/node/942782#comment-4574324 patch it works like a charm, but drupal 7.7 updated some of the files u need to update via patch and it just don't work anymore.

jn2’s picture

It would be best to concentrate on the 8.x patch. That must be committed before 7.x.

That said, something has changed since the #101 patch was rolled that causes it to fail. Here's what I got when I tried to apply it to today's most updated 8.x:

Checking patch includes/menu.inc...
error: while searching for:
 * @param $path
 *   The path, for example 'node/5'. The function will find the corresponding
 *   menu link ('node/5' if it exists, or fallback to 'node/%').
 *
 * @return
 *   A fully translated menu link, or NULL if not matching menu link was
 *   found. The most specific menu link ('node/5' preferred over 'node/%') in
 *   the most preferred menu (as defined by menu_get_active_menu_names()) is
 *   returned.
 */
function menu_link_get_preferred($path = NULL) {
  $preferred_links = &drupal_static(__FUNCTION__);

  if (!isset($path)) {
    $path = $_GET['q'];
  }

  if (!isset($preferred_links[$path])) {
    $preferred_links[$path] = FALSE;

    // Look for the correct menu link by building a list of candidate paths,
    // which are ordered by priority (translated hrefs are preferred over
    // untranslated paths). Afterwards, the most relevant path is picked from

error: patch failed: includes/menu.inc:2318
error: includes/menu.inc: patch does not apply

Looks like the third hunk fails.

jn2’s picture

Here's a the D8 patch from #101 re-rolled. I did not include the menu.install update for D6 to D7, since that doesn't belong in a D8 patch. This worked fine on my dev site. We'll see what the testbot says.

jn2’s picture

Status: Needs work » Needs review

I tried to test for #120's comment that it doesn't work for <front>. If the menu block is set to only show on the front page, the expanded menu won't appear because the block will not show on the page the link goes to (since it is not the front page). Expected behavior. If the block is set to show on all pages except the front page, it works as expected.

xjm’s picture

Tagging.

bitcore’s picture

For those interested in maintaining their website's security updates, without having to manually re-patch:
Update on my current conditions:
I updated from 7.4 to 7.7, but neglected to replace the following files during the upgrade:
patching file includes/menu.inc
patching file modules/menu/menu.install
patching file modules/menu/menu.module

Everything works as expected after performing the upgrade.

I did a comparison between the two files (using ultraedit), and I think it is important to note that it does appear that changes were made to menu.inc from 7.4 to 7.7, mainly involving some logic surrounding a couple SQL queries. Since my main motivation to upgrade was security, I incorporated those changes into my modified files.

Changes are around these lines; search for these strings:
" foreach ($candidates as $mlid) {"
"'updated' => 0,"
these two strings in menu.inc are a line or two above where the logic changes reside.

I would love to see this patch incorporated into the core.

James Andres’s picture

Apologies to distract from the D8 ongoing work. Here is a re-roll of #101 for Drupal 7.7 that takes into account the fact that 7.7 already has the function menu_update_7001. I couldn't find a better place to post this than here, thanks.

dcotruta’s picture

Subscribing. How is this _not_ critical? How can it be ok that in 7.7 you _cannot_ have a multisite setup with more than one level of navigation (since you'd have to create custom menus for each site).

aspilicious’s picture

*ignore this*

Aeternum’s picture

Subscribing. Agree with #134.

sleepingmonk’s picture

subscribe

stevieb’s picture

subscribe

stevieb’s picture

I downloaded the accordion_menu module and as a solution it works
http://drupal.org/project/accordion_menu

Pavlos-1’s picture

subscribe

deceth’s picture

subscribe

raitwebs’s picture

Running D7.7, tried manual patching of #101 - No go.
Copied the files suggested in #132 and applied the patch to those - Still no go.

James Andres’s picture

@raitwebs, did you try the patch I posted in #133?

bitcore’s picture

Update: I had the menu module go nuts today during the deletion of a page.
This is *most likely* due to me manually applying the patch listed in #101 to 7.4, and then merging the 7.7 updates in includes/menu.inc.
I'll be checking out the accordion menu project since it appears to offer some interesting functionality that we really wanted in the first place. Thanks stevieb for the heads up.

pillarsdotnet’s picture

#133 re-rolled for current 7.x checkout.

dstol’s picture

Another reroll based on #145 to correct a variable name.

+++ b/includes/menu.inc
@@ -2338,23 +2345,30 @@ function menu_set_active_trail($new_trail = NULL) {
+function menu_link_get_preferred($path = NULL, $select4ed_menu = NULL) {
steven.itterly’s picture

Version: 8.x-dev » 7.7

subscribing

pillarsdotnet’s picture

Version: 7.7 » 8.x-dev

Please don't change the version.

raitwebs’s picture

@James Andres just tried #146 with 7.7 files. Nothing. Or, well I might be missing something here and my issue is not related with this bug. My custom menus receive the active trail just fine, as long as I'm on a page that has an actual menu entry.

Menu
|_Submenu
...|_News
|_Another submenu

News is a Views generated page. If I go to the News page, the menu behaves nicely, receiving the trail. However once I click on an actual news node, the menu closes and there's no active trail.
The news items have the path of: menu/submenu/news/nodetitle
I am using "Menu position" module to put the nodes into the menu. Works like a charm for the main menu.

nicnet’s picture

subscribe

khiminrm’s picture

subscribe

travisc’s picture

custom_menus.patch queued for re-testing.

travisc’s picture

duplicate

travisc’s picture

Just tested this patch on 7.7, seems to work as described, no issues yet... THANK YOU!

johnbarclay’s picture

This is critical to 2 sites I'm working on. I will test the patches as soon as there is indication there is interest in applying to 7.x when they are working.

jn2’s picture

@johnbarclay
(and @ anyone else who wants to see this committed to D7)

If you want to get this patch committed to 7.x, then please help test the patch for 8.x. That's the ONLY way it will ever be committed to 7.x. All bug fixes go into 8.x first. I also want to see this in 7.x. That's why I re-rolled the patch for 8.x.

calculus’s picture

sub

Matthias Lersch’s picture

For me the patch from #133 worked fine with drupal 7.7 and solved my problem with one custom menu as primary and secondary menu.

The patches from #145 and #146 failed:

$ git apply --check per_menu_preferred_links-942782-146-d7.patch
error: patch failed: includes/menu.inc:2338
error: includes/menu.inc: patch does not apply
aspilicious’s picture

Status: Needs review » Needs work

Needs new rerolls, I don't know what patch to apply to D7 or D8. And they don't apply...

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
7.52 KB
8.79 KB

Re-rolled for 7.x and 8.x. Corrected some fuzz; no code changes. Note that #133 is still valid for Drupal 7.7, but patches for review are always made against the latest git checkout of the development branch.

GiorgosK’s picture

maybe unrelated to this issue
I had children menu items being properly expanded (when parent active they would appear in the menu)
but after after a while this functionality stopped ...

I applied the patch from #146 in the hopes that this would solve it ... but no luck
if anybody has a clue on what might be causing this I would be greatful ...

aspilicious’s picture

Off-topic: Girogosk ==> Edit the menu and look if the checkbox "show expanded" or something like that is checked.
This here is not a support channel for every menu problem. Please try to stay focused on this one patch.

If you got an other problem please open a new issue.

ennazussuzanne’s picture

subscribe

GiorgosK’s picture

@aspilicious thanks for the reply
my problem might have seemed unrelated but it shows the same exact symptoms and its another system menu bug #1266486: entity translated children menus are not present when parent is viewed [menu entity_translation support] and link might be useful to someone

tran_tien’s picture

+1 sub

pindaman’s picture

melon’s picture

Re-rolled for 7.x as 7.8 already contains menu_update_7002() and this caused a fatal error of redeclaration. Renamed the function to menu_update_7003() so this applies to 7.8 installs now.

tomwrobel’s picture

subscribing

mlecha’s picture

subscribe

colan’s picture

That patch didn't apply. How about this one?

pillarsdotnet’s picture

interdiff shows no difference between #167 and #170.

Manual inspection reveals that the patch in #167 contains full sha1 commit references; #170 contains abbreviated ones, and a slightly shorter subject line. I fail to see why one would apply and the other would not.

(checking out 7.x-dev from git...)

Okay, it applies with slight fuzz. Here's a re-roll to remove the fuzz.

pillarsdotnet’s picture

For real, this time.

colan’s picture

Looks great. #172 for D7 applies and works too. I'm declaring it RTBCed. Can someone with a D8 set-up test #160? I tried applying it to the latest 8.x-dev, and it still applies cleanly, but I don't have an active site I can test it with.

pillarsdotnet’s picture

I don't think anybody has an active site running Drupal 8 yet.

colan’s picture

Status: Needs review » Reviewed & tested by the community

I just got a D8 set up to test #160. All good. Please also commit #172 for D7.

tomwrobel’s picture

#172 did the job, thank you!

catch’s picture

How many additional queries per page is this adding? One per menu?

Scyther’s picture

Sub

catch’s picture

Status: Reviewed & tested by the community » Needs review

CNR until there's an answer to #177.

colan’s picture

Status: Needs review » Reviewed & tested by the community

Looking at the patch file it appears as though we have added the following (potential) queries:

catch’s picture

Status: Reviewed & tested by the community » Needs review

Has anyone checked this query is still indexed with this condition removed?

     // Weight must be taken from {menu_links}, not {menu_router}.
     $query->addField('ml', 'weight', 'link_weight');
     $query->fields('m');
-    $query->condition('ml.menu_name', $menu_names, 'IN');
     $query->condition('ml.link_path', $path_candidates, 'IN');
bulldozer2003’s picture

If you use the patch, you will need to remember to execute a database update before the changes will take place!
Patch from 172 works with 7.8

jacieyang’s picture

#2: 942782-3-custom_menus.patch queued for re-testing.

pillarsdotnet’s picture

Patch no longer applies since #520106 was committed. The rejected fragment is:

--- includes/menu.inc                                                                
+++ includes/menu.inc                                                                
@@ -1199,7 +1199,7 @@
         // parameters accordingly.                                                  
         if ($item['access']) {                                                      
           // Find a menu link corresponding to the current path.                    
-          if ($active_link = menu_link_get_preferred()) {                           
+          if ($active_link = menu_link_get_preferred(NULL, $menu_name)) {           
             // The active link may only be taken into account to build the          
             // active trail, if it resides in the requested menu. Otherwise,        
             // we'd needlessly re-run _menu_build_tree() queries for every menu

And the conflicting commit fragment is:

@@ -1198,8 +1239,9 @@ function menu_tree_page_data($menu_name, $max_depth = NULL, $only_active_trail =
         // If the item for the current page is accessible, build the tree
         // parameters accordingly.
         if ($item['access']) {
-          // Find a menu link corresponding to the current path.
-          if ($active_link = menu_link_get_preferred()) {
+          // Find a menu link corresponding to the current path. If $active_path
+          // is NULL, let menu_link_get_preferred() determine the path.
+          if ($active_link = menu_link_get_preferred($active_path)) {
             // The active link may only be taken into account to build the
             // active trail, if it resides in the requested menu. Otherwise,
             // we'd needlessly re-run _menu_build_tree() queries for every menu

Leaving this at CNW because I don't know the right way to resolve the conflict.

pillarsdotnet’s picture

Just a wild hairy guess. Would this work?

--- a/includes/menu.inc
+++ b/includes/menu.inc
@@ -1241,7 +1241,7 @@ function menu_tree_page_data($menu_name, $max_depth = NULL, $on
         if ($item['access']) {
           // Find a menu link corresponding to the current path. If $active_path
           // is NULL, let menu_link_get_preferred() determine the path.
-          if ($active_link = menu_link_get_preferred($active_path)) {
+          if ($active_link = menu_link_get_preferred($active_path, $menu_name)) {
             // The active link may only be taken into account to build the
             // active trail, if it resides in the requested menu. Otherwise,
             // we'd needlessly re-run _menu_build_tree() queries for every menu
pillarsdotnet’s picture

Status: Needs work » Needs review

Let's see what the testbot has to say, anyway.

pillarsdotnet’s picture

So to answer the question posed by @catch in #181:

Has anyone checked this query is still indexed with this condition removed?

     // Weight must be taken from {menu_links}, not {menu_router}.
     $query->addField('ml', 'weight', 'link_weight');
     $query->fields('m');
-    $query->condition('ml.menu_name', $menu_names, 'IN');
     $query->condition('ml.link_path', $path_candidates, 'IN');

So I added the following to find out:

--- a/includes/menu.inc
+++ b/includes/menu.inc
@@ -2439,7 +2439,12 @@ function menu_link_get_preferred($path = NULL, $selected_menu
     // Sort candidates by link path and menu name.
     $candidates = array();
-    foreach ($query->execute() as $candidate) {
+    $result = $query->execute();
+    ob_start();
+    $result->debugDumpParams();
+    $menu_query = ob_get_clean();
+    error_log("$menu_query\n", 3, '/tmp/menu.log');
+    foreach ($result as $candidate) {
       $candidate['weight'] = $candidate['link_weight'];
       $candidates[$candidate['link_path']][$candidate['menu_name']] = $candidate;
       // Add any menus not already in the menu name search list.

Then I added a new menu called "Custom menu" and a Basic page called "Test page".

I edited my newly-added page and clicked on [x] Provide a menu link and did not see my custom menu in the Parent item dropdown list. (Why?)

Selected Main menu instead, and clicked on Save.

Back to Edit.

Changed the Parent item to Custom menu and clicked Save again.

Checked the output of /tmp/menu.log and saw:

SQL: [205] SELECT ml.*, m.*, ml.weight AS link_weight
FROM 
menu_links ml
LEFT OUTER JOIN menu_router m ON m.path = ml.router_path
WHERE  (ml.link_path IN  (:db_condition_placeholder_0, :db_condition_placeholder_1)) 
Params:  2
Key: Name: [27] :db_condition_placeholder_0
paramno=-1
name=[27] ":db_condition_placeholder_0"
is_param=1
param_type=2
Key: Name: [27] :db_condition_placeholder_1
paramno=-1
name=[27] ":db_condition_placeholder_1"
is_param=1
param_type=2

(sigh) Apparently the PHP docs were wrong when they said Dumps ... the list of parameters, with their name, ... the value, ...

Okay, will some genius tell me the right way to extract the actual query, complete with placeholder values?

Anyhow, running DESCRIBE SELECT ml.*, m.*, ml.weight AS link_weight FROM menu_links ml LEFT OUTER JOIN menu_router m ON m.path = ml.router_path WHERE (ml.link_path IN ('foo'));
I get:

+----+-------------+-------+--------+---------------+-----------+---------+-------------------+------+-------------+
| id | select_type | table | type   | possible_keys | key       | key_len | ref               | rows | Extra       |
+----+-------------+-------+--------+---------------+-----------+---------+-------------------+------+-------------+
|  1 | SIMPLE      | ml    | ref    | path_menu     | path_menu | 386     | const             |    1 | Using where |
|  1 | SIMPLE      | m     | eq_ref | PRIMARY       | PRIMARY   | 767     | d8.ml.router_path |    1 |             |
+----+-------------+-------+--------+---------------+-----------+---------+-------------------+------+-------------+
2 rows in set (0.00 sec)

Does that answer your question?

jrchamp’s picture

Looks like the path_menu index from the menu_links table and the primary key on the menu_router table are being used, so yes it appears the the query is not doing any table scans.

catch’s picture

It looks like it's still using menu_path which is good.

However if that's the case, then does the menu_path index still need to include the menu_name column then?

webengr’s picture

subscribe

-- this is aggravating... had to move menu items back to navigation menu so I could use CSS on active trail,
hope this gets fixed in drupal 7 also.

TimG1’s picture

eek...sorry, subscribe.

wickedskaman’s picture

It's incredible that this thread is almost a year old now and hasn't been fixed in the trunk. This is a critical feature for CSS work and hacky workarounds are annoying seeing as it's something that was working fine in D6.

Just 2 more cents.

pillarsdotnet’s picture

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

@#189 by catch on October 1, 2011 at 3:40pm:

It looks like it's still using menu_path which is good.

However if that's the case, then does the menu_path index still need to include the menu_name column then?

I have no idea how to begin to answer that question. I don't even know whether you want me to run an EXPLAIN on some query or whether you want me to roll a patch or whether you're asking me about the functionality of the entire rest of Drupal core as it relates to the the menu system.

sun’s picture

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

@catch: That's a tough question. Would be interesting to investigate whether there are automated daemons/tools capable of monitoring index usage.

For now, I guess this means grepping for menu_links and checking for queries that are using that index and the menu_name in that index.

Please also note that there is no menu system maintainer sign-off for this approach and patch yet. The only one who chimed in and who'd I relate with that group despite not officially listed is @Damien Tournoud. And @JohnAlbin of course, but it's partially his own patch.

cangeceiro’s picture

patch in #185 worked for me, subscribing

Tor Arne Thune’s picture

FileSize
5.29 KB

The patch in #185 works for me and applies cleanly to 8.x.

Code style in the patch looks good. Can't comment on the logic.

catch’s picture

For now, I guess this means grepping for menu_links and checking for queries that are using that index and the menu_name in that index.

Yeah this is all that's needed.

pillarsdotnet’s picture

@#197 by catch on October 8, 2011 at 9:35am:

It looks like it's still using menu_path which is good.

However if that's the case, then does the menu_path index still need to include the menu_name column then?

For now, I guess this means grepping for menu_links and checking for queries that are using that index and the menu_name in that index.

Yeah this is all that's needed.

Okay; here are all D8 queries with a condition depending on {menu_links}.menu_name:

ForumTestCase::doAdminTests()

    // Retrieve forum menu id.
    $mlid = db_query_range("SELECT mlid FROM {menu_links} WHERE link_path = 'forum' AND menu_name = 'navigation' AND module = 'system' ORDER BY mlid ASC", 0, 1)->fetchField();

toolbar_get_menu_tree()

  $admin_link = db_query('SELECT * FROM {menu_links} WHERE menu_name = :menu_name AND module = :module AND link_path = :path', array(':menu_name' => 'management', ':module' => 'system', ':path' => 'admin'))->fetchAssoc();

menu_node_prepare()

        $mlid = db_query_range("SELECT mlid FROM {menu_links} WHERE link_path = :path AND menu_name = :menu_name AND module = 'menu' ORDER BY mlid ASC", 0, 1, array(
          ':path' => 'node/' . $node->nid,
          ':menu_name' => $menu_name,
        ))->fetchField();

MenuTestCase::deleteCustomMenu()

    // Test if all menu links associated to the menu were removed from database.
    $result = db_query("SELECT menu_name FROM {menu_links} WHERE menu_name = :menu_name", array(':menu_name' => $menu_name))->fetchField();

menu_delete_menu_confirm()

  $num_links = db_query("SELECT COUNT(*) FROM {menu_links} WHERE menu_name = :menu", array(':menu' => $menu['menu_name']))->fetchField();

menu_delete_menu_confirm_submit()

  // Reset all the menu links defined by the system via hook_menu().
  $result = db_query("SELECT * FROM {menu_links} ml INNER JOIN {menu_router} m ON ml.router_path = m.path WHERE ml.menu_name = :menu AND ml.module = 'system' ORDER BY m.number_parts ASC", array(':menu' => $menu['menu_name']), array('fetch' => PDO::FETCH_ASSOC));

book_menu_subtree_data()

  $link_exists = db_query_range("SELECT 1 FROM {menu_links} WHERE menu_name = :menu", 0, 1, array(':menu' => $value))->fetchField();

menu_edit_menu_name_exists()

      $query = db_select('menu_links', 'ml', array('fetch' => PDO::FETCH_ASSOC));
      $query->join('menu_router', 'm', 'm.path = ml.router_path');
      $query->join('book', 'b', 'ml.mlid = b.mlid');
      $query->fields('b');
      $query->fields('m', array('load_functions', 'to_arg_functions', 'access_callback', 'access_arguments', 'page_callback', 'page_arguments', 'delivery_callback', 'title', 'title_callback', 'title_arguments', 'type'));
      $query->fields('ml');
      $query->condition('menu_name', $link['menu_name']);
      for ($i = 1; $i <= MENU_MAX_DEPTH && $link["p$i"]; ++$i) {
        $query->condition("p$i", $link["p$i"]);
      }
      for ($i = 1; $i <= MENU_MAX_DEPTH; ++$i) {
        $query->orderBy("p$i");
      }

system_admin_config_page()

    $result = db_query("
      SELECT m.*, ml.*
      FROM {menu_links} ml
      INNER JOIN {menu_router} m ON ml.router_path = m.path
      WHERE ml.link_path <> 'admin/help' AND menu_name = :menu_name AND ml.plid = :mlid AND hidden = 0", $admin, array('fetch' => PDO::FETCH_ASSOC));

menu_tree_page_data()

              $result = db_select('menu_links', NULL, array('fetch' => PDO::FETCH_ASSOC))
                ->fields('menu_links', array('mlid'))
                ->condition('menu_name', $menu_name)
                ->condition('expanded', 1)
                ->condition('has_children', 1)
                ->condition('plid', $parents, 'IN')
                ->condition('mlid', $parents, 'NOT IN')
                ->execute();

menu_load_links()

  $links = db_select('menu_links', 'ml', array('fetch' => PDO::FETCH_ASSOC))
    ->fields('ml')
    ->condition('ml.menu_name', $menu_name)

_menu_link_find_parent()

    $query = db_select('menu_links');
    $query->condition('module', 'system');
    // We always respect the link's 'menu_name'; inheritance for router items is
    // ensured in _menu_router_build().
    $query->condition('menu_name', $menu_link['menu_name']);

    // Find the parent - it must be unique.
    $parent_path = $menu_link['link_path'];
    do {
      $parent = FALSE;
      $parent_path = substr($parent_path, 0, strrpos($parent_path, '/'));
      $new_query = clone $query;
      $new_query->condition('link_path', $parent_path);
      // Only valid if we get a unique result.
      if ($new_query->countQuery()->execute()->fetchField() == 1) {
        $parent = $new_query->fields('menu_links')->execute()->fetchAssoc();
      }

_menu_set_expanded_menus()

  $names = db_query("SELECT menu_name FROM {menu_links} WHERE expanded <> 0 GROUP BY menu_name")->fetchCol();

menu_link_children_relative_depth()

  $query = db_select('menu_links');
  $query->addField('menu_links', 'depth');
  $query->condition('menu_name', $item['menu_name']);
  $query->orderBy('depth', 'DESC');
  $query->range(0, 1);

  $i = 1;
  $p = 'p1';
  while ($i <= MENU_MAX_DEPTH && $item[$p]) {
    $query->condition($p, $item[$p]);
    $p = 'p' . ++$i;
  }

  $max_depth = $query->execute()->fetchField();

menu_link_move_children()

  $query = db_update('menu_links');

  $query->fields(array('menu_name' => $item['menu_name']));

  $p = 'p1';
  $expressions = array();
  for ($i = 1; $i <= $item['depth']; $p = 'p' . ++$i) {
    $expressions[] = array($p, ":p_$i", array(":p_$i" => $item[$p]));
  }
  $j = $existing_item['depth'] + 1;
  while ($i <= MENU_MAX_DEPTH && $j <= MENU_MAX_DEPTH) {
    $expressions[] = array('p' . $i++, 'p' . $j++, array());
  }
  while ($i <= MENU_MAX_DEPTH) {
    $expressions[] = array('p' . $i++, 0, array());
  }

  $shift = $item['depth'] - $existing_item['depth'];
  if ($shift > 0) {
    // The order of expressions must be reversed so the new values don't
    // overwrite the old ones before they can be used because "Single-table
    // UPDATE assignments are generally evaluated from left to right"
    // see: http://dev.mysql.com/doc/refman/5.0/en/update.html
    $expressions = array_reverse($expressions);
  }
  foreach ($expressions as $expression) {
    $query->expression($expression[0], $expression[1], $expression[2]);
  }

  $query->expression('depth', 'depth + :depth', array(':depth' => $shift));
  $query->condition('menu_name', $existing_item['menu_name']);
  $p = 'p1';
  for ($i = 1; $i <= MENU_MAX_DEPTH && $existing_item[$p]; $p = 'p' . ++$i) {
    $query->condition($p, $existing_item[$p]);
  }

  $query->execute();

menu_update_parental_status()

    $query = db_select('menu_links');
    $query->addField('menu_links', 'mlid');
    $query->condition('menu_name', $item['menu_name']);
    $query->condition('hidden', 0);
    $query->condition('plid', $item['plid']);
    $query->range(0, 1);
    if ($exclude) {
      $query->condition('mlid', $item['mlid'], '<>');
    }
    $parent_has_children = ((bool) $query->execute()->fetchField()) ? 1 : 0;
colan’s picture

Status: Needs review » Reviewed & tested by the community

Ready to go?

chx’s picture

Status: Reviewed & tested by the community » Needs work

Thanks for working on this patch. It could use an automated test before it is committed. If you need help you can ask the friendly people in the #drupal-contribute IRC channel.

kerios83’s picture

I hope it will be in 7.10 ohhh - "Added two new API functions (menu_tree_set_path()/menu_tree_get_path()) were added in order to enable setting the active menu trail for dynamically generated menu paths."

royerd’s picture

Why are these menus working properly on drupal.org and not in our 7.9 version of drupal? drupal.org is version 7, right? So why doesn't the install work the same way that the mail drupal site works? --wait, my mistake: I see that the drupal.org is probably using the book menus for documentation not custom menus. Hard to believe that such a fundamental problem was not detected earlier.

MustangGB’s picture

drupal.org is version 7, right?

Not yet, still v6

colan’s picture

Tor Arne Thune’s picture

Here is a reroll of the patch in #185 with a test added to assert that the child link is displayed when viewing the parent link's node, for custom menus.
The first patch is the test only and should fail.

chaplinskiy@gmail.com’s picture

#6: 942782-6-custom_mens.patch queued for re-testing.

peterx’s picture

#205 per-menu-preferred-links-942782-205.patch works beautifully in Drupal 7.9.

colan’s picture

Status: Needs review » Reviewed & tested by the community
caktux’s picture

Losing the page title in many admin pages after applying the latest patch to Drupal 7.9... Maybe something related to locale or i18n?

aspilicious’s picture

Status: Reviewed & tested by the community » Needs review

We need to check 209 first

Tor Arne Thune’s picture

@#209: How did you apply the patch in #205 to Drupal 7.9? It's made for Drupal 8.x. Did you mean the latest Drupal 7 patch?

Tor Arne Thune’s picture

Here's a D7 version of the patch in #205 with the D7 update function from the patch in #172. For those who want to test on D7. Be sure to run update.php after applying the patch.

jherencia’s picture

Status: Needs review » Reviewed & tested by the community

@colan set this as reviewed & tested by the community and nothing has changed since then for D8.x.

caktux’s picture

@#211: Tried the D7 patch and was getting the same result, so I applied the D8 one with -p2, slightly offset...

Just tried the one in #212 and ran update.php, but titles in admin are still getting lost. Taking a wild guess that i18n_menu might depend on something that got changed by this patch.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

Setting an issue RTBC doesn't actually mean it is RTBC. It just means one person thinks it is. :)

There is still this question that needs to be addressed:

Losing the page title in many admin pages after applying the latest patch to Drupal 7.9... Maybe something related to locale or i18n?

Just tried the one in #212 and ran update.php, but titles in admin are still getting lost. Taking a wild guess that i18n_menu might depend on something that got changed by this patch.

So, we need to track down the cause of that issue before we move forward here.

caktux’s picture

I confirm it's i18n_menu that causes the title issue since it expects menu_link_get_preferred() to return an array. It will need to adapt to the __preferred__ and $menu_name indexes also.

xjm’s picture

If I understand correctly, this patch changes the datatype of a return value under certain circumstances? That's an API change, which raises red flags for backporting this fix. Is there any way to work around that in the patch?

caktux’s picture

Looking at it again, I'm pretty sure the datatype of the return value doesn't change. The thing is that i18n_menu rebuilds menu_link_get_preferred's static cache. If that's not considered an API change then I think it should be safe to backport.

linno’s picture

xjm’s picture

Alright, I looked closely at the patch and menu_link_get_preferred() was already capable of returning false before the patch; it just wasn't properly documented that it could do so. What's changing is not a return value but the array structure of the static cache, as @caktux implies in #218. I'm not sure we should really support contrib tinkering with that, so maybe someone should open a followup issue with i18n that references this issue.

caktux’s picture

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Alright, now that this question has been resolved we can move this back to RTBC. :) Thanks @caktux.

catch’s picture

laghalt’s picture

Applied the #172 patch on Drupal 7.9
and it worked.

But when installing the i18n-module "Menu translation" every menu was efected:
Only the first level of menus where displayed. That goes for the main-menu that was suposed to bee affected of the "Menu translation" module, but also for every menu whatever. The admin menu was only displayed in the toolbar, the breacrum-like menu was not shown.

Reverting the patched menu.inc solved the "Menu translation"-bug - leaving the active trail bug.

I originaly reported this bug at the i18n project, but it might be useful here.
http://drupal.org/node/1354632

xjm’s picture

@laghalt: Did you see #216 through #221? There should be a patch for i18n in #1351678: Follow menu_link_get_preferred active trail handling for custom menus. I'd suggest following up on that issue.

prabhatjn’s picture

I am not sure how people put that the patch worked for them on D7. It fails every time (I got WSofD), both for 7.8 and 7.9.
Since this is an important issue for one of the site I am developing, can someone please explain how it worked for them. I have used #172 for myself.
Also it seems there is a slight error when applying patch...
Here is the output I have got:

$ patch -p1 < per_menu_preferred_links-942782-186.patch
patching file includes/menu.inc
Hunk #1 FAILED at 1241.
Hunk #2 succeeded at 2206 (offset -38 lines).
Hunk #3 succeeded at 2383 (offset 2 lines).
Hunk #4 succeeded at 2388 (offset -38 lines).
Hunk #5 succeeded at 2437 (offset 2 lines).
Hunk #6 succeeded at 2404 (offset -38 lines).
1 out of 6 hunks FAILED -- saving rejects to file includes/menu.inc.rej
patching file modules/menu/menu.module

Thanks everyone,
Perry.

prabhatjn’s picture

I forgot to tell, I have also tried #212 on my D7 site.

Perry.

aspilicious’s picture

Here's a D7 version of the patch in #205 with the D7 update function from the patch in #172. For those who want to test on D7. Be sure to run update.php after applying the patch.

Done that?

xjm’s picture

@Perry Jain: The patch did not apply properly, so that is why you are getting whitescreens. It is rolled against 7.x-dev.

aspilicious’s picture

Just tested the D7 patch myself and it works fine (after running update.php)

for D8: 205
for D7: 212

Let's do this catch and webchick!

prabhatjn’s picture

Thanks @xjm,

you were right, patch at #212 worked on d7-dev version. The only thing I need to decide is if I should take risk to move my site to a d7-dev version.
I wish the big guys here solve this problem quickly...

Thanks anyway,
Perry.

xjm’s picture

#231: There is a good chance this fix will be in the next point release of Drupal 7, so I wouldn't migrate to 7.x-dev just yet. :)

laghalt’s picture

Thanks xjm!

I first thought that the #216-#221 was about D8.

#172 combined with the #221 : i18n patch worked great.
Note that applying the patch to i18n on a system without the #172 patch results in the i18n module failing.

As for the patch diskussion:
If a hunk missmaches in a patch you can usualy fix that manualy. These patches are quite small so they are not that hard to read.

xjm’s picture

Some before/after screenshots for the summary.

xjm’s picture

Issue summary: View changes

(xjm) First outline of a summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Added an issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

pwolanin’s picture

Overall this looks like the right direction, though as DamZ says in #27, excluding custom menus was a deliberate choice we made at one point, I think due to performance concerns (not sure those are valid).

I'm not sure, however, that it's correct to rely on __preferred__ being an illegal menu name. The only actual constraint is that it be NOT NULL and <= 32 chars, so it might be better to use an empty string as suggested some place in this thread, or use a magic placeholder that's > 32 chars.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Setting NW on account of #236. catch suggested a constant for the magic key (possibly defined as an empty string, or something over 32 chars, or something else not yet suggested).

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes

Updated issue summary.

pwolanin’s picture

Status: Needs work » Reviewed & tested by the community

We used such a constant in the D6 DB system: http://api.drupal.org/api/drupal/includes--database.inc/constant/DB_ERROR/6

We hard-coded the hash rather than calculating it each time, but the code comments indicate what it means.

e.g.:

sha1('MENU_PREFERRED_LINK') = 1cf698d64d1aa4b83907cf6ed55db3a7f8e92c91

which is 40 chars.

define('MENU_PREFERRED_LINK', '1cf698d64d1aa4b83907cf6ed55db3a7f8e92c91');
pwolanin’s picture

Status: Reviewed & tested by the community » Needs work
xjm’s picture

Assigned: Unassigned » xjm

I'll roll with #238.

xjm’s picture

Status: Needs work » Needs review
FileSize
1.91 KB
10.21 KB
xjm’s picture

Assigned: xjm » Unassigned
xjm’s picture

Status: Needs review » Needs work
FileSize
1.9 KB
10.21 KB

Uhm. Without wrapping a constant in quotes. Derp.

Note that the D7 backport should switch to define().

xjm’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -Needs backport to D7

The last submitted patch, custom-active-trail-942782-242.patch, failed testing.

Tor Arne Thune’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D7

#243: custom-active-trail-942782-242.patch queued for re-testing.

Does the menu.module file need to be included now, for the constant to be used in menu.inc?

xjm’s picture

Assigned: Unassigned » xjm
Status: Needs review » Needs work

#246 Oh! Duh. :) It was supposed to be in menu.inc. Rerolling.

xjm’s picture

Assigned: xjm » Unassigned
Status: Needs work » Needs review
FileSize
1.78 KB
10.26 KB

With the constant defined in the correct file.

Tor Arne Thune’s picture

#248 looks like a good solution to me, and answers part of the "concern" in #236.

JamesOakley’s picture

I don't understand how to convert the D8 patches in this issue to D7 versions. Is there a D7 version of this that I can try? I'd like to help with testing, as this is the single issue that is keeping one of my sites running on D6.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Just tested this again on my dev site. (followed the steps from the summary) If I didn't do anything wrong this should be rtbc.

JamesOakley’s picture

This issue seems to be fixed in 7.10 core release yesterday, although it wasn't mentioned in the release notes.

xjm’s picture

#252: No, this patch has not been committed yet.

JamesOakley’s picture

which is what I thought. I'm not sure why it's working all of a sudden. I'm sure the priority is to get the patch tested and committed to 8.x, and then we can all make sure 7.x is working.

aspilicious’s picture

JamesOakley this is NOT fixed. Look at the summary and reproduce that use case. You'll see it is *not* fixed in 7.10

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed/pushed to 8.x, this will need a re-roll for 7.x. Thanks for all the work on this everyone.

xjm’s picture

Status: Patch (to be ported) » Needs review
FileSize
452 bytes
10.21 KB

Rerolled for D7 with define().

Tor Arne Thune’s picture

Is the interdiff for the last D7 patch in #212? Because I couldn't find the update function in the new patch.

xjm’s picture

Status: Needs review » Needs work

The interdiff is against the D8 patch. I forgot that we had to add the update function as well.

aspilicious’s picture

Status: Needs work » Reviewed & tested by the community

Ok tested this *again* and the patch still works *surprise surprise* so I'm marking this rtbc again for drupal 7.

aspilicious’s picture

Status: Reviewed & tested by the community » Needs work

Needs work, didn't test an upgrade... *sigh*

xjm’s picture

Status: Needs work » Needs review
FileSize
1.54 KB
11.3 KB
Tor Arne Thune’s picture

Looks good now with the update function.

Mr.vantri’s picture

Status: Needs review » Reviewed & tested by the community

Hay lam

Mr.vantri’s picture

CUng duoc

Mr.vantri’s picture

hay qua di mat

geerlingguy’s picture

Status: Reviewed & tested by the community » Needs review
dj1999’s picture

#262 patch works for me.

Thanks!

artfulrobot’s picture

Patch #262 works for me on Drupal 7.10. Thank you! I was going mad thinking I'd completely misunderstood what menus were.

Summit’s picture

Hi, Could you post your changed files please?
Will this go in 7.11?
Greetings, Martijn

xjm’s picture

@Summit -- The issue is marked "needs review," which means that the current patch for D7 needs review and testing. Several people are reporting it is working; however, we might want to have someone test update.php when (e.g.) the Menu Block module is installed and configured, to ensure the configuration variable is updated properly.

Once the issue has been reviewed and tested, it can be marked "reviewed and tested by the community," and it will in all likelihood be in 7.11 barring something unforeseen. It has already been committed to D8.

Zachmo’s picture

I'm having issues getting this patch to work with different content types. I can get a "basic page" under a "basic page" to show the top lvl as active but as soon as I put another content type under the basic page it no longer has active trail class. A "Team Member" under a "Team Member" didn't work either. For some reason only the content type that drupal came with is working properly. I tried creating a brand new menu and got the same result. Has anyone run into this problem?

I ran update.php but I dont think it was working before that either.

xjm’s picture

FileSize
3.84 KB

@hortonculture: I am unable to reproduce the problem that you describe. For me, with the patch applied and the update run, custom content types in custom menus receive the appropriate styling:
menu.png
It could be that your issue is being caused by a contributed module or something else. It may be a separate issue.

Zachmo’s picture

Thanks for trying to replicate. I'll try on a clean install.

7wonders’s picture

works for me on d7 dev

FooZee’s picture

Works very well for me on D7.10 (also using a superfish menu block)... I see that some are confirming this too, should this be moved to reviewed and tested by the community ?

xjm’s picture

Aye, I think we have sufficient review now. Thanks everyone for reporting your results! I personally can't mark the issue RTBC since I made the most recent version of the patch.

Do note that, in general, it's best to post specifically how you tested the patch and what the result was, rather than just "works for me." :) That way, it helps reviewers and committers' confidence in the solution.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Followed the steps from the summary once again, checked the code for styling issues.
This had a lot of code reviews before.

R T B C

magpie5212’s picture

Subscribe, I will wait until this is incorporated in a release.

xjm’s picture

Hi @magpie5212 -- You don't need to post "subscribe" comments anymore. There's a green "Follow" button in the upper-right corner of the issue that you can use. Thanks!

sun’s picture

This looks OK for me. However, we need to revisit, analyze, and revamp this functionality for D8 (separate issue[s]).

linno’s picture

hi all
not work for me d7.10 zeropoint theme, and after apply patch my main menu not show child item on hover
help me
thanks

Tor Arne Thune’s picture

#282: Did you run update.php after applying the patch?

xjm’s picture

@linno -- Also, in core, the menus are not intended to show the child items "on hover." They show children when you click on the item. If it's not working with a contributed module that shows you child items on hover, I'd suggest filing a support request with that module.

linno’s picture

hello
@Tor Arne Thune, yes infact update says me apply update 7002 and 7003
@xjm my theme use drop down menu superfish (?), I put in my libraries foldel the plugin, have only stylized css, but I think use core menu, i've installed only menu firstchild sunday e work normally. Maybe is a zeropoint problem?
Help !

linno’s picture

FileSize
556.25 KB
567.89 KB
467.16 KB

Ok...
so... after many tests now my menu magically work, but after the patch I tried only first called menu item works, if I click the second item on sub-menu not work fine, lost active-trails in first menu parent, why?
Attach images to display better.
again sorry for my bad english
thanks.

xjm’s picture

@linno, you should open a separate support issue for that in the superfish module. This issue is specifically about the patch above. Thanks.

webchick’s picture

Wow!

Thanks SO much for the awesome issue summary, and for the thoughtful and thorough work/discussion on the patch here. I can't believe you managed to fix this bug in a backwards-compatible way. AWESOME!

The constant hash thing is pretty weird, but it has sign-off from Peter. I did leave a message for chx on #drupal-contribute to see if he had anything more substantial to say about this patch beyond the fact that it needed tests. I'll commit this in 24 hours or so, barring any OMG!! emergencies.

chx’s picture

II do not see anything OMG world ending in there.

benjf’s picture

the patch in comment #262 works for me. thanks!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

All right! Heeeere we go...

Committed and pushed to 7.x. WOOHOO!! Thank you SO much for all of your great work on this one, folks!!

kerios83’s picture

THX, guys I have been waiting for this a looong time :)

Tor Arne Thune’s picture

What a great christmas gift (for us geeks) :) Thanks to all the testers, patchers and committers!

wickedskaman’s picture

Big ups everyone! What an annoying complicated little guy. It's always the seemingly simple things isn't it? :)

Summit’s picture

Hi, because this patch is so important, shouldn't a Drupal 7.11 be based on this?
I am not happy of bringing a .dev version into production and would love a stable version with this patch please.
Thanks a lot in advance for considering this.
greetings, Martijn

JamesOakley’s picture

@Summit: If a new Drupal core release were made every time a "major" bug were fixed, it would become impossible to maintain a website that runs on Drupal - we'd all be forever upgrading core.

This is a patch that does not affect every site. If yours is a site that is affected, there are two options: (i) Wait for Drupal 7.11 to come out (which will not be too long), or (ii) install Drupal 7.10 and then apply this one patch.

You are quite right - I wouldn't run a live site on a -dev release either. But there's no need. The current 7.x-dev is ahead of 7.10 by many patches to fix many issues. If it's just this one patch you need, apply just this one patch.

Summit’s picture

Hi James, I will do. Thanks for your update. Greetings, Martijn

Summit’s picture

FileSize
51.02 KB

Hi, Patch #262 worked for me also. See attached .zip file with all changed files.
Menu.inc off course in folder includes, the rest in folder modules/menu.

Edit: Somehow for one site it works..and for other site it doesn't...will need to investigate more..anyone else has problems still?
EDIT2: I have the problem..the one site that doesn't get "active-trail" has i18n Menu translation 7.x-1.3 installed. Disabling that module solved my problem.

Greetings, Martijn

xjm’s picture

@Summit -- Please see the summary. The issue with i18n can be resolved with the patch in #1351678: Follow menu_link_get_preferred active trail handling for custom menus. Also, this patch has already been committed, so folks can use 7.x-dev until 7.11 is released.

JamesOakley’s picture

Martijn

As per the remarks at the top of this issue (before you get into all the comments), i18n needs special treatment for other reasons, and that is being handled in an issue (#1351678: Follow menu_link_get_preferred active trail handling for custom menus) for the internationalization module.

Just for the record - there's no need to post a zip file of files changed. That is what the .patch file is - only it also details which parts of a file have changed, meaning that it can be applied even if there have been other changes to other parts of the document.

Summit’s picture

Thanks guys, Drupal rocks! Martijn

Martin Mayer’s picture

When I use the patch from #262, the child links in the main menu disappear. They come back when I undo the patch.

aspilicious’s picture

Martin don't tell me you're using the i18n module...

Dave Reid’s picture

mollask’s picture

Version: 7.x-dev » 7.10
Priority: Major » Normal

Hi N00b here (or N00b012579 depending on how many of us you have)

Does this patch go into the /modules/menu dir? I'm also wondering if the D7.12 is still unhappy with secondary nav. D7 keeps telling me it's an imperative that I upgrade. Don't want to if this isn't resolved in D7.12.

xjm’s picture

Version: 7.10 » 7.x-dev
Priority: Normal » Major

Please don't change the status settings as they apply to the whole issue. This issue is fixed in 7.12, so if you upgrade core and your contrib modules to the latest versions you do not need this patch.

mollask’s picture

Woops. And sorry. Hope I didn't f-things up too bad. Thanks for replying. I might be able to breath better now.

Off to upgrade now.

pmitsos’s picture

Status: Fixed » Needs review

#6: 942782-6-custom_mens.patch queued for re-testing.

Tor Arne Thune’s picture

Status: Needs review » Fixed
testtest23112’s picture

Status: Fixed » Needs review
Issue tags: -Needs backport to D7

#262: d7-942782-262.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs backport to D7

The last submitted patch, d7-942782-262.patch, failed testing.

Tor Arne Thune’s picture

Status: Needs work » Fixed

I can't begin to understand the reasoning behind re-testing these patches, so I assume it's a mistake or misunderstanding.

testtest23112’s picture

Sorry, indeed this was my mistake. I apologise.

bvanmeurs’s picture

Status: Fixed » Needs work

In Drupal 7.12 I still have this issue and ended up here. I can see that some code has been hooked in on custom menu save and delete. I think the fact that I still have this problem is due to the fact that I deleted the 'menu_default_active_menus' because in bug hunting I wanted to rebuild this. This had the undesirable consequence that all my custom menu's are not active any more. Now I have to manually set the variable again..

In short, imho the current solution (doing changes to the to menu_default_active_menus veriable in the menu update and delete hooks) is not robust enough. And in fact, I think it's a bad solution in more ways.

Some considerations to the patchers:
* Why is this variable necessary if any custom module is added when it is first saved?
* In what situation would I not want to my menu to have an active trail? Is this situation one that only applies to programmers that can edit a Drupal variable (which is in itself very tricky).
* Of course, the question above is 'no: also GUI users and front-enders want to be able to disable the active trail on a particular menu'. Then why not just providing a GUI checkbox (in the menu edit screen) to make a menu active or inactive? Seems like a simple thing to do to me and a much better solution to the problem.

xjm’s picture

Status: Needs work » Fixed

@bvanmeurs -- Could you open a followup issue? Thanks.

dev_007’s picture

Status: Fixed » Needs review
webchick’s picture

Status: Needs review » Fixed
magpie5212’s picture

Is there a follow up? I have the same problem in 7.12 (just upgraded this morning).

See: http://www.southamonline.org.uk/archive.

The sub-menu shows when you are on the page, but not when on the parent.

xjm’s picture

Please try clearing your site cache and see if that helps; otherwise open or look for a separate issue and link it here. It could be caused by a contributed module so you might want to confirm steps to reproduce in the new issue (starting from "Install Drupal core with the X profile").

magpie5212’s picture

Built a new site from scratch and it works. So now to install modules one at a time until it fails.

magpie5212’s picture

I have pinned this down to the menu items for the link . Sub-items under that link do no show. Other sub-items are OK.

xjm’s picture

Again, please post a separate issue.

magpie5212’s picture

quantos’s picture

I still have a problem with the */user active trail. Neither active nor active trail markers are being generated for the user item but they all for all the other menu block items.

I have a Drupal 7.12, Menu Block 7.23 and an Omega/Delta/Context build. My temp, low-tech solution was simply to apply a hard-code active style fix via css :

body.page-user div.menu-block-3 li.menu-mlid-2 a { xxx }

That might help anyone needing a quick, easy fix without too many implications when the issue is fully resolved. The other thing I've noticed is that the system doesn't seem to be generating an active state at all for */user A tag. I.E. any css mark-up using a:active is also completely ignored.

I hope that helps someone.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

jddeli’s picture

Version: 7.x-dev » 7.12
Assigned: Unassigned » jddeli
Status: Closed (fixed) » Active

I still have a problem in Drupal 7.12.
This is still a problem.
We can not work with Drupal 7 all of us who have websites in other countries.
There is no reason for us to use the version 7 of Drupal with so many problems.
When there is a stable version that works then might be use it .

JamesOakley’s picture

Version: 7.12 » 7.x-dev
Assigned: jddeli » Unassigned
Status: Active » Closed (fixed)

@jddeli - Sorry to hear you're still having problems with Drupal 7.12.

The particular bug dealt with in this issue has been fixed. The solution was committed to the Drupal code repository on Christmas Day (oh yes!), and 7.12 came out in February of this year. I'm sure Drupal 7.12 has some bugs in it, but this exact issue is not one of them.

(Just to explain, in case you weren't clear, the "Assigned" attribute of an issue is for code maintainers to assign the person who will work on a solution to the issue in question. I don't think you meant to put yourself down as working on this specific bug fix).

I suggest you either open a new issue in the Drupal Core issue queue, explaining the exact problem you're having. Or, if you're not sure, you could give us all a little more to go on - either here or in the forums section of drupal.org. If you say the issue relates to other countries, it may well be that your problem relates to the i18n module, but it's hard to say unless you try and detail the problem a little more exactly.

jddeli’s picture

Assigned: Unassigned » jddeli

I have in includes/menu.inc on line 2300
$active = variable_get('menu_default_active_menus', array_keys(menu_list_system_menus()));
I changed it
$active = variable_get('menu_default_active_menus', array_keys(menu_get_menus()));
and it works
Sorry JamesOakley i certainly have i18n module because i have drupal in multilanguage sites.

JamesOakley’s picture

OK, great - but before trying to verify that you've fixed the problem, it would be really helpful to know precisely what problem you've got.

jddeli’s picture

Assigned: Unassigned » jddeli

I make a test site for pentlis municipality.
I made a block menu to place the menu wrote.
Of course I made multilingualism Greek and English.
http://penteli1.smart2shop.gr
You can see now that the menus are stay open when i choose them.

MustangGB’s picture

Assigned: jddeli » Unassigned
Nor4a’s picture

Assigned: jddeli » Unassigned

Yess! This from #329

$active = variable_get('menu_default_active_menus', array_keys(menu_get_menus()));

saved my life :)
Thanks!

Evoksha’s picture

We are also seeing this issue in 7.12 (menu block installed). The issue is simply that child menu items do not expand. Due to the number of people still reporting the issue in 7.12, I would recommend that the issue be reopened.

TelFiRE’s picture

Issue is still there in 7.14. Active-trail is completely inconsistant.

broon’s picture

I second this.
On a new project with Drupal 7.14, I were using the Superfish module for menus. However, it didn't behave like I expected and I first thought it to be an error in that module (see this issue). But then I dropped Superfish and just used the custom module and it turns out the "active-trail" class is only added if the currently displayed content is a view. If the content is a node, all "active-trail" classes are missing.

It might be a simple solution, please take a look at http://www.designhammer.com/blog/active-trails-menu-items-drupal-7
The two lines of PHP code mentioned in that article made all the menus work as I expected, whether it be the default Drupal core menu type, Menu block menus or even Superfish.

CowTownFarmBoy’s picture

Version: 7.x-dev » 7.14
Status: Closed (fixed) » Needs work
FileSize
65.3 KB
92.64 KB
93.37 KB

I upgraded from 6.26 to 7.14 and have the active-trail problem. I have Danland Silverfish running and all the links appear on the menu as they should. All the hover works perfect. All the parent and child entries are there and functional. Every time I click on any item I go exactly where I should and expect to go.

Ready?

However the first element in the menu Home which is the front page never has the active-trail set at all ever. And when I select the last menu item on the far right it will not become the active-trail in the HTML instead the second entry from the left gets the active-trail set. I have changed the line of code mentioned above and that did nothing. I have attached three images from this problem to illustrate what is happening with the firebug HTML included in the image.

With what I am seeing I cannot say this is fixed, but should be re-opened and become again an active problem.

BTW the one solution that has been proposed was to revert back to 7.10 and not move forward till this is truly addressed in an upgrade. What would happen if I did try going back wouldn't there be problems with the database etc...

JamesOakley’s picture

Version: 7.14 » 7.x-dev
Status: Needs work » Closed (fixed)

CowTownFarmBoy: What you're describing here may be related to this issue. However, after an issue has been marked as fixed for some time, the way to report a problem like yours is to open a new issue and make a reference back to this one. That way you are more likely to get help. There is also a strong likelihood that the symptoms you have may be similar but the issue is actually different.

CowTownFarmBoy’s picture

Thanks James for clearing up the process of how this should be done. I appreciate the help.

I have created a new issue called Drupal core upgrade from 6.26 to 7.14 causes menu block to fail. The only item on the menu that currently fails is the Home tab. All others are now working. There is more on the new issue. Which as far as I can tell is still an issue. I have read the patch as posted here and the core .menu.inc and find the patch included. However it still does not work in my case.

Again thanks for the help with process. The link to the new issue is http://drupal.org/node/1660916

NenadP’s picture

Is this problem still present in Drupal 7.14 ? I am having issues - menu does not remember position on page reload - but only with url-s that have extra php variables ie http://mydomain.com/category?var1=value&var2=value2

yang_yi_cn’s picture

yang_yi_cn’s picture

Issue summary: View changes

(xjm)