Menu links are sometimes not properly re-parented

Well done. As there can be no item with plid 0 this wont break any existing, legit link and if you have an issue, I can accept this.

A patch for D7. - Although I rather badly need this fix in D6, since it is blocking an admin_menu 3.0 release.

While the code in D7 is slightly different, I would probably have found the cause much earlier, since it literally skips the entire parent query when plid is not > 0.

Yeah, good one.

This sounds like quite the edge case. I'd really like test coverage for this or we are incredibly likely to break it again someday.

sun: Can you see if you can replicate this with a small test module that does hook_menu_alter()?

There's even more to it. :(

I hope the added docs give you a clue. At least, I've tried to find a sane degree of verbosity to explain the very yummy issue without ending up in a novel.

Same patch for D7.

Re-introducing the cached menu router + passing that info on to hook_menu_link_alter() implementations. Not sure whether that still works as it should, but at least, the functions still exist and it seems that the menu router cache is still populated at the same time as in D6. So let's see what the bot thinks.

I'm seriously right before resignation. As #132524-5: Port Admin Menu to 6.x documents, I'm trying to understand the innards of this menu system since December 2007, but I still horribly fail to do so. Because of that, I am unable to change or improve the system -- not even its documentation. It is probably one of the worst documented sub-systems we have in core. Don't get me wrong, I'd love to help - but I simply don't have a PhD degree, or whatever it takes to truly understand the flow and inner processing of this system. I can only guess that this is also the reason why there are literally zero changes between D6 and D7 (aside from the DBTNG, registry, and documentation changes)...

Note that this comment is a straight 1:1 copy from D6. Keep it comparable, as long as no one understands it.

The last submitted patch failed testing.

I don't really see the value of adding back the 3rd param to hook_menu_link_alter() - we only left it in D6 so as not to break the function signature.

Also, the patch here seems to try to fix up the data after a bug has occured, versus solving the true bug. It sounds like the real bug stems from not anticipating that a link would move to hidden = -1 becuaqse the corresponding router item was altered from a MENU_NORMAL_ITEM to a MENU_CALLBACK

Another try.

the real bug stems from not anticipating that a link would move to hidden = -1 becuaqse the corresponding router item was altered from a MENU_NORMAL_ITEM to a MENU_CALLBACK

Perhaps. Perhaps not. It sounds sane, but I simply can't tell, because I have absolutely no clue where a parent router item checks for potential children (or whatever is required to prevent a MENU_CALLBACK becoming hidden = -1).

The last submitted patch failed testing.

Last try before I seriously give up and mark admin_menu's 6.x releases as unsupported.

pwolanin stated in IRC that I would assume the current menu system would work. Of course, I do. Otherwise, I wouldn't have tried for months to find the cause in my code.

webchick suggested that I should try to ping acquia or Dries or whatever. Not sure what could get out of that. I assume they rather focus on the D7 Toolbar.

Tests pass. Code review needed.

To clarify further.

The architecture of admin_menu 6.x-1.x is most likely the cause for a bunch of Drupal core bug reports:

#499614: menu_rebuild() uses lots of memory
#311626: admin/build/modules very slow on some configurations

+ more. I think I waded at least once through Drupal core's queue and marked a lot of other issues as duplicate. In addition to those, I'm still marking 3-4 issues per month in admin_menu's queue as duplicate/by design/won't fix.

All of those issues are caused by the design of admin_menu 1.x, which deletes + copies all administrative menu links from scratch on every single menu rebuild. This action leads to a vast increase in memory consumption.

admin_menu 3.x has been rewritten to entirely re-use the menu system. But users report that some menu links are completely missing or that many links are output in a wrong location, which is caused by this issue (it's possible that there are even more bugs in the menu system, but at least that's one of the issues I was experiencing myself).

To summarize:

- admin_menu 1.x adds a lot of burden to Drupal core's queue, adds negative karma to Drupal, and should vanish as soon as possible. It can only be used on sites that have a very high memory limit (or none at all).

- admin_menu 3.x cannot be released due to bugs in the menu sytem.

Lastly, it's possible that #149562: Menu module causes duplicate menu items may be a duplicate of this issue.

I can finally confirm that this patch fixes the re-parenting.

