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
- Create a custom menu
- Create two nodes, Foo and Bar, and add them to the custom menu with Bar as a child of Foo.
- Put the menu block for the custom menu in a sidebar.
- Click on Foo. Bar is not displayed.
- Visit Bar. The menu item for Foo is not active or expanded.
-
Menu structure
-
Visiting Foo
-
Visiting Bar:
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
-
Visiting Bar:
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):
- _menu_translate() is now called from within a new nested loop, when previously it was called one layer out.
- On saving each new menu, there's a new variable_get() followed by a new variable_set()
- 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
- The i18n module relies on the pre-patch static cache structure in menu_link_get_preferred(). A patch for i18n supporting this patch is available in #1351678: Follow menu_link_get_preferred active trail handling for custom menus.
- The Menu block module already includes a workaround for this issue, so it may need to be updated once this patch is committed.
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 tomenu_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:
- Go to admin/structure/menu and add a new menu.
- Add the new menu block to the sidebar so you can see the menu's active state.
- Create a node and place it in the menu, using the 'menu settings' on the node form.
- Create a second node and place it in the menu, with the first node as its parent item.
- 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 tomenu_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.
Comment | File | Size | Author |
---|---|---|---|
#337 | Superfish6_2012.JPG | 93.37 KB | CowTownFarmBoy |
#337 | Superfish26_2012.JPG | 92.64 KB | CowTownFarmBoy |
#337 | Superfish36_2012.JPG | 65.3 KB | CowTownFarmBoy |
#298 | menu.zip | 51.02 KB | Summit |
#286 | print-2.png | 467.16 KB | linno |
Comments
Comment #2
becw CreditAttribution: becw commentedFixed a typo.
Comment #3
JohnAlbinNice 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.Comment #4
becw CreditAttribution: becw commentedOk, 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'.
Comment #6
becw CreditAttribution: becw commentedUsing 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.
Comment #7
crizGreat, 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.
Comment #8
crizBecause of http://drupal.org/node/950034#comment-3614150
Comment #9
Damien Tournoud CreditAttribution: Damien Tournoud commentedNot 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.
Comment #10
crizOkay, 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:
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).
Comment #11
Damien Tournoud CreditAttribution: Damien Tournoud commented#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.
Comment #12
crizSo #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...
Comment #13
becw CreditAttribution: becw commentedre #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"?
Comment #14
crizThis bug seems not to appear with alpha 7. See http://drupal.org/node/950034#comment-3618858
Comment #15
sunLooks like a duplicate of #950034: Using the same source for main/secondary with a custom menu doesn't work
Comment #16
JohnAlbinIf #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.
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. :-(
Comment #17
JohnAlbinPer 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:
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.
Comment #18
mfer CreditAttribution: mfer commentedsubscribing.
Comment #19
webchickI can see this is nasty if you run into it, but I don't see how it is critical.
Comment #20
sunI'm not sure whether this expectation is entirely correct. As Damien already explained:
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.
Comment #21
mfer CreditAttribution: mfer commentedThere 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.
Comment #22
Damien Tournoud CreditAttribution: Damien Tournoud commentedSo 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.
Comment #23
sunDoesn'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.
Comment #24
Damien Tournoud CreditAttribution: Damien Tournoud commentedNo, but does it really matters? The global, unique, preferred link is still used to build the breadcrumb.
Comment #26
mfer CreditAttribution: mfer commentedMaybe 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.
Comment #27
Damien Tournoud CreditAttribution: Damien Tournoud commented@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.
Comment #28
JohnAlbinI 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):
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):
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.
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.)
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).
Comment #29
wickedskaman CreditAttribution: wickedskaman commentedI 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
Comment #30
pixelwhip CreditAttribution: pixelwhip commentedThe ability to style the active trail of custom menus would greatly improve their usability for end users. Subscribing.
Comment #31
JohnAlbinwickedskaman'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.
Comment #32
amanaplan CreditAttribution: amanaplan commentedsubscribing
Comment #33
amanaplan CreditAttribution: amanaplan commentedMarked #968878: Custom menus do not expand past top level menu items in blocks as a duplicate of this issue.
Comment #34
usaussie CreditAttribution: usaussie commentedsubscribing.
Comment #35
timhilliard CreditAttribution: timhilliard commentedsubscribing
Comment #36
anavarreSubscribing
Comment #37
BenK CreditAttribution: BenK commentedSubscribing
Comment #38
geerlingguy CreditAttribution: geerlingguy commentedSubscribe.
Comment #39
JohnAlbinOk. 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.
Comment #41
timhilliard CreditAttribution: timhilliard commentedHi 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
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.
Comment #42
timhilliard CreditAttribution: timhilliard commentedwhoops, error with last patch.
Comment #43
JohnAlbinI 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.
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.
Comment #44
timhilliard CreditAttribution: timhilliard commented@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.
Comment #45
JohnAlbinThat'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. ;-)
Comment #46
JohnAlbinThe 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.
Comment #47
W32 CreditAttribution: W32 commentedSubscribing
Comment #48
W32 CreditAttribution: W32 commentedStandard "Primary link" and "Secondary link" menu blocks are not working too, after update site from D6 to D7.
Comment #49
Damien Tournoud CreditAttribution: Damien Tournoud commentedI 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:
We should rather change the return value to
return isset($preferred_links[$path][$menu_name]) ? $preferred_links[$path][$menu_name] : FALSE;
.Comment #50
sunComment #51
killer_tilapia CreditAttribution: killer_tilapia commentedSubscribing
Comment #52
jn2 CreditAttribution: jn2 commentedsubscribing
Comment #53
Shmee CreditAttribution: Shmee commentedsubscribing
Patch in #43 works for me. And everything looks good.
Comment #54
Shmee CreditAttribution: Shmee commentedAnybody 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?
Comment #55
becw CreditAttribution: becw commented@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
Comment #56
Shmee CreditAttribution: Shmee commentedThanks a lot man! This worked great.
Comment #57
NewZeal CreditAttribution: NewZeal commentedThis 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.
Comment #58
Alan D. CreditAttribution: Alan D. commentedFixes go against dev => 7.1 (hopefully)
Comment #59
prezidentchimp CreditAttribution: prezidentchimp commentedCount me in as someone else who is looking forward to this menu problem being taken care of.
Comment #60
dropcube CreditAttribution: dropcube commentedSubscribing.
Comment #61
jlab CreditAttribution: jlab commentedSubscribing. 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.
Comment #62
Monzer Emam CreditAttribution: Monzer Emam commentedSubscribing
Comment #63
wbrain CreditAttribution: wbrain commentedsubscribing
Comment #64
anydigital CreditAttribution: anydigital commented#43: 942782-43-per-menu-preferred-link.patch queued for re-testing.
Comment #65
pthite CreditAttribution: pthite commentedsubscribing
Comment #66
anydigital CreditAttribution: anydigital commentedsubscribing
Comment #67
jrchamp CreditAttribution: jrchamp commentedIt 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.
Comment #68
Dmitriy.trt CreditAttribution: Dmitriy.trt commentedsubscribing
Comment #69
carn1x CreditAttribution: carn1x commentedsubscribing
Comment #70
jrchamp CreditAttribution: jrchamp commentedThis 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.
Comment #71
markabur CreditAttribution: markabur commentedI 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.
Comment #75
paragkenia CreditAttribution: paragkenia commentedSubscribe
Comment #76
sbatyrov CreditAttribution: sbatyrov commentedcustom_menus.patch queued for re-testing.
Comment #77
torrance123 CreditAttribution: torrance123 commentedcustom_menus.patch queued for re-testing.
Comment #78
torrance123 CreditAttribution: torrance123 commentedThe patch in comment 70 works for me with no problems. Thanks for your work!
Comment #79
aspilicious CreditAttribution: aspilicious commentedSubscribe, got bitten by tha bug :)
Comment #80
bmarcotte CreditAttribution: bmarcotte commentedThank 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
Comment #81
skeates CreditAttribution: skeates commentedSorry 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.
Comment #82
jrchamp CreditAttribution: jrchamp commented@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
Comment #83
skeates CreditAttribution: skeates commentedThank you that worked a charm. Seem to be learning new things every day.
Comment #84
vito_swat CreditAttribution: vito_swat commentedsubscribe
Comment #85
andros CreditAttribution: andros commentedSubscribe
Comment #86
pillarsdotnet CreditAttribution: pillarsdotnet commentedReview of #70, which still applies cleanly to 8.x:
Please consider changing to:
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.
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.Again, why not just use an empty string?
That should be
NULL
rather thanFALSE
to match the doxygen comments at the top of the function:Comment #87
pillarsdotnet CreditAttribution: pillarsdotnet commentedComment #88
jrchamp CreditAttribution: jrchamp commentedI 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.
Comment #89
bowersox CreditAttribution: bowersox commentedWhy 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.
Comment #90
pillarsdotnet CreditAttribution: pillarsdotnet commentedTagging
Comment #91
Greg Varga CreditAttribution: Greg Varga commentedSubscribe
Comment #92
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedThe new patch in #88 needs some review.
Comment #93
Tommy Kaneko CreditAttribution: Tommy Kaneko commentedSEE 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!
Comment #94
pillarsdotnet CreditAttribution: pillarsdotnet commented@#88
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
Comment #95
Matt Townsend CreditAttribution: Matt Townsend commentedIs there any news on getting this fix into 7.x?
Comment #96
pillarsdotnet CreditAttribution: pillarsdotnet commentedI 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):
You can help by:
Comment #97
MurzGot this issue too, subscribe.
Comment #98
MurzI 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?
Comment #99
r_honey CreditAttribution: r_honey commentedsubscribing...
Comment #100
jlab CreditAttribution: jlab commentedI have also tested the patch from #88 on my D7 sites and the menus is now working as expected.
Comment #101
pillarsdotnet CreditAttribution: pillarsdotnet commented#88 re-rolled for current d8 and d7 using
git format-patch
. No code changes.Comment #102
star-szr@#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.
Comment #103
gagarine CreditAttribution: gagarine commentedWith 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).
Comment #104
lee20 CreditAttribution: lee20 commentedSubscribing... and apologizing for the pollution.
Comment #105
lee20 CreditAttribution: lee20 commentedArg.. 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.
Comment #106
pillarsdotnet CreditAttribution: pillarsdotnet commentedComment #107
stewart.adam CreditAttribution: stewart.adam commentedSubscribing
Comment #108
timcosgrove CreditAttribution: timcosgrove commentedSubscribing. We need custom menus to behave as one would expect. Will hammer at this and mark it RBTC if it seems sound.
Comment #109
mabuweb CreditAttribution: mabuweb commentedPatch in #88 doesn't work for me in D7.4
Comment #110
derMatze CreditAttribution: derMatze commentedSubscribing
Comment #111
MustangGB CreditAttribution: MustangGB commentedTested #101 in D7.4
Doesn't work for
<front>
Comment #113
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedAll 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.
Comment #114
kerios83 CreditAttribution: kerios83 commentedI'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.
Comment #115
bitcore CreditAttribution: bitcore commentedPatch 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.
Comment #116
mabuweb CreditAttribution: mabuweb commentedPatch posted in #101 is works for me too in Drupal 7.4. if applied manually.
Thanks
Comment #117
kerios83 CreditAttribution: kerios83 commentedYep I can confirm that #101 - per_menu_preferred_links-942782-101-d7.patch works fine.
Comment #118
Shmee CreditAttribution: Shmee commented@kerios83
That is what I had to do.
Comment #119
sonar_un CreditAttribution: sonar_un commentedSubscribe, can't wait until this gets committed into D7
Comment #120
MustangGB CreditAttribution: MustangGB commentedAs I mentioned in #111 patch at #101 doesn't work for
<front>
Comment #121
Yanivs CreditAttribution: Yanivs commentedInstalled 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
Comment #122
joostvdl CreditAttribution: joostvdl commentedDefault Patch from #101 results in :
Further it works. Not tested the option.
Comment #123
dstolSubscribing as this affects D7 taxonomy menu.
Comment #124
kerios83 CreditAttribution: kerios83 commentedOk, 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 ?
Comment #125
spamator12 CreditAttribution: spamator12 commented#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.
Comment #126
aspilicious CreditAttribution: aspilicious commentedWhat does not work? Doesn't it get applied (the patch) or do you get errors or warnings?
Comment #127
spamator12 CreditAttribution: spamator12 commented@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.
Comment #128
jn2 CreditAttribution: jn2 commentedIt 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:
Looks like the third hunk fails.
Comment #129
jn2 CreditAttribution: jn2 commentedHere'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.
Comment #130
jn2 CreditAttribution: jn2 commentedI 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.Comment #131
xjmTagging.
Comment #132
bitcore CreditAttribution: bitcore commentedFor 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.
Comment #133
James Andres CreditAttribution: James Andres commentedApologies 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.Comment #134
dcotruta CreditAttribution: dcotruta commentedSubscribing. 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).
Comment #135
aspilicious CreditAttribution: aspilicious commented*ignore this*
Comment #136
Aeternum CreditAttribution: Aeternum commentedSubscribing. Agree with #134.
Comment #137
sleepingmonksubscribe
Comment #138
stevieb CreditAttribution: stevieb commentedsubscribe
Comment #139
stevieb CreditAttribution: stevieb commentedI downloaded the accordion_menu module and as a solution it works
http://drupal.org/project/accordion_menu
Comment #140
Pavlos-1 CreditAttribution: Pavlos-1 commentedsubscribe
Comment #141
deceth CreditAttribution: deceth commentedsubscribe
Comment #142
raitwebs CreditAttribution: raitwebs commentedRunning D7.7, tried manual patching of #101 - No go.
Copied the files suggested in #132 and applied the patch to those - Still no go.
Comment #143
James Andres CreditAttribution: James Andres commented@raitwebs, did you try the patch I posted in #133?
Comment #144
bitcore CreditAttribution: bitcore commentedUpdate: 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.
Comment #145
pillarsdotnet CreditAttribution: pillarsdotnet commented#133 re-rolled for current 7.x checkout.
Comment #146
dstolAnother reroll based on #145 to correct a variable name.
Comment #147
steven.itterly CreditAttribution: steven.itterly commentedsubscribing
Comment #148
pillarsdotnet CreditAttribution: pillarsdotnet commentedPlease don't change the version.
Comment #149
raitwebs CreditAttribution: raitwebs commented@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.
Comment #150
nicnet CreditAttribution: nicnet commentedsubscribe
Comment #151
khiminrm CreditAttribution: khiminrm commentedsubscribe
Comment #152
travisc CreditAttribution: travisc commentedcustom_menus.patch queued for re-testing.
Comment #153
travisc CreditAttribution: travisc commentedduplicate
Comment #154
travisc CreditAttribution: travisc commentedJust tested this patch on 7.7, seems to work as described, no issues yet... THANK YOU!
Comment #155
johnbarclay CreditAttribution: johnbarclay commentedThis 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.
Comment #156
jn2 CreditAttribution: jn2 commented@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.
Comment #157
calculus CreditAttribution: calculus commentedsub
Comment #158
Matthias Lersch CreditAttribution: Matthias Lersch commentedFor 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:
Comment #159
aspilicious CreditAttribution: aspilicious commentedNeeds new rerolls, I don't know what patch to apply to D7 or D8. And they don't apply...
Comment #160
pillarsdotnet CreditAttribution: pillarsdotnet commentedRe-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.
Comment #161
GiorgosKmaybe 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 ...
Comment #162
aspilicious CreditAttribution: aspilicious commentedOff-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.
Comment #163
ennazussuzanne CreditAttribution: ennazussuzanne commentedsubscribe
Comment #164
GiorgosK@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
Comment #165
tran_tien CreditAttribution: tran_tien commented+1 sub
Comment #166
pindaman CreditAttribution: pindaman commented#101: per_menu_preferred_links-942782-101.patch queued for re-testing.
Comment #167
melon CreditAttribution: melon commentedRe-rolled for 7.x as 7.8 already contains
menu_update_7002()
and this caused a fatal error of redeclaration. Renamed the function tomenu_update_7003()
so this applies to 7.8 installs now.Comment #168
tomwrobel CreditAttribution: tomwrobel commentedsubscribing
Comment #169
mlecha CreditAttribution: mlecha commentedsubscribe
Comment #170
colanThat patch didn't apply. How about this one?
Comment #171
pillarsdotnet CreditAttribution: pillarsdotnet commentedinterdiff 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.
Comment #172
pillarsdotnet CreditAttribution: pillarsdotnet commentedFor real, this time.
Comment #173
colanLooks 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.
Comment #174
pillarsdotnet CreditAttribution: pillarsdotnet commentedI don't think anybody has an active site running Drupal 8 yet.
Comment #175
colanI just got a D8 set up to test #160. All good. Please also commit #172 for D7.
Comment #176
tomwrobel CreditAttribution: tomwrobel commented#172 did the job, thank you!
Comment #177
catchHow many additional queries per page is this adding? One per menu?
Comment #178
Scyther CreditAttribution: Scyther commentedSub
Comment #179
catchCNR until there's an answer to #177.
Comment #180
colanLooking at the patch file it appears as though we have added the following (potential) queries:
Comment #181
catchHas anyone checked this query is still indexed with this condition removed?
Comment #182
bulldozer2003If 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
Comment #183
jacieyang CreditAttribution: jacieyang commented#2: 942782-3-custom_menus.patch queued for re-testing.
Comment #184
pillarsdotnet CreditAttribution: pillarsdotnet commentedPatch no longer applies since #520106 was committed. The rejected fragment is:
And the conflicting commit fragment is:
Leaving this at CNW because I don't know the right way to resolve the conflict.
Comment #185
pillarsdotnet CreditAttribution: pillarsdotnet commentedJust a wild hairy guess. Would this work?
Comment #186
pillarsdotnet CreditAttribution: pillarsdotnet commentedLet's see what the testbot has to say, anyway.
Comment #187
pillarsdotnet CreditAttribution: pillarsdotnet commentedSo to answer the question posed by @catch in #181:
So I added the following to find out:
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 theParent item
dropdown list. (Why?)Selected
Main menu
instead, and clicked onSave
.Back to
Edit
.Changed the
Parent item
toCustom menu
and clickedSave
again.Checked the output of
/tmp/menu.log
and saw:(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:
Does that answer your question?
Comment #188
jrchamp CreditAttribution: jrchamp commentedLooks 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.
Comment #189
catchIt 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?
Comment #190
webengr CreditAttribution: webengr commentedsubscribe
-- 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.
Comment #191
TimG1 CreditAttribution: TimG1 commentedeek...sorry, subscribe.
Comment #192
wickedskaman CreditAttribution: wickedskaman commentedIt'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.
Comment #193
pillarsdotnet CreditAttribution: pillarsdotnet commented@#189 by catch on October 1, 2011 at 3:40pm:
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.
Comment #194
sun@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.
Comment #195
cangeceiro CreditAttribution: cangeceiro commentedpatch in #185 worked for me, subscribing
Comment #196
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedThe 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.
Comment #197
catchYeah this is all that's needed.
Comment #198
pillarsdotnet CreditAttribution: pillarsdotnet commented@#197 by catch on October 8, 2011 at 9:35am:
Okay; here are all D8 queries with a condition depending on
{menu_links}.menu_name
:ForumTestCase::doAdminTests()
toolbar_get_menu_tree()
menu_node_prepare()
MenuTestCase::deleteCustomMenu()
menu_delete_menu_confirm()
menu_delete_menu_confirm_submit()
book_menu_subtree_data()
menu_edit_menu_name_exists()
system_admin_config_page()
menu_tree_page_data()
menu_load_links()
_menu_link_find_parent()
_menu_set_expanded_menus()
menu_link_children_relative_depth()
menu_link_move_children()
menu_update_parental_status()
Comment #199
colanReady to go?
Comment #200
chx CreditAttribution: chx commentedThanks 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.
Comment #201
kerios83 CreditAttribution: kerios83 commentedI 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."
Comment #202
royerd CreditAttribution: royerd commentedWhy 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.
Comment #203
MustangGB CreditAttribution: MustangGB commentedNot yet, still v6
Comment #204
colanThis'll need to get re-rolled as per http://xjm.drupalgardens.com/blog/rerolling-drupal-8-core-patches
Comment #205
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedHere 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.
Comment #206
chaplinskiy@gmail.com CreditAttribution: chaplinskiy@gmail.com commented#6: 942782-6-custom_mens.patch queued for re-testing.
Comment #207
peterx CreditAttribution: peterx commented#205 per-menu-preferred-links-942782-205.patch works beautifully in Drupal 7.9.
Comment #208
colanComment #209
caktux CreditAttribution: caktux commentedLosing the page title in many admin pages after applying the latest patch to Drupal 7.9... Maybe something related to locale or i18n?
Comment #210
aspilicious CreditAttribution: aspilicious commentedWe need to check 209 first
Comment #211
Tor Arne Thune CreditAttribution: Tor Arne Thune commented@#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?
Comment #212
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedHere'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.
Comment #213
jherencia CreditAttribution: jherencia commented@colan set this as reviewed & tested by the community and nothing has changed since then for D8.x.
Comment #214
caktux CreditAttribution: caktux commented@#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.
Comment #215
xjmSetting 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:
So, we need to track down the cause of that issue before we move forward here.
Comment #216
caktux CreditAttribution: caktux commentedI 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.Comment #217
xjmIf 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?
Comment #218
caktux CreditAttribution: caktux commentedLooking 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.Comment #219
linno CreditAttribution: linno commented#205: per-menu-preferred-links-942782-205.patch queued for re-testing.
Comment #220
xjmAlright, 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.Comment #221
caktux CreditAttribution: caktux commentedi18n's issue: #1351678: Follow menu_link_get_preferred active trail handling for custom menus
Comment #222
xjmAlright, now that this question has been resolved we can move this back to RTBC. :) Thanks @caktux.
Comment #223
catch#205: per-menu-preferred-links-942782-205.patch queued for re-testing.
Comment #224
laghalt CreditAttribution: laghalt commentedApplied 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
Comment #225
xjm@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.
Comment #226
prabhatjn CreditAttribution: prabhatjn commentedI 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:
Thanks everyone,
Perry.
Comment #227
prabhatjn CreditAttribution: prabhatjn commentedI forgot to tell, I have also tried #212 on my D7 site.
Perry.
Comment #228
aspilicious CreditAttribution: aspilicious commentedDone that?
Comment #229
xjm@Perry Jain: The patch did not apply properly, so that is why you are getting whitescreens. It is rolled against 7.x-dev.
Comment #230
aspilicious CreditAttribution: aspilicious commentedJust 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!
Comment #231
prabhatjn CreditAttribution: prabhatjn commentedThanks @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.
Comment #232
xjm#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. :)
Comment #233
laghalt CreditAttribution: laghalt commentedThanks 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.
Comment #234
xjmSome before/after screenshots for the summary.
Comment #234.0
xjm(xjm) First outline of a summary.
Comment #234.1
xjmUpdated issue summary.
Comment #234.2
xjmUpdated issue summary.
Comment #234.3
xjmUpdated issue summary.
Comment #234.4
xjmUpdated issue summary.
Comment #234.5
xjmUpdated issue summary.
Comment #234.6
xjmUpdated issue summary.
Comment #234.7
xjmUpdated issue summary.
Comment #235
xjmAdded an issue summary.
Comment #235.0
xjmUpdated issue summary.
Comment #235.1
xjmUpdated issue summary.
Comment #235.2
xjmUpdated issue summary.
Comment #235.3
xjmUpdated issue summary.
Comment #235.4
xjmUpdated issue summary.
Comment #236
pwolanin CreditAttribution: pwolanin commentedOverall 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.Comment #237
xjmSetting 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).
Comment #237.0
xjmUpdated issue summary.
Comment #237.1
xjmUpdated issue summary.
Comment #238
pwolanin CreditAttribution: pwolanin commentedWe 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.
Comment #239
pwolanin CreditAttribution: pwolanin commentedComment #240
xjmI'll roll with #238.
Comment #241
xjmComment #242
xjmComment #243
xjmUhm. Without wrapping a constant in quotes. Derp.
Note that the D7 backport should switch to
define()
.Comment #244
xjmComment #246
Tor Arne Thune CreditAttribution: Tor Arne Thune commented#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?
Comment #247
xjm#246 Oh! Duh. :) It was supposed to be in
menu.inc
. Rerolling.Comment #248
xjmWith the constant defined in the correct file.
Comment #249
Tor Arne Thune CreditAttribution: Tor Arne Thune commented#248 looks like a good solution to me, and answers part of the "concern" in #236.
Comment #250
JamesOakleyI 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.
Comment #251
aspilicious CreditAttribution: aspilicious commentedJust tested this again on my dev site. (followed the steps from the summary) If I didn't do anything wrong this should be rtbc.
Comment #252
JamesOakleyThis issue seems to be fixed in 7.10 core release yesterday, although it wasn't mentioned in the release notes.
Comment #253
xjm#252: No, this patch has not been committed yet.
Comment #254
JamesOakleywhich 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.
Comment #255
aspilicious CreditAttribution: aspilicious commentedJamesOakley this is NOT fixed. Look at the summary and reproduce that use case. You'll see it is *not* fixed in 7.10
Comment #256
catchCommitted/pushed to 8.x, this will need a re-roll for 7.x. Thanks for all the work on this everyone.
Comment #257
xjmRerolled for D7 with
define()
.Comment #258
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedIs the interdiff for the last D7 patch in #212? Because I couldn't find the update function in the new patch.
Comment #259
xjmThe interdiff is against the D8 patch. I forgot that we had to add the update function as well.
Comment #260
aspilicious CreditAttribution: aspilicious commentedOk tested this *again* and the patch still works *surprise surprise* so I'm marking this rtbc again for drupal 7.
Comment #261
aspilicious CreditAttribution: aspilicious commentedNeeds work, didn't test an upgrade... *sigh*
Comment #262
xjmComment #263
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedLooks good now with the update function.
Comment #264
Mr.vantri CreditAttribution: Mr.vantri commentedHay lam
Comment #265
Mr.vantri CreditAttribution: Mr.vantri commentedCUng duoc
Comment #266
Mr.vantri CreditAttribution: Mr.vantri commentedhay qua di mat
Comment #267
geerlingguy CreditAttribution: geerlingguy commentedComment #268
dj1999 CreditAttribution: dj1999 commented#262 patch works for me.
Thanks!
Comment #269
artfulrobot CreditAttribution: artfulrobot commentedPatch #262 works for me on Drupal 7.10. Thank you! I was going mad thinking I'd completely misunderstood what menus were.
Comment #270
Summit CreditAttribution: Summit commentedHi, Could you post your changed files please?
Will this go in 7.11?
Greetings, Martijn
Comment #271
xjm@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.
Comment #272
Zachmo CreditAttribution: Zachmo commentedI'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.
Comment #273
xjm@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:
It could be that your issue is being caused by a contributed module or something else. It may be a separate issue.
Comment #274
Zachmo CreditAttribution: Zachmo commentedThanks for trying to replicate. I'll try on a clean install.
Comment #275
7wonders CreditAttribution: 7wonders commentedworks for me on d7 dev
Comment #276
FooZee CreditAttribution: FooZee commentedWorks 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 ?
Comment #277
xjmAye, 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.
Comment #278
aspilicious CreditAttribution: aspilicious commentedFollowed 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
Comment #279
magpie5212 CreditAttribution: magpie5212 commentedSubscribe, I will wait until this is incorporated in a release.
Comment #280
xjmHi @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!
Comment #281
sunThis looks OK for me. However, we need to revisit, analyze, and revamp this functionality for D8 (separate issue[s]).
Comment #282
linno CreditAttribution: linno commentedhi 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
Comment #283
Tor Arne Thune CreditAttribution: Tor Arne Thune commented#282: Did you run update.php after applying the patch?
Comment #284
xjm@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.
Comment #285
linno CreditAttribution: linno commentedhello
@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 !
Comment #286
linno CreditAttribution: linno commentedOk...
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.
Comment #287
xjm@linno, you should open a separate support issue for that in the superfish module. This issue is specifically about the patch above. Thanks.
Comment #288
webchickWow!
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.
Comment #289
chx CreditAttribution: chx commentedII do not see anything OMG world ending in there.
Comment #290
benjf CreditAttribution: benjf commentedthe patch in comment #262 works for me. thanks!
Comment #291
webchickAll 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!!
Comment #292
kerios83 CreditAttribution: kerios83 commentedTHX, guys I have been waiting for this a looong time :)
Comment #293
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedWhat a great christmas gift (for us geeks) :) Thanks to all the testers, patchers and committers!
Comment #294
wickedskaman CreditAttribution: wickedskaman commentedBig ups everyone! What an annoying complicated little guy. It's always the seemingly simple things isn't it? :)
Comment #295
Summit CreditAttribution: Summit commentedHi, 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
Comment #296
JamesOakley@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.
Comment #297
Summit CreditAttribution: Summit commentedHi James, I will do. Thanks for your update. Greetings, Martijn
Comment #298
Summit CreditAttribution: Summit commentedHi, 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
Comment #299
xjm@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.
Comment #300
JamesOakleyMartijn
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.
Comment #301
Summit CreditAttribution: Summit commentedThanks guys, Drupal rocks! Martijn
Comment #302
Martin Mayer CreditAttribution: Martin Mayer commentedWhen I use the patch from #262, the child links in the main menu disappear. They come back when I undo the patch.
Comment #303
aspilicious CreditAttribution: aspilicious commentedMartin don't tell me you're using the i18n module...
Comment #305
Dave ReidFollowup to menu_block.module: #1425342: Drupal core upgrade from 7.10 to 7.12 causes menu block to fail
Comment #306
mollask CreditAttribution: mollask commentedHi 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.
Comment #307
xjmPlease 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.
Comment #308
mollask CreditAttribution: mollask commentedWoops. 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.
Comment #309
pmitsos CreditAttribution: pmitsos commented#6: 942782-6-custom_mens.patch queued for re-testing.
Comment #310
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #311
testtest23112 CreditAttribution: testtest23112 commented#262: d7-942782-262.patch queued for re-testing.
Comment #313
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedI can't begin to understand the reasoning behind re-testing these patches, so I assume it's a mistake or misunderstanding.
Comment #314
testtest23112 CreditAttribution: testtest23112 commentedSorry, indeed this was my mistake. I apologise.
Comment #315
bvanmeurs CreditAttribution: bvanmeurs commentedIn 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.
Comment #316
xjm@bvanmeurs -- Could you open a followup issue? Thanks.
Comment #317
dev_007 CreditAttribution: dev_007 commented#248: custom-active-trail-942782-248.patch queued for re-testing.
Comment #318
webchickComment #319
magpie5212 CreditAttribution: magpie5212 commentedIs 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.
Comment #320
xjmPlease 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").
Comment #321
magpie5212 CreditAttribution: magpie5212 commentedBuilt a new site from scratch and it works. So now to install modules one at a time until it fails.
Comment #322
magpie5212 CreditAttribution: magpie5212 commentedI have pinned this down to the menu items for the link . Sub-items under that link do no show. Other sub-items are OK.
Comment #323
xjmAgain, please post a separate issue.
Comment #324
magpie5212 CreditAttribution: magpie5212 commentedFollow on issue here: #1468420: Submenus of a parent menu item that links to the front page (<front>) do not show when viewing the site's front page.
Comment #325
quantos CreditAttribution: quantos commentedI 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.
Comment #327
jddeli CreditAttribution: jddeli commentedI 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 .
Comment #328
JamesOakley@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.
Comment #329
jddeli CreditAttribution: jddeli commentedI 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.
Comment #330
JamesOakleyOK, 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.
Comment #331
jddeli CreditAttribution: jddeli commentedI 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.
Comment #332
MustangGB CreditAttribution: MustangGB commentedComment #333
Nor4a CreditAttribution: Nor4a commentedYess! This from #329
saved my life :)
Thanks!
Comment #334
Evoksha CreditAttribution: Evoksha commentedWe 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.
Comment #335
TelFiRE CreditAttribution: TelFiRE commentedIssue is still there in 7.14. Active-trail is completely inconsistant.
Comment #336
broonI 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.
Comment #337
CowTownFarmBoy CreditAttribution: CowTownFarmBoy commentedI 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...
Comment #338
JamesOakleyCowTownFarmBoy: 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.
Comment #339
CowTownFarmBoy CreditAttribution: CowTownFarmBoy commentedThanks 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
Comment #340
NenadP CreditAttribution: NenadP commentedIs 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
Comment #341
yang_yi_cn CreditAttribution: yang_yi_cn commentedas #338 suggested I create a new issue for this #1710744: Custom menus never receive an active trail (but system menu does)
Comment #341.0
yang_yi_cn CreditAttribution: yang_yi_cn commented(xjm)