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

    Only local images are allowed.

  • Visiting Foo

    Only local images are allowed.

  • Visiting Bar:

    Only local images are allowed.

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

    Only local images are allowed.

  • Visiting Bar:

    Only local images are allowed.

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.

Files: 
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
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d7-942782-262.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#262 interdiff-248-262.txt1.54 KBxjm
#257 d7-942782-257.patch10.21 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 37,307 pass(es).
[ View ]
#257 interdiff.txt452 bytesxjm
#248 custom-active-trail-942782-248.patch10.26 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch custom-active-trail-942782-248.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#248 interdiff-205-248.txt1.78 KBxjm
#243 custom-active-trail-942782-242.patch10.21 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 33,980 pass(es), 0 fail(s), and 1,023 exception(es).
[ View ]
#243 interdiff-205-242.txt1.9 KBxjm
#241 custom-active-trail-942782-241.patch10.21 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 34,136 pass(es).
[ View ]
#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
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch per-menu-preferred-links-942782-212-D7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#205 per-menu-preferred-links-942782-205-test-only.patch2.46 KBTor Arne Thune
FAILED: [[SimpleTest]]: [MySQL] 33,706 pass(es), 41 fail(s), and 294 exception(es).
[ View ]
#205 per-menu-preferred-links-942782-205.patch9.58 KBTor Arne Thune
PASSED: [[SimpleTest]]: [MySQL] 34,100 pass(es).
[ View ]
#196 expandedCustomMenu.png5.29 KBTor Arne Thune
#185 per_menu_preferred_links-942782-186.patch7.59 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 33,146 pass(es).
[ View ]
#184 per_menu_preferred_links-942782-185-incomplete.patch6.93 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 33,150 pass(es).
[ View ]
#184 per_menu_preferred_links-942782-includes--menu.inc_.rej_.txt569 bytespillarsdotnet
#172 per_menu_preferred_links-942782-172-d7.patch8.73 KBpillarsdotnet
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch per_menu_preferred_links-942782-172-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#171 per_menu_preferred_links-942782-171-d7.patch0 bytespillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 38,066 pass(es).
[ View ]
#170 custom_menus_receive_active_trail-942782-170-d7.patch8.58 KBcolan
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch custom_menus_receive_active_trail-942782-170-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#167 per_menu_preferred_links-942782-167-d7.patch8.79 KBmelon
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch per_menu_preferred_links-942782-167-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#160 per_menu_preferred_links-942782-160-d7.patch8.79 KBpillarsdotnet
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch per_menu_preferred_links-942782-160-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#160 per_menu_preferred_links-942782-160.patch7.52 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 32,915 pass(es).
[ View ]
#146 per_menu_preferred_links-942782-146-d7.patch8.71 KBdstol
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch per_menu_preferred_links-942782-146-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#145 per_menu_preferred_links-942782-145-d7.patch8.71 KBpillarsdotnet
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch per_menu_preferred_links-942782-145-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#133 per_menu_preferred_links-942782-133-d7.patch8.1 KBJames Andres
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch per_menu_preferred_links-942782-133-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#129 per_menu_preferred_links-942782-129.patch6.94 KBjn2
PASSED: [[SimpleTest]]: [MySQL] 33,608 pass(es).
[ View ]
#101 per_menu_preferred_links-942782-101.patch8.73 KBpillarsdotnet
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch per_menu_preferred_links-942782-101.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#101 per_menu_preferred_links-942782-101-d7.patch8.77 KBpillarsdotnet
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch per_menu_preferred_links-942782-101-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#88 942782-88-per-menu-preferred-link.patch8.11 KBjrchamp
PASSED: [[SimpleTest]]: [MySQL] 29,717 pass(es).
[ View ]
#70 942782-70-per-menu-preferred-link.patch8.02 KBjrchamp
PASSED: [[SimpleTest]]: [MySQL] 31,599 pass(es).
[ View ]
#43 942782-43-per-menu-preferred-link.patch8.22 KBJohnAlbin
PASSED: [[SimpleTest]]: [MySQL] 31,539 pass(es).
[ View ]
#42 942782-41-custom-menus-expanding.patch5.55 KBtimhilliard
FAILED: [[SimpleTest]]: [MySQL] 31,443 pass(es), 47 fail(s), and 0 exception(es).
[ View ]
#41 942782-custom-menus-expanding.patch5.56 KBtimhilliard
FAILED: [[SimpleTest]]: [MySQL] 31,540 pass(es), 6 fail(s), and 0 exception(es).
[ View ]
#39 942782-39-per-menu-preferred-link.patch7.45 KBJohnAlbin
FAILED: [[SimpleTest]]: [MySQL] 31,207 pass(es), 59 fail(s), and 13,210 exception(es).
[ View ]
#22 942782-per-menu-preferred-link.patch3.06 KBDamien Tournoud
FAILED: [[SimpleTest]]: [MySQL] 31,542 pass(es), 0 fail(s), and 4 exception(es).
[ View ]
#17 custom-menu-d7-regression.png48.76 KBJohnAlbin
#14 Bild 13.png27.67 KBcriz
#6 942782-6-custom_mens.patch2.33 KBbecw
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 942782-6-custom_mens.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#4 942782-4-custom_menus.patch2.66 KBbecw
FAILED: [[SimpleTest]]: [MySQL] 26,208 pass(es), 196 fail(s), and 15 exception(es).
[ View ]
#2 942782-3-custom_menus.patch1.41 KBbecw
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 942782-3-custom_menus.patch. See the log in the details link for more information.
[ View ]
custom_menus.patch1.41 KBbecw
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch custom_menus.patch. See the log in the details link for more information.
[ View ]

