Using the same source for main/secondary with a custom menu doesn't work

Well, this seems to have broken behaviour that was working before, and will result in a lot of sites' navigation breaking on update. Escalating to critical.

OK first of all this is only broken when using the same source for primary and secondary menu, just in case anyone else tries to reproduce this and fails in the same way I did. If you create a custom menu and set that as the source for secondary links, it works fine.

I didn't bother to try to reproduce with the single-source feature, don't have time right now.

To move this forward we need the following, any or all of these would be helpful:

# Install alpha 7 and try to reproduce the bug, #907690: Breadcrumbs don't work for dynamic paths & local tasks #2 only got in a month ago and changed a lot of the logic surrounding this, would be good to rule that out as the cause - alpha 7 is here: http://drupal.org/node/913030

# Write a simpletest that creates a custom menu with two levels of links, sets those variables, then verifies that things show up on the right pages. This test should fail with HEAD.

# In either this issue or a related one, someone mentioned that the 'expanded' logic is broken for custom menus, since this feature relies on much the same thing, that was likely broken at the same time, so testing of alpha 7 for that feature too would be good (this could also have a test written for it, would be very similar to the one for secondary source).

Here is test to show the bug.

What I don't like in the test is defining block name of menu. How to find out the name of the block (for block administration page) by name of menu? Should it be system_* or menu_*?

CNR for the bot, let's watch that test fail.

The test fail triggered is in MenuTestCase->verifyMainSecondaryLinks() on line 625.

It is very strange that so many other fails appeared. Looks like something gone wrong with a lot of tests from menu.

@7: The single-source feature worked in alpha7 (see #949558: Secondary menu empty (2nd level in hierarchy) (duplicate of this issue)).

Isn't this too edge case for critical? I can't imagine many sites use same menu for both.

@#13: Not sure if this is not a misunderstanding ... this issue is not about duplicating the same items but about
the feature advertised as:
"An advanced option allows you to use the same source for both Main links (currently xxx) 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."
So the items are from different levels of the same menu.

Quickly skimming the code in HEAD, this issue seems to be same as #942782: Custom menus never receive an active trail. Not sure why this worked with alpha7.

Attached patch would be a simple but not really performance-friendly way to resolve this. It's not only bad for performance, but it also leads to arbitrary results, since the order/weight of menus in that system variable array controls, which link in which menu is going to be the active link for the current page.

Therefore, I would recommend a proper fix, which actually seems to be very simple:

Add a {menu_custom}.weight column and a tableDrag to the menu overview page.

Apparently, we did the same for user roles, and that was like a 10 minute action.

And this is how the recommended solution would look like. Note: Only cared for the menu admin overview page for now.

Also, I really have to wonder what's so complex about a #type 'table'.

drupal_pre_render_table() looks too useful to hide in a menu include. Please move to theme.inc or other common file.

Do we need some help text describing why one might want to re-order menus?

Totally agreed on the generic usefulness :)