@sun - I have had perfectly fine experience using admin_menu 6.x-1.x, so unless there's a recent regression I don't understand why it causes such angst.

The patch to move menu_rebuild off the modules page would likely help it.

I think the reason plid ends up zero is that we are setting it that way. See menu.inc line 1961 in function _menu_navigation_links_rebuild($menu)

I think it is this code that needs to be altered or fixed - this assigning plid to 0 was the least effort way to having functional code when we previously worked on fixing reparenting issues, but probably should have left a todo there.

note, I'm not totally sure this is the right way to go - previous I set plid = 0 (I now recall) since I assumed we did not want to have moved links end up someplace mysterious in the hierarchy based just on path. However, perhaps this is better behavior.

That may be related, but that portion of the code only deals with (existing) items that have a menu_name mismatch:

I don't think the menu_name plays a role. It's rather the unwanted re-parenting of children when a parent (mistakenly or not) is or becomes a MENU_CALLBACK.

I'll try to come up with a test module.

see code comment...

But you may be right to some extent. Though that may be another issue.

Currently, the re-parenting fails for the aforementioned (MENU_CALLBACK having children) condition. That I need to prove with a test module, but I was able to repeatedly replicate the behavior - for items that definitely had the same menu_name already stored in {menu_links}.

However, it's perfectly possible that this condition in _menu_navigation_links_rebuild() is the cause for the bug users reported after upgrading from 1.x to 3.x - because the menu_name changes from 'navigation' to 'admin_menu' there. That means, the condition translates to FALSE, because the 'menu_name' is not empty and it's different to the existing.

But the item did not really lose its parent. Rather, similar to the other code in my patch, we're relying on potentially stale data in {menu_links} in the database from a previous rebuild - as far as menu router items are concerned. hook_menu() passes a perfectly matching 'menu_name' for the "existing" router item, and only the old data in {menu_links} no longer applies.

Hence, a potential amendment could be to only test the menu_name if the item wasn't customized.

Subscribing.

It would be nice to totally ditch "customized" and the reset functionality imho.

this is possibly a dupe of #408482: Menu items do not follow parent when moving to new menu . . .

subscribing

Subscribing

.

Subscribing.

Subscribing

Subscribe. Patch from #13 looks good to me (though I also have trouble wrapping my head around the menu system so the best I could do is review the code and try to break it). I assume we're still just waiting on tests cases?

Subscribing.

Subscribing.

Please, others.
Let sun be happy. Apply some patch faster - he proves what he says and the patch seems ok.

Quote from above sun's words:
"To summarize:

- admin_menu 1.x adds a lot of burden to Drupal core's queue, adds negative karma to Drupal, and should vanish as soon as possible. It can only be used on sites that have a very high memory limit (or none at all).

- admin_menu 3.x cannot be released due to bugs in the menu sytem.

Lastly, it's possible that #149562: Menu module causes duplicate menu items may be a duplicate of this issue."

@rsvelko - the goal is to get the code right. It is likely duplicate with the issue moshe linked, so we sould merge the work to there. Anyhow, this is a bugfix that we won't worry about till next week at the earliest.

(Post deleted, I was in the wrong issue queue.)

Why use the slow and buggy core menu functions?

I have not tried the patches you propose, but I want to propose a different approach for admin_menu_rebuild.
- Starting with the $menu array created by menu_router_build(), build a menu tree for admin_menu in memory.
- Invoke admin_menu_alter to let other modules modify this tree.
- Delete all from menu_links with menu="admin_menu"
- write the new admin_menu tree directly into the menu_links table, without using menu_link_save, in a big all-at-once INSERT query. No need for menu_link_save, you can do all of this manually in a much more efficient way.

Only side effect is that people can't manually add new items to admin_menu. But who wants that?

If you still want to allow people to manually add menu items in admin menu, you could allow to merge another menu into admin menu when it is loaded.

I once wanted to fix menu_navigation_links_rebuild, and was just as confused as you say to be now. I recommend not to mess with or rely on menu_link_save and menu_navigation_links_rebuild, but instead just do a simple and robust manual solution based directly on hook_menu / menu_router.

----

One confusing aspect of the menu system is that it's actually two separate things: menu links and router paths.