Comments

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new1.41 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 942782-3-custom_menus.patch. See the log in the details link for more information.
[ View ]

Fixed a typo.

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.

Status:Needs work» Needs review
StatusFileSize
new2.66 KB
FAILED: [[SimpleTest]]: [MySQL] 26,208 pass(es), 196 fail(s), and 15 exception(es).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new2.33 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 942782-6-custom_mens.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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.

Priority:Major» Critical

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.

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).

#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.

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...

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"?

StatusFileSize
new27.67 KB

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

Status:Needs review» Closed (duplicate)

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. :-(

Title:Custom menus never appear in active trailCustom menus never receive an active trail
StatusFileSize
new48.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.

subscribing.

Priority:Critical» Major

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

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.

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.

Status:Needs work» Needs review
StatusFileSize
new3.06 KB
FAILED: [[SimpleTest]]: [MySQL] 31,542 pass(es), 0 fail(s), and 4 exception(es).
[ View ]

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.

+++ 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.

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.

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.

@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.

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).

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

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

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.

subscribing

subscribing.

subscribing

Subscribing

Subscribing

Subscribe.

Status:Needs work» Needs review
StatusFileSize
new7.45 KB
FAILED: [[SimpleTest]]: [MySQL] 31,207 pass(es), 59 fail(s), and 13,210 exception(es).
[ View ]

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.

StatusFileSize
new5.56 KB
FAILED: [[SimpleTest]]: [MySQL] 31,540 pass(es), 6 fail(s), and 0 exception(es).
[ View ]

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.

StatusFileSize
new5.55 KB
FAILED: [[SimpleTest]]: [MySQL] 31,443 pass(es), 47 fail(s), and 0 exception(es).
[ View ]

whoops, error with last patch.

Status:Needs work» Needs review
StatusFileSize
new8.22 KB
PASSED: [[SimpleTest]]: [MySQL] 31,539 pass(es).
[ View ]

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.

@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.

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. ;-)

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.

Subscribing

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

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:

<?php
+    $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;.

Status:Needs review» Needs work

Subscribing

subscribing

subscribing

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

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?

@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

Thanks a lot man! This worked great.

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.

Version:7.0» 7.x-dev

Fixes go against dev => 7.1 (hopefully)

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

Subscribing.

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.

Subscribing

subscribing

Status:Needs work» Needs review

subscribing

subscribing

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.

subscribing

subscribing

StatusFileSize
new8.02 KB
PASSED: [[SimpleTest]]: [MySQL] 31,599 pass(es).
[ View ]

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.

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.

Subscribe

custom_menus.patch queued for re-testing.

custom_menus.patch queued for re-testing.

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

Subscribe, got bitten by tha bug :)

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

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.

@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

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

subscribe

Subscribe

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.

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

StatusFileSize
new8.11 KB
PASSED: [[SimpleTest]]: [MySQL] 29,717 pass(es).
[ View ]

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.

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.

Issue tags:+needs backport to D7

Tagging

Subscribe

Status:Needs work» Needs review

The new patch in #88 needs some review.

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!

@#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

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

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.

Got this issue too, subscribe.

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?

subscribing...

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

StatusFileSize
new8.77 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch per_menu_preferred_links-942782-101-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new8.73 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch per_menu_preferred_links-942782-101.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

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

@#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.

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).

Subscribing... and apologizing for the pollution.

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.

Issue tags:+needs backport to D7

Subscribing

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

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

Subscribing

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

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.

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.

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.

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

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

@kerios83

That is what I had to do.

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

Status:Needs review» Needs work

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

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

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.

Subscribing as this affects D7 taxonomy menu.

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 ?

#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.

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

@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.

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.

StatusFileSize
new6.94 KB
PASSED: [[SimpleTest]]: [MySQL] 33,608 pass(es).
[ View ]

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.

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.

Tagging.

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.

StatusFileSize
new8.1 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch per_menu_preferred_links-942782-133-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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).

*ignore this*

Subscribing. Agree with #134.

subscribe

subscribe

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

subscribe

subscribe

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.

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

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.

