Old, broken menu items cannot be deleted

We make a good effort in _menu_navigation_links_rebuild (thanks to webernet for catching a bug in here) to keep the updated mark only in items that are likely to be broken and then let the delete go through.

I think this is more my issue here then the one i opened at http://drupal.org/node/194585... with a wrong idea about what garbaged my menu tree.

Someone able to provide / explain how the value menu_links.hidden is calculated? I cannot find anything about this in D5 - so i have no idea how D5 knows to make "admin/block" (D4.7.x link) hidden and show "admin/build/block" only...

Those items are going to appear because... well, the reason I have already explained it twice in the other issue, but just for you I will explain at 2:45am for the third time but no more -- I am going to delete comments that parrot "old links appear", OK? Drupal can not discern between menu links to paths belonging to modules disabled/uninstalled/upgraded a year ago or just disabled right now because of the upgrade. Those items will appear no matter how many times you try to bash my poor head and you won't die from deleting them by hand after the upgrade. The issue here is that those items currently can not be deleted. The patch intends to fix that.

I re-read all comments here and in the other thread and one major thing i'm totally missing is - nobody explained - why there is no way to know what is bogus and what not.

1. How is D5 able to detect outdated/bogus admin links?
2. Why are we not able to to remove this outdated/bogus links if D5 is able to remove them (or make them hidden)!?
3. If we remove outdated links why is there any harm?
   1. If a module that is no more installed loose the links in DB i don't see any problem.
   2. And the most interesting - If a module is re-activated every possibly deleted link - found in hook_menu will be re-created, isn't it? Where is a data loss here?
   3. And if all above not possible, why not deleting core links known for sure in update process - maybe with a hardcoded list? (etc. admin/blocks)

Hass, on an installed site, Drupal can check through all links and see what paths are valid and which ones are not. In the update process, users are asked to disable their modules, so it is not possible to do these checks there. Later, once you enabled all the modules you surely need, and you are fine with loosing everything else, which might be dated back later or not, a menu cleanup can be done, as then the system knows again about the valid links. Update is an intermediate stage where we cannot tell.

Hass, you see your D4.7 -> D5 upgrade also left those links in the table, that's why those are still there, so it did not remove them. In D6 however, those are displayed, while in D5, they were not.

#6 3.2. - Where is a data loss here? IMO the lost data is any customizations done. Changed titles, weights, whatever.

About how D5 knew bogus menu links from valid links. As Gabor says once you have all your modules enabled, D5 ran hook_menu(TRUE) and then checked customizations for the returned paths. We do not have those paths at update time. Not to mention these paths can change with when the D6 upgrade finishes. The D6 menu update does something no Drupal did before: it iterates the menu table record by record simply because there is nothing better it can do.

We are talking of manually cleaning up a few links. That surely can't be a big problem. A message maybe about on the menu overview screen... possible, I will add later. But currently the problem is they can't be deleted, so fix that first.

I'm not deep enough inside this menu code part so i cannot say much about this...

The only thing i complain about are the 30+ broken (i counted at minimum 25 (~20 core / 5 old modules) on my box in "admin" and "admin/settings") links from D47 - i need to manually remove today. This is far far away from a "few" what could be theoretical max 9 links.

It would be a very good idea to delete all outdated links we know sure. Something like "admin/blocks" that exists in "admin/build/blocks" could be deleted without loosing anything or am i wrong? From usability side i feel stressed and heavily confused if i see all this dead links. I'm sure many people will open many cases in d.o about this and write about an unclean, difficult and problematic update process and we should not ignore this. My personal first idea was - the site install got broken.

My 2 cents.

hass: As explained, cleaning up is only possible in runtime, so a "menu cleanup" module could be developed which you could run once all the modules you updated are done. (I am not volunteering, just pointing out a possibility).

Yes, a module which offers checkboxes for each item where the updated flag is 1 and a delete button is a certain possibility. Still you will need to manually clean up but said cleanup would be easier that way. I am unsure whether I want to develop said module.