The hook_menu implementations are in fact what we know as routes in django or Zend or symfony, and the menu_router table is all that information written to the DB. Router items are not the links displayed in a menu, but they usually contain hints for menu building, such as "I want to be in a tab", or "The menu link title should be [...]". The hook_menu hook should rather be called hook_route or similar, and the menu_router table should rather be called router_paths or just routes..

The menus that you create manually live in the menu_links table, and have little to do with menu_router.

The navigation menu (menu_navigation_links_rebuild) can be seen as a third player, which uses data and hints from menu_router / hook_menu to build menu links. It would be logical to implement this thing as a listener to menu_router. The navigation menu links live in the same table as the manually added links. They can be manually modified, but they can also need to be updated whenever there is a change in menu_router. This is where we typically get into trouble.

Most of the issues posted about the menu system show us problems in menu_router_build. But in fact, the menu_router system is the more healthier of the two main parts of the menu system! Right now the menu_router_build process is seriously flawed, but it can be fixed with a quite straightforward solution (see the link below). The system has some crappy implementation, but no structural flaws, I would say.

See Rewrite the function menu_rebuild for a fix to menu_router_build. The issue is in a sleepy mode, because (i) I'm not familiar with D7 and would prefer if someone else could pick up this work, and (ii) I get the feeling that only half of the comments are actually read by someone. The comments in that thread contain a lot of useful information, but I don't feel D7-savvy enough to roll it myself.

It does not have a solution for the menu_navigation_links_rebuild, because this part is much more complex.

----------

Back to admin_menu:

Similar to navigation menu, it would be a good idea to implement the admin menu as a listener to menu_router. Unfortunately, this is not how menu_router works. So until then, admin_menu has to call menu_router_build to get an up-to-date array of router items.

I do not recommend trying to extract anything from the navigation menu in menu_links. This menu is a terrible mix of automatic items from menu_navigation_links_rebuild, and manual modifications or added items. Instead, use menu_router, which is the much healthier of the two tables.

--------

EDIT:
I clicked my way to this issue from the admin_menu module page, and totally missed the fact that this issue is about the menu system in general, not about admin_menu.

To clarify:
By "listener" I mean introducing a new hook. Such as

menu_navigation_links_rebuild could then be moved into a new (core) module - such as "system_navigation" (we want to avoid nameclashes with contrib) - as an implementation of the above hook (that would be system_navigation_menu_router_update()).

admin_menu would have its own implementation, that can be more light-weight because it does not need to take into account manually added menu links.

The very cool thing is that you can turn this module off, and thus gain a lot of speed, if you don't need the system's built-in navigation menu (which is very likely if you use admin menu).

The result is that we totally get rid of the menu rebuild bottleneck:
- menu_router_build can be patched.
- menu_navigation_links_rebuild can be switched off if you don't need system navigation menu.
- admin_menu gets its own rebuild function, which can be much faster than menu_navigation_links_rebuild.

@donquixote: You keep on talking about an obsolete implementation in admin_menu 1.x. Please familiarize yourself with 3.x first. Administration menu needs to use regular menu links to allow for menu link customizations/alterations/additions by the user. The approach of only using menu router data is dead.

