D5->D6: menu items duplicated

I have duplicate "Administer". (Czech localized site, played with menu items weights previously.) Yes, I know the new one is capitalized, but still it's not very nice behavior.

The menu update checks for duplicates. It might be that in some locale the two entries happen to render the same but compare menu_links table, they will differ.

I examined what I can do through the UI:

- Previously, on 5.x, I "just" wanted my menu to have a logical order, so changed some weights. "administer" moved to the bottom.
- After 5.x->6.x upgrade, there is another "Administer" item. Both pointing to the same path (which I can't edit anyway).
- Observing the administrative UI (menu items id as seen in the operations links), I realized that I have not only "Administer" duplicated - I've the *whole* tree under "Administer" duplicated! (i.e. the items that survived the upgrade - 19 items)
- The only thing I can do is "reset" the old item (which only just moves it to where the new one is), or customize any (pointless), or disable (probably the only way to get rid of double display, but still my administrative menu UI is spammed with duplicate entries).

I examined menu_links table (after the additional link reset and disable), and both the links are identical, excepting 'mlid', 'p1', 'hidden', 'updated' and 'customized' fields.

The menu update needs to be changed do the following:

for each (D5 item) {
  if (menu item at that path already exists) {
    move to D5 path
  }
  else {
    create new item
  }
}

Basically, if you've customized any system menu items in D5, you'll currently get duplicates in D6.

Simplified code with comments. The patch is bigger because we removed stuff.

I just saw this with a 5.x->6.x update of a clean 5.x install with no translations or customizations - the "Create content" link was duplicated in an odd way.

Reproducible on a D5 system:
1. with no change in weights.
2. with English local.
And the duplicates are:
Administer, Administer -> logs, Administer -> settings, Administer -> users.

Drupal 5 immediately copies everything to menu table once you visit menu page, I just checked this. This means it is not possible that a child is in the menu table but it's parent is not. This means there is no need for menu links to be filled in beforehand by menu_rebuild. And as router paths are set to empty string anyways, there is no need for the router table to be filled in. We had a rebuild call just before the menu update finishes anyways, we now make sure it happens even if menu module is disabled. So the code can be simplified (and sped up which does not really matter on update but why not).

I just tried to test this, but was unable to reproduce the duplicated items in an unpatched install. So I'm bumping this for people who've experienced the issue to review it on their setups :)

Tested #14 --
-Still getting a duplicate of 'Contact form' in navigation (was moved to primary links under 5.x)

Also -- Both patched and unpatched -- on update only --
-Getting invalid 'url aliases' and 'contact form' links (under 'administer') pointing to /admin/path and /admin/contact (the correct links are also created under 'site building' and point to /admin/build/path and /admin/build/contact)
-Links to forum, aggregator, search appear -- none of these modules were enabled under 5.x, and still aren't.

time to attach / mail me menu table dumps.

After receiving webernet's table and switching off aggregator and forum I do not see anything that should not be there. Do you? There is no duplicate of any contact link:

mysql> select menu_name,hidden,link_path,link_title from menu_links where link_title like '%contact%' order by link_path;
+---------------+--------+------------------------------+-----------------------+
| menu_name     | hidden | link_path                    | link_title            |
+---------------+--------+------------------------------+-----------------------+
| navigation    |      0 | admin/build/contact          | Contact form          |
| navigation    |     -1 | admin/build/contact/delete/% | Delete contact        |
| navigation    |     -1 | admin/build/contact/edit/%   | Edit contact category |
| navigation    |      0 | admin/contact                | contact form          |
| navigation    |     -1 | admin/help/contact           | contact               |
| primary-links |      1 | contact                      | Contact Form          |
| primary-links |      0 | node/2                       | Contact Us            |
+---------------+--------+------------------------------+-----------------------+

The admin/contact link you see comes from Drupal 4.7, delete by hand, there is no way Drupal 6 can know this is a bogus link. If I'd delete all the links which are without a router path, then when you switch back your contributed modules (which you surely switch off as per update instructions) then all the links to contributed paths would be gone. You surely do not want that. This goes to hass, too.

Lacking anything that I could work on, I set back to CNR.

As a followup we can create a contrib module or a button somewhere which deletes internal links without a router_path. This can only occur on update, menu_link_save bails out normally if router_path is not found.

I'm not currently sitting at my LAMP testing computer, but I've attached my 5.x (pre-upgrade) menu table to another issue: http://drupal.org/files/issues/menu-table.sql_.txt

I didn't test the patch here yet, though; just providing another testing data, if useful. Unpatched 6.x-dev gives me duplicate link for 'admin' (old one staying, along with some of the descendants inside, plus new one added). Going to test with patch this evening.

I am feeling this patch is RTBC and we have a misunderstanding of what the menu upgrade can and can not do. Let me say this again: bogus links can not be identified at core upgrade time because there is no way to discern between "contrib disabled months ago" and "contrib disabled for upgrade". If you have a link which belongs to a core module and is duplicated or is not place where it should then set it back to CNW. If all you have is "wow this link was not here in D5 because I disabled / upgraded my modules/core" then please set to RTBC because that's not something we can solve. Yes, manual cleanup of the user menu will be required. Small price to pay, I say.

hass, please send your table to chx1975 at google's mail service.

Re-tested #14, and this time I'm not seeing the duplicate items I was getting without the patch.

The other issues I pointed out are a separate issue --> http://drupal.org/node/196043

This patch is RTBC.

I tested upgrade on both 6.x-dev and 6.x-dev + patch #14. While HEAD still reproduces my problem, the patch was successful - no more duplicates, my menu looks as expected. So patch #14 works fine for me.

Giving time to #25 feedback now, but otherwise this is RTBC for me. Note that I didn't really read and understand the code, I just observed that it works for me.

Hass:

mysql> select path from menu where type=118 group by path having count(mid) > 1 and sum(pid)=3;
+-----------------+
| path            |
+-----------------+
| node/11         |
| node/15         |
| node/17         |
| node/18         |
| node/7          |
| taxonomy/term/4 |
+-----------------+
6 rows in set (0.00 sec)

This is from the menu table dump you sent me. You have your primary links (pid=2) repeated in Navigation (pid=1) in your Drupal 5 database with the same type and the same path. If there is any way the menu upgrade code could know these are bogus it escapes me.

Both JirkaRybka and webernet said we are fine. Your DB dump does not counter this. Expect a little manual work after upgrade.

@haas: menu_links.hidden is new for D6: -1 is a callback (hidden and can never show), 0 is a normal link, 1 is hidden (disabled) but could be re-enabled.

OK, committed this one, continue at http://drupal.org/node/196043 with other issues.