I'm not sure how many users will *find* that module in the current pile of contribs, and then *download* it just for the sake of single post-update cleanup. As for myself, I'll rather go to PhpMyAdmin, if unable to delete from UI.

@chx #9: I think this is something which might be more problematic than first appears - #10 could be a typical reaction to the upgrade process and broken menus are something the vast majority of upgrades will encounter.

In my case I end up with a broken 'Logs' menu and a new 'Reports' menu because I run d5 module "Update Status" which has become d6 core. I suspect many admins are also running this module. Now after the d5-6 upgrade my routine task of looking at the "watchdog" is broken and I'm starting to panic. No problem on a beta site in a sandbox. But an admin upgrading a production site might be reaching for the back-up tapes (and cursing) if it's not easy to understand why this is happening and apply the appropriate fix.

This issue is something that takes time to get one's head around, as evidenced by the repeated rationale here and in http://drupal.org/node/194585. Despite that rationale it could be perceived by many that the menu is being broken by a "faulty upgrade process".

It might need something substantially more visible than "A message...on the menu overview screen". It's the kind of thing we'd want all admins to be watching out for and sorting out as part of the upgrade sequence. Perhaps the information to fix this should be included in UPGRADE.TXT or maybe include a pointer in UPGRADE.txt to a handbook page? I'd say somewhere between items 12 & 14 in UPGRADE.TXT (// $Id: UPGRADE.txt,v 1.10 2007/09/14 20:00:42 goba Exp $) could be a good place.

Or drupal_set_message() in the upgrade process?

@vjordan: Well this should be documented in UPGRADE.txt, but it must be an obtrusive error message in admin section that, do not move away until all menu points have a router path (?)... like the "requirement" hook for modules.

chx: how complicated would it be to provide a "clean up your menu" post update on the menu admin screen, with some help text: "Once you updated all your modules, it is advised to run a cleanup of your menu table, so menu items remained from older versions, but impossible to remove in the standard update process are removed."? This seems to be a problem with which a huge percentage of our user base will be faced.

I have no idea what problems webernet had. Review and commit this one and I will write a cleanup tab, OK?

chx: patch looks workable IMHO, and I don't see what might have caused problem for webernet.

hass, webernet: can you please re-test with #2? ("Unfortunately" I always stayed away from using menu module in most projects, so I don't have a useful data set to test with).

Patch works fine for me.

webernet: we got the other issue fixed in the meantime :) Please test again, if you can.

That's not the intention as far as I see.

After playing with webernet's table a lot, I am sure the patch does what the patch should. Example

mysql> select menu_name,mlid,plid,link_path,router_path from menu_links where link_path like '%66%';
+---------------+------+------+---------------------+-------------+
| menu_name     | mlid | plid | link_path           | router_path |
+---------------+------+------+---------------------+-------------+
| primary-links |   18 |    0 | noded/6666666666666 |             |
+---------------+------+------+---------------------+-------------+
1 row in set (0.00 sec)

This is after I changed node/6 to something that does not have a router_path for sure. So no, the patch does not delete what not should be deleted. That you do not see it? Well, you will when you switch on a module which gives it a router_path until then it fails access check.

Ah well, let it CNR. I hope webernet will second my findings.

OK he pointed me out some missing links.

I forgot about the cleanup at the end of menu and there was the menu_delete_item , of course. I was keeping an eye on customized items but the uncustomized items were failing.

We can do better though if we move the module checking to the cleanup query.

That query was there to kill the items put in by the first menu_rebuild which is now gone. Because what is module if it's not system? Menu. But then customized is 1. But then this query wont' affect anything. Gone. This damned update is so hard!

I think this patch was pretty heavily investigated and reviewed. Although I don't have a good data set to test myself, I trust webernet's and chx's judgement on this that it is ready.

It would be great to get that cleanup tab in a follow up, so the deletion will not be a manual process.

This patch does not work with Drupal 6.4. Can anyone confirm this?