StatusFileSize
new8.71 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch per_menu_preferred_links-942782-145-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

StatusFileSize
new8.71 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch per_menu_preferred_links-942782-146-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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) {

Version:8.x-dev» 7.7

subscribing

Version:7.7» 8.x-dev

Please don't change the version.

@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.

subscribe

subscribe

custom_menus.patch queued for re-testing.

duplicate

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

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.

@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.

sub

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

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...

Status:Needs work» Needs review
StatusFileSize
new7.52 KB
PASSED: [[SimpleTest]]: [MySQL] 32,915 pass(es).
[ View ]
new8.79 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch per_menu_preferred_links-942782-160-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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 ...

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.

subscribe

@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

+1 sub

StatusFileSize
new8.79 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch per_menu_preferred_links-942782-167-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

subscribing

subscribe

StatusFileSize
new8.58 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch custom_menus_receive_active_trail-942782-170-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

StatusFileSize
new0 bytes
PASSED: [[SimpleTest]]: [MySQL] 38,066 pass(es).
[ View ]

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.

StatusFileSize
new8.73 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch per_menu_preferred_links-942782-172-d7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

For real, this time.

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.

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

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.

#172 did the job, thank you!

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

Sub

Status:Reviewed & tested by the community» Needs review

CNR until there's an answer to #177.

Status:Needs review» Reviewed & tested by the community

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

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');

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

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

Status:Needs review» Needs work
StatusFileSize
new569 bytes
new6.93 KB
PASSED: [[SimpleTest]]: [MySQL] 33,150 pass(es).
[ View ]

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.

StatusFileSize
new7.59 KB
PASSED: [[SimpleTest]]: [MySQL] 33,146 pass(es).
[ View ]

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

Status:Needs work» Needs review

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

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?

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.

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?

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.

eek...sorry, subscribe.

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.

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.

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.

patch in #185 worked for me, subscribing

StatusFileSize
new5.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.

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.

@#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;

Status:Needs review» Reviewed & tested by the community

Ready to go?

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.

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."

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.

drupal.org is version 7, right?

Not yet, still v6

Status:Needs work» Needs review
StatusFileSize
new9.58 KB
PASSED: [[SimpleTest]]: [MySQL] 34,100 pass(es).
[ View ]
new2.46 KB
FAILED: [[SimpleTest]]: [MySQL] 33,706 pass(es), 41 fail(s), and 294 exception(es).
[ View ]

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.

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

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

Status:Needs review» Reviewed & tested by the community

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

Status:Reviewed & tested by the community» Needs review

We need to check 209 first

@#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?

StatusFileSize
new10.62 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch per-menu-preferred-links-942782-212-D7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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.

@#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.

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.

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.

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?

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.

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.

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.

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

@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.

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.

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

Perry.

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?

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

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!

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.

#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. :)

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.

StatusFileSize
new3.37 KB
new3.3 KB
new3.02 KB
new2.92 KB
new3.72 KB

Some before/after screenshots for the summary.

Issue summary:View changes

(xjm) First outline of a summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

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.

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).

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

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');

Status:Reviewed & tested by the community» Needs work

Assigned:Unassigned» xjm

I'll roll with #238.

Status:Needs work» Needs review
StatusFileSize
new1.91 KB
new10.21 KB
PASSED: [[SimpleTest]]: [MySQL] 34,136 pass(es).
[ View ]

Assigned:xjm» Unassigned

Status:Needs review» Needs work
StatusFileSize
new1.9 KB
new10.21 KB
FAILED: [[SimpleTest]]: [MySQL] 33,980 pass(es), 0 fail(s), and 1,023 exception(es).
[ View ]

Uhm. Without wrapping a constant in quotes. Derp.

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

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.

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?

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

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

Assigned:xjm» Unassigned
Status:Needs work» Needs review
StatusFileSize
new1.78 KB
new10.26 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch custom-active-trail-942782-248.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

With the constant defined in the correct file.

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

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.

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.

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

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

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.

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

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.

Status:Patch (to be ported)» Needs review
StatusFileSize
new452 bytes
new10.21 KB
PASSED: [[SimpleTest]]: [MySQL] 37,307 pass(es).
[ View ]

Rerolled for D7 with define().

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

Status:Needs review» Needs work

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

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.

Status:Reviewed & tested by the community» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new1.54 KB
new11.3 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d7-942782-262.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Looks good now with the update function.

Status:Needs review» Reviewed & tested by the community

Hay lam

CUng duoc

hay qua di mat

Status:Reviewed & tested by the community» Needs review

#262 patch works for me.

Thanks!

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

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

@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.

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.

StatusFileSize
new3.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.

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

works for me on d7 dev

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 ?

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.

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

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

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!

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

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

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

@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.

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 !

StatusFileSize
new556.25 KB
new567.89 KB
new467.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.

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

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.

II do not see anything OMG world ending in there.

Pages