Upgrading messes up Primary Links and Navigation

Is this for real? Setting primary links to Navigation?? What's the point??

Possible use cases:

1. One might have one of those JS menu modules like Nice Menus or Admin Menu that collapse the navigation down so that it's not spiraling out of control. :)
2. One might've already re-ordered/added/disabled things as they want in the Navigation menu, and are using the "drill down" functionality that happens when both primary and secondary are set to the same menu.

Can you please upload a dump of your D5 menu table for testing purposes? So far this is the first report we've received of data munging due to the upgrade since before RC1.

I can reproduce most of the original issue by setting "Menu containing primary links" to Navigation, and then performing an update. The administration menu is spread between primary links and navigation, and there are no 'reset' links...

Maybe do something like the following in the update code?

if ("Menu containing primary links" == Navigation) {
  primary links = new empty menu;
  navigation = navigation; 
}

Same for secondary links.

Actually, I've heard of several people setting Primary Links to Navigation (secondary too). I was given that advice a long time ago when I was first starting out. I don't do it any more though.

Even without that, I saw problem #2 on my first upgrade (beta3?), but not since then.

I though a bug fix went in that might make this a little better - but this is indeed kind of a mess. Since we have (mostly) decoupled router items and links, and since being able to move items form under /admin and thus have them NOT show up on the administration apge was considered an inportant feature, I'm not sure how to resolve this without some substantial rewriting.

I think I'd agree with the basic approach suggested by webernet - either move everything back to navigation and leave primary links emoty, or perhaps put the amin links in navigation and then duplicate them in the primary links.

I also think webernet's suggestion is good, although it'd need documenting. Duplication the admin links between primary links and navigation might work as well.

yes - I think basically the mechanism could look like this for these cases:

put everything form the D5 navigation menu into the primary (or secondary) links menu BUT set module='menu' NOT module='system'

then I think menu rebuild will put a clean, new, set of links derived from the router items into the navigation menu with module='system'

As for the fourth bullet in #11 --> http://drupal.org/node/212557

I am very much against this approach. I might look at the problem tomorrow but making a special case out of the primary and secondary links just because -- nope, I do not like this.

So, this gets us somewhat back to this issue/comment: http://drupal.org/node/160168#comment-582457

grumble. someone plz up 6021. Note that this is a whole another approach and yet I severely dislike and yet I know this is how D5 worked so I have no better idea.

also, it may be a foolish suggestion, but we could htink about using better batch APi methods too: http://drupal.org/node/198980

We probably should change as little as possible this late in the game.

Slight change - PHP syntax error

The last patch contains some cruft, ie line ending changes and other formatting changes.

I dislike doing this because it adds extra complexity and special cases to the menu, that's all. But lets see more drastic changes in D7. (I am thinking on removing primary and secondary links from Drupal to be replaced by appropriate theming of a block in the header region)

patch is crufty indeed - also seems to accidentally comment out part of of the drag-n-drop functionality.

re-roll with most of the cruft removed, and with other fixes (including changing the description string, since the existing one doesn't make sense in light of the altered functionality)

no update path fix yet, so CNR means just check that this part of the functionality is right.

Note, per chx, that while it might make sense to rethinking the hard-coded primary & secondary menus, in light of the hopes of getting a release out real soon now (TM), we will hew to the path of the least code changes to achieve an acceptable fix.

ok, here's with a first, UNTESTED attempt to fix the update path. The code's a bit ugly, but seems to be the minimal change possible.

Patch looks reasonable, but it would be easier to review without code style changes. I wonder why was this feature lost in the first place.

+    '#description' => t('Select what should be displayed as the secondary links. If the same menu as is the source for the primary links (currently %primary) is chosen, the children of the active primary menu link (if any) will be shown instead of the top-level links that are shown if the source is a different menu.', array('%primary' => $menu_options[$primary])),

Something is odd here around "If the same menu as is the source..." Also, I've read this several times and still can't really quite put my finger on what it actually means.

You're right, that is a pretty confusing description.

If the same menu as is the source for the primary links (currently %primary) is chosen, the children of the active primary menu link (if any) will be shown instead of the top-level links that are shown if the source is a different menu.

Translated to comprehensible English, as far as I understand it:

You can choose the same menu for secondary links as for primary links (currently %primary). If you do this, the children of the active primary menu link will be displayed as secondary links.

Better?

Hrm, not quite... this does not cover a lot of possibilities. Lets see this patch. The update, at the end of the day, actually got simpler.

typofix.

this does not cover a lot of possibilities

Sorry, but now I'm stumped. When I saw that monster sentence I thought I had an inkling of what it meant, but if it doesn't mean what I said, then what possibilities am I missing?

If you are referring to "instead of the top-level links that are shown if the source is a different menu", then that just describes what the secondary links menu does normally - which is pretty self-explanatory. It's setting primary and secondary links to be the same menu that is the unexpected neat feature, and that is what needs to be explained.

@Arancaytar: chx was referring to the (update) code, not your text improvements. IMHO your text improvements are better, and definitely cover the task at hand there. Chx is working on the code :)

Oh, okay. I read the new patch, but didn't pay close attention to what it changed from the previous one.

I guess the text improvement ought to wait until the rest of the patch is ready, since it changes only a single line.

Should note that a slightly altered version of Arancaytar's text improvements are in #30. Overall the string changes look good to me, and I never realised that about secondary links...

I'm not sure if I'll have time to actually test this today, either way can I confirm this is what needs to be checked?