- Added help text.
- Removed dedicated theme function for the menu title/description table cell (identical to #type 'item').
- Merged in the patch containing only tests from #8.
- Made the form submit handler actually update the menu system variable.

oopsie, forgot an essential array_keys(), sorry.

Removed the merged-in tests to see whether HEAD fails, too.

Alright - those test failures are caused by a system module update that indirectly invokes _block_rehash() during the update, which in turn invokes system_block_info_alter() or whatnot, which invokes menu_load_all(), which in turn fails, because the new 'weight' column does not exist yet. That's the exact reason for why we do not invoke API functions and hooks in module updates.

Fixed that update in attached patch.

Um. What?

This functionality worked just fine in Drupals 4.x, 5.x, and 6.x without all this crazy extra code. Please explain why on earth this is needed, esp. at this stage in the release cycle when we are done adding nice things.

Subscribing and still not working. As a regression, does it qualify for mayor?

There's a patch here, please be careful when changing issue status. The patch does indeed look like a lot of new stuff just to fix one regression.

Will this be fixed in the next release?

Making custom menu available for main and secondary links is even more important when dealing with multilingual sites where menus for each menu is used.

I have introduced this patch (post #27 but still submenu is not appearing).
Do I need ti uninstall all or should it automatically appear after opening the site?
Regards,
Alejandro.

This really looks like it should be merged with #942782: Custom menus never receive an active trail, although that is going to be painful at this point.

I think I get the reasoning for the weight column, but can we get that kind of UI change into D7 any more?

#8 has a patch with a test in, this definitely needs some.

I'm a bit surprised. Neither this nor the patch in #942782: Custom menus never receive an active trail resolved this issue, but after applying both, and reverting both, reverting variables, flushing caches, running updates, and updating theme settings in Appearance, secondary links with same source as primary links re-appeared in my theme.

I did database dumps and performed a diff between before and after, yielding lots of ugly and interesting things, but no totally obvious reason for why the menu system suddenly started to build a second level for secondary links. Will further investigate this later.

I got here thanks to @catch and this issue #1231856: menu_update_7001() breaks when updating from 7.4 or upgrading from D6. I had secondary menus working but the upgrade seems to have disrupted it. I'm not sure what in the upgrade disrupted my installation.

@sun's patch looks good, but it's big. Does anyone have any feedback to @webchick's comment on #28 about it working previously without the extra code?

I don't have the skills to fix this, but it's been an open issue now for close to a year. If it's not fixed soon workarounds should be documented.

iirc this was working for some time during the Drupal 7 release cycle and was broken in September or October 2010. I don't specifically remember it working but I think that's what webchick was referring to.

I don't know which commit caused the breakage, but that might be something worth bisecting - if we'd done that 10 months ago we could possibly have just reverted the regression and not be here now.

#942782: Custom menus never receive an active trail was opened around the same time and the issues should probably be merged although the patches completely conflict and each issue has taken quite divergent paths, so we need to try to preserve some of the history if we do that (and confirm any solution actually fixes both issues).

Tagging.

Judging from what I wrote back then in the duplicate issue #949558: Secondary menu empty (2nd level in hierarchy) it worked for me in D7 alpha7 but not in D7 beta1.

That's three weeks:
Alpha 7: September 16, 2010 at 1:11am – http://drupal.org/node/913030
Beta 1: October 7, 2010 at 3:36am – http://drupal.org/node/934174

I wouldn't mind phasing out this feature. It's a limited, stub-like core feature. If possible, I suggest aiming for a D7-focussed stop gap solution, without re-inventing this first for D8 etc. Re-inventing menu ux is definitly a to-do, but this specific feature is not the right context for that.

Gentle bump towards a pragmatic fix :)

Seems a reasonable tag to add as per DrupalWTFs, since it is advertised in 7.x help text but not working.
Related issue: #410646: "Secondary menu" exists but is no longer the default source for the secondary links

It's >1 year since last update to this issue so, I think this bug will not get attention enough until D8 gets in bug squashing phase.
Point is, independent of how this bug gets handled in 8.x, I think we should stop advertising a broken feature in 7.x, at least until it is fixed. It has been driving me crazy until I googled for a possible error. I think it's a big no-no for a released product.
Let's keep in mind that people are already downloading D7 with this bug. We should at least not add more confusion by keeping the text.
I've been tricked by this because coming from 5.x and 6.x I expected it to work. I double checked the description text to see if the feature was still supported. This text is plain misleading and can bring lots of frustration.
I can't fix the bug myself, but I think it's worth creating a novice tagged issue to remove the text "An advanced option [...] and the children of the active link will appear in the Secondary links.". When the bug is
dealt with, just reverting to original text would suffice. I know it will affect translations, but IMO this is the lesser of two evils.
How about *temporarily* removing the help text until this issue gets fixed?
Obviously, a link to the related issue should be put in this one's description to keep track and undo it later.

BTW: I don't agree with phasing out this feature in 8.x, but that's another story, specially taking in account the new improvements being made.

http://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.

The summary may need to be updated with information from comments.

I'm working on a summary for this at the drupalcon sprint

Added an issue summary above, but I tested this in both D8 and D7.22 and the menu settings are working as expected. I was able to make a custom menu, use it as the source for both primary and secondary menus, and have the secondary menu properly pick up the active menu item's children.

No patch commited and still tagged as "Needs tests". Reopening to discuss test coverage.

adding an issue summary at drupalcon portland

I'm not sure what the current status of this issue is (conflicting statements in recent comments), but for D8, this is definitely fixed/obsolete.