@pwolanin: Administration menu currently uses hook_menu_alter() to trick normally invisible local tasks into the visible menu tree, which in turn makes the condition if (!$data['link']['hidden']) { in menu_tree_output() equate to TRUE. Due to the vastly revamped menu system functions in D7, I now realize that it could probably use menu_tree_output() directly by implementing a custom version of menu_tree(), which also contains local tasks/actions in the visible list of links. That, however, doesn't solve the improper re-parenting when menu links are saved...

@sun - this issue is just about the menu system bug right? what's admin_menu have to do with it. I think the fix is correct, I'd like a better test though.

@donquixote - if you are actually interested in this, roll a patch for Drupal 8, since it's too late for Drupal 7.

@sun: If you want to allow manual customization, then you are right. I did not consider this as a requirement. So, it's actually a good thing to fix the menu_link_save. However, I still wonder if it would be possible to do the menu_navigation_links_rebuild without the expensive calls to menu_link_save. It's slowing down not only the modules page, but also the content type management. Of course, having to care about manually customized links makes this a lot more difficult than menu_router_build.

@pwolanin: I don't think it's too late, not even for D6. Introducing a hook inside menu_rebuild would not be noticed by any module that does not implement the hook. The only negative thing could be name clashes. On the other hand, admin_menu (in its old implementation) was the only existing use case for this, so maybe we don't need it.

Anyway, I apologize for leading off the path of this issue.

Had a brain-dead debugging session with smk-ka today. Before I forget about the details until I manage to actually work on this, here are the findings:

A test module should cover:

1. Implement hook_menu_alter() using a conditional alteration for testing purposes to reflect a real-world issue. That being:
     $items['admin']['type'] = MENU_CALLBACK;

   Rebuild the menu. Afterwards, (un)set a variable, so the alteration no longer happens. And rebuild the menu again.

   Current result: All menu router items below admin/ are located at the top level. The (now longer altered nor customized) 'Administer' link is displayed next to them.

   Expected result: Uncustomized router items below 'Administer' are displayed below 'Administer'.

   Note: The same applies to items deeper in the tree, not only to items below the top level.

2. When trying to fix the previous point, it's very likely to break something different:

   Implement hook_menu(), containing a parent item that has 'menu_name' set, followed by child items that do not have a 'menu_name' (@see 'node/add').

   Potential result: The parent item appears in the proper menu. But children appear in the 'navigation' menu.

   Expected result: Children follow their parent, unless being customized.

3. When trying to fix the previous point, it's very likely to break something different:

   Move the link admin/content/comment ([Moderate] Comments) to the Navigation menu; using the admin/structure/menu/item/... form.

   Potential result #1: Neither the link, nor its children are moved at all, but marked as customized.

   Potential result #2: The parent link (admin/content/comment) is properly moved to the Navigation menu, but children vanish.

   Expected result: The parent link is moved to the Navigation menu and children follow that parent.

4. When trying to fix the previous point, it's very likely to break something different:

   Move the link admin/content/comment ([Moderate] Comments) to the Navigation menu; using the admin/structure/menu/item/... form.

   Set a variable to trigger a new menu router item in the test module that is located below admin/content/comment. Rebuild the menu.

   Potential result: The link for the new router item is entirely displaced.

   Expected result: The link for the new router item follows the parent of the existing (and customized) router item.

Please note that all of these are not hypothetical findings. Contributed modules can screw up a lot. And. Any data, once stored in {menu_links}, is permanent, because we always base assumptions on existing menu link data. Due to that, it's not at all like we could fix this "for new links only" -- due to its design, the menu system needs to properly handle already screwed up data, because currently, the only way to get out of that mess is to TRUNCATE {menu_links}; and invoke menu_rebuild() two times afterwards. That is, because 'hidden', 'menu_name', and 'pX' columns can contain wrong values from a(ny) previous menu rebuild, which is ultimately taken into account for subsequent rebuilds.

Attached patch gets at least to solve 3.1, but starts to fail at 3.2. Not at all complete, but may give a clue.

I owe some beers the one who fixes this. :)

The last submitted patch failed testing.

oopsie - merged that debugging code wrongly.

I'm curious whether this patch even passes the tests. If it does, then this entire re-parenting stuff isn't covered by tests at all.

Please additionally note that menu tree output is currently broken in D7 - to be fixed by #566094: menu_tree_data() doesn't build proper hierarchy

The last submitted patch failed testing.

This patch merges all patches from this issue as well as the patch from #566094: menu_tree_data() doesn't build proper hierarchy (which is required to fix this issue).

It should hopefully pass - but doesn't contain the new tests outlined in #41 yet.

The last submitted patch failed testing.

Re-rolled. Still missing tests.

Subscribe

Subscribing. I can confirm the bad behavior resultng from admin_menu on Drupal-6 on at least four sites. Would love to see a fix for this in the next Drupal-6 point release.

sun, your patch in #48 is for D7 correct. Is there a D6 issue open? I'd be willing to help as it's causing me major headaches in taxonomy menu.

@indytechcook: the hope is that we can get it committed into D7 and then look at getting it into D6.

Subscribing
Big fan of seeing this fixed

The last submitted patch failed testing.

oh I know those failing DBLog tests. Has nothing to do with this patch. Re-uploading the latest.