1. install D5, create primary + secondary links as new menus from scratch, upgrade
2. install D5, set navigation to primary links, upgrade
3. install D5, set primary and secondary links to the same menu, upgrade
4. clean D6 install I guess.

Anything missing from that?

Should note that a slightly altered version of Arancaytar's text improvements are in #30.

Doesn't look that way - this is from #30:

-    '#description' => t('Select what should be displayed as the secondary links. If %primary is chosen, the children of the active primary menu link (if any) will be shown instead of the links in the %secondary menu.', array('%secondary' => $menu_options['secondary-links'],  '%primary' => $menu_options['primary-links'])),
+    '#description' => t('Select what should be displayed as the secondary links. If the same menu as is the source for the primary links (currently %primary) is chosen, the children of the active primary menu link (if any) will be shown instead of the top-level links that are shown if the source is a different menu.', array('%primary' => $menu_options[$primary])),

I have not included string fixes those posts happened while i was fiddling with the code.

@David_Rothstein - all three of those should exist as entries in {menu_custom} or the update code is broken. It's fine if there are zero items in {menu_links} in any of the menus.

Has anyone tested with Administer being moved out of Navigation into it's own menu? I ask because I just had some very weird happenings with that situation and a module being converted that had an improperly constructed menu.

Anyone else to test this, so that we can get over this issue? :)

Ok I'll do these one by one, and keep editing the post, just in case someone lovely wants to work backwards from seven.

1. install D5, create primary + secondary links as new menus from scratch, upgrade
- fine.

2. install D5, set primary links to navigation, upgrade
- ok I did this, but left my empty primary links menu untouched. I get empty primary links and secondary links after upgrade and a decent navigation menu. Secondary links appears.

2a. deleted primary links. Both empty menus appear, navigation is still set to primary links in settings. seems fine this one.

3. install D5, set secondary links to navigation, upgrade
- This way I get empty secondary links and primary links menus. Navigation still set as secondary links in settings. The top links in Garland are showing top level Navigation items - I guess because primary is empty?? Haven't looked into how Garland treats this. Anyway, no damage at least.

4. install D5, set both primary and secondary links to navigation, upgrade

5. install D5, set primary and secondary links to the same (non-navigation) menu, upgrade
6. install D5, set primary and/or secondary links to "No primary/secondary links", upgrade
7. clean D6 install I guess.

One minor issue with #46 -- The primary links menu no longer gets a description (menu had been renamed prior to update).

[edit]

Will try to fix the problem noted by webernet and clean up the code a little more during further testing.

this patch puts in the description as suggested by webernet, and cleans up the code a little. It keeps the basically the approach worked out by David_Rothstein.

I rean a few update scenarios with MySQL, but I'm not known for my ability to hit all the edge cases when writing patches...

4. install D5, set both primary and secondary links to navigation, upgrade

upgrade smooth. Settings shows secondary links as set to navigation. Primary Links menu (with new description) contains the navigation menu, but it also contains my old watchdog links etc.Also the Navigation menu exists independently of the Primary Links menu, and that contains all new drupal admin menu items (reports, actions) which are now outside the admin menu. I don't think this is a showstopper, so not going to mark as needs work based on this.

The original primary links menu still shows there as empty with no description - this is a good thing, I might've put something in there and/or changed it if it was a real site.

5. install D5, set primary and secondary links to the same (non-navigation) menu, upgrade
Yep good. No issues at all.

6. install D5, set primary and/or secondary links to "No primary/secondary links", upgrade
Set both of these and renamed primary links for good measure. Got spangly new Primary and Secondary links menus with the descriptions, and in settings it's a radio (which defaults to secondary links). Again, seems fine.

Alright, as far as I'm concerned the issues in 4. are minor, and iirc the upgrade path to remove stale links was introduced late in the cycle, and was non-trivial. Considering the menu is outside of it's normal settings at this point. Having navigation set to both primary and secondary links to me seems like a fairly odd edge case, especially doing so with the entire admin menu tree left in there - if you're working day-to-day with that, then surely you can handle deleting about five menu items and moving a couple over (and you could've also tested this patch in the past three days).

Leaving at needs review, someone who can think of real valid use cases for 4. will have to explain what desired behaviour is and/or patch/test this.

Testing the latest patch.

Catch: I also run through all test cases with #52, and I cannot see any errors popping up. Both primary and secondary links menus are properly created. Invalid menu items used (eg. node/12 when there was no such node) disappear as expected in Drupal 6, valid items remain. Settings of the primary/secondary links to each other or navigation or no menu stays after the upgrade. I have seen no menu item duplication is the primary/secondary menus when the navigation menu was set for their source with the latest patch (#52). Did you test these last scenarios with the latest patch in #52?

OK I must've gone wrong somewhere - couldn't reproduce the errors I got with 4.

Now, if I set navigation to both primary and secondary links (leaving the empty primary menu as it is), then upgrade, I get a nice Navigation menu, no bogus items, everything that's supposed to be there, and empty primary and secondary links menus, with descriptions.

I think that's pretty damn good, so marking RTBC since all the other tests were fine anyway.

Wow, thanks, committed #52 to 6.x. (I also got notice from webernet on IRC, that he also tested and all looked fine to him as well). Still needs to be committed to 7.x.

bump - needs to be committed to HEAD

Committed to CVS HEAD. Thanks.

From #10:

Second, what happened to the menu_primary_menu variable in Drupal 6 -- is it even used?

I think the variable override in settings.php for primary links is now done like this:

'menu_primary_links_source' => 'menu_name',

where 'menu_name' comes from your menu_custom table.