Problem/Motivation

  • If the node/add menu item is moved to another menu than Navigation, its children items are not moved with it. See comment #142for details.
  • Moving the node/add menu item outside the Navigation menu is not properly registered in the Database, since newly created content types are not grouped under node/add in its new loacation, but instead inserted as top level items in the Navigation menu.
  • Manually moving a content type's menu item outside the Navigation menu is possible, but if the content type is later deleted, the menu item is not. It can be manually deleted by clicking the "Reset" link, but this causes a duplicated "node/add" item in the Navigation menu which can only be deleted by editing the database. See comment #145 for details.

Proposed resolution

  • Some of the problems outlined in the original post (see below for details) were solved by the patch provided in #105, and committed to HEAD by webchick in (#120).
  • The module responsible for the automatic creation or deletion of menu items for content types needs to do a db query to check where the menu items of the existing content types are located. If duplicate entries exist, it should make an intelligent guess on which is the "right" one.

Remaining tasks

(reviews needed, tests to be written or run, documentation to be written, etc.)

User interface changes

(new or changed features/functionality, modules added or removed, changes to URL paths, changes to user interface text)

API changes

(API changes/additions that would affect module, install profile, and theme developers, including examples of before/after code if appropriate)

---

Original report by sun

Note: It is possible that the following might not be accurate. However, the patch fixes the bug.

Symptom

  • When exposing local tasks in a menu tree, it can (later) happen that those local tasks are rendered on the top-level of the menu tree instead of below their respective parents.

Details

  • The issue is not directly replicable. You need a router item having local tasks, and very possibly, a third-party module that somehow manages to hi-jack the information returned by hook_menu() (via hook_menu_alter()) and turns that router item into a MENU_CALLBACK.
  • The menu system currently tries to reparent any link by searching for the next available parent in the menu.
  • Since our router item link has been turned into a MENU_CALLBACK, the re-parenting fails and the local tasks are stored with a plid of 0 (zero), which therefore makes them appear on the top-level of a menu.

Cause

  • menu_link_save() processes all menu links, but simply checks for
      if (isset($item['plid'])) {

    to invoke the re-parenting process.
  • Aforementioned local tasks have a plid set, but it is '0'.
  • Therefore, no re-parenting happens, and the local tasks end up on the top-level.

Solution

  • Test for
      if (!empty($item['plid'])) {

    to invoke the re-parenting check.
  • This does not affect existing menu links having a plid of 0, because the re-parenting will simply return nothing.

I'd like to mark this critical, because I've spent a lot of time in the last six months with trying to understand what exactly and more importantly, why menu.inc fails.

Files: 
CommentFileSizeAuthor
#192 550254-192.patch2.12 KBjrglasgow
PASSED: [[SimpleTest]]: [MySQL] 57,149 pass(es).
[ View ]
#185 550254-158-test-reparenting-of-menu-links.patch2.01 KBcorvus_ch
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 550254-158.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#182 menu_links_not_properly_reparented-550254-182.patch650 bytesanrikun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch menu_links_not_properly_reparented-550254-182.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#175 menu_links_not_properly_reparented_plus_user_active_trail-550254-175-D7-do-not-test.patch1.91 KBanrikun
#171 menu_links_not_properly_reparented-550254-171.patch650 bytesanrikun
FAILED: [[SimpleTest]]: [MySQL] 36,504 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
#171 user_active_trail-1564388-1.patch1.33 KBanrikun
PASSED: [[SimpleTest]]: [MySQL] 37,110 pass(es).
[ View ]
#167 drupal8.menu-link-parent.167.patch650 bytessun
FAILED: [[SimpleTest]]: [MySQL] 35,406 pass(es), 16 fail(s), and 0 exception(s).
[ View ]
#166 menu_links_not_properly_reparented-550254-166.patch1.93 KBanrikun
PASSED: [[SimpleTest]]: [MySQL] 35,419 pass(es).
[ View ]
#164 menu_links_not_properly_reparented-550254-164.patch2.9 KBanrikun
PASSED: [[SimpleTest]]: [MySQL] 35,066 pass(es).
[ View ]
#135 wrongmenu.png28.61 KBtinny
#105 550254-reparenting.patch12.34 KBDamien Tournoud
PASSED: [[SimpleTest]]: [MySQL] 27,552 pass(es).
[ View ]
#100 550254-reparenting.patch12.04 KBDamien Tournoud
PASSED: [[SimpleTest]]: [MySQL] 26,203 pass(es).
[ View ]
#96 550254-reparenting.patch11.92 KBDamien Tournoud
PASSED: [[SimpleTest]]: [MySQL] 26,204 pass(es).
[ View ]
#95 550254-reparenting-test.patch7.38 KBDamien Tournoud
FAILED: [[SimpleTest]]: [MySQL] 26,201 pass(es), 5 fail(s), and 0 exception(es).
[ View ]
#88 550254-with-upgrade.patch3.74 KBcatch
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 550254-with-upgrade.patch.
[ View ]
#69 drupal.menu_.69.patch2.65 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 25,635 pass(es).
[ View ]
#64 drupal.menu_.64.patch1.62 KBsun
FAILED: [[SimpleTest]]: [MySQL] 18,896 pass(es), 44 fail(s), and 14 exception(es).
[ View ]
#56 drupal-menu.56.patch3.58 KBsun
Passed on all environments.
[ View ]
#55 drupal-menu.48.patch3.31 KBsun
Passed on all environments.
[ View ]
#48 drupal-menu.48.patch3.31 KBsun
Failed: 13646 passes, 3 fails, 0 exceptions
[ View ]
#46 drupal-menu.46.patch7.17 KBsun
Failed: Failed to apply patch.
[ View ]
#43 drupal.menu-reparenting.43.patch2.42 KBsun
Failed: 12684 passes, 114 fails, 28 exceptions
[ View ]
#41 drupal.menu-reparenting.patch2.14 KBsun
Failed: Invalid PHP syntax in includes/menu.inc.
[ View ]
#17 plid-not-0-550254-17.patch650 bytespwolanin
Failed: 12897 passes, 1 fail, 0 exceptions
[ View ]
#13 drupal.menu-link-save-plid-13.patch2.43 KBsun
Unable to apply patch drupal.menu-link-save-plid-13.patch
[ View ]
#10 drupal.menu-link-save-plid-10.patch2.29 KBsun
Failed: 12084 passes, 2 fails, 339184 exceptions
[ View ]
#6 drupal.menu-link-save-plid-6.patch2.41 KBsun
Failed: 12064 passes, 2 fails, 339184 exceptions
[ View ]
#5 drupal6.menu-link-save-plid-d6.patch1.76 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal6.menu-link-save-plid-d6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#2 drupal.menu-link-save-plid.patch876 bytessun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal.menu-link-save-plid.patch.
[ View ]
drupal6.menu-link-save-plid.patch667 bytessun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal6.menu-link-save-plid.patch.
[ View ]

Comments

Status:Needs review» Reviewed & tested by the community

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.

Version:6.x-dev» 7.x-dev
StatusFileSize
new876 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal.menu-link-save-plid.patch.
[ View ]

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.

Status:Reviewed & tested by the community» Needs work

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()?

Status:Needs work» Needs review
StatusFileSize
new1.76 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal6.menu-link-save-plid-d6.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

StatusFileSize
new2.41 KB
Failed: 12064 passes, 2 fails, 339184 exceptions
[ View ]

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)...

+++ includes/menu.inc 16 Aug 2009 01:57:18 -0000
@@ -2086,7 +2086,10 @@ function _menu_delete_item($item, $force
+  // Get the router if it's already in memory. $menu will be NULL, unless this
+  // is during a menu rebuild

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

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new2.29 KB
Failed: 12084 passes, 2 fails, 339184 exceptions
[ View ]

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).

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Closed (works as designed)» Needs review
StatusFileSize
new2.43 KB
Unable to apply patch drupal.menu-link-save-plid-13.patch
[ View ]

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.

Priority:Normal» Critical

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)

        else {
          // If it moved, put it at the top level in the new menu.
          $item['plid'] = 0;
        }

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.

StatusFileSize
new650 bytes
Failed: 12897 passes, 1 fail, 0 exceptions
[ View ]

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.

Priority:Critical» Normal

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

<?php
       
// A change in hook_menu may move the link to a different menu
       
if (empty($item['menu_name']) || ($item['menu_name'] == $existing_item['menu_name'])) {
         
$item['menu_name'] = $existing_item['menu_name'];
         
$item['plid'] = $existing_item['plid'];
        }
        else {
         
// If it moved, put it at the top level in the new menu.
         
$item['plid'] = 0;
        }
?>

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.

<?php
function _menu_navigation_links_rebuild($menu) {
 
// Add normal and suggested items as links.
 
$menu_links = array();
  foreach (
$menu as $path => $item) {
    if (
$item['_visible']) {
     
$menu_links[$path] = $item;
     
$sort[$path] = $item['_number_parts'];
    }
  }
?>

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.

        if (empty($item['menu_name']) || ($item['menu_name'] == $existing_item['menu_name'])) {
        }
        else {
          // If it moved, put it at the top level in the new menu.
          $item['plid'] = 0;
        }

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.

        if (!$existing_item['customized'] || empty($item['menu_name']) || ($item['menu_name'] == $existing_item['menu_name'])) {

Subscribing.

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

subscribing

Project:Drupal core» Anita Kravitz
Version:7.x-dev» 6.x-1.x-dev
Component:menu system» Code
Assigned:Unassigned» translector
Status:Needs review» Needs work

Subscribing

Project:Anita Kravitz» Drupal core
Version:6.x-1.x-dev» 7.x-dev
Component:Code» menu system
Assigned:translector» Unassigned
Status:Needs work» Needs review

.

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

<?php
/**
* @param $menu array
*   The menu array, as generated by menu_router_build.
*
* @param $changes object
*   Changes in menu_router since the last update. Contains the arrays
*   $changes->insert, $changes->update, $changes->delete
*/
function hook_menu_router_update($menu, $changes);
?>

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.

StatusFileSize
new2.14 KB
Failed: Invalid PHP syntax in includes/menu.inc.
[ View ]

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. :)

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.42 KB
Failed: 12684 passes, 114 fails, 28 exceptions
[ View ]

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

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new7.17 KB
Failed: Failed to apply patch.
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.31 KB
Failed: 13646 passes, 3 fails, 0 exceptions
[ View ]

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

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.31 KB
Passed on all environments.
[ View ]

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

StatusFileSize
new3.58 KB
Passed on all environments.
[ View ]

Additionally, please note that I had to merge this patch into #631550: Stale + improper logic for MENU_VISIBLE_IN_TREE and MENU_VISIBLE_IN_BREADCRUMB to make the latter one work.

Doing so revealed that the re-parenting check for $item['plid'] is entirely wrong in the current code, but also in the latest patch of this issue.

Subscribing

Subscribing

@pwolanin (#39)
I made a new issue for my #36 suggestion.
#653784: Separate out menu links from hook_menu

@donquixote - I think they should be in a separate inc file, but why do you suggest a module? In any case, not going to happen for D7.

I tried this patch from #56 on my drupal-6.15 system and it failed. worse, it messed up my system for a while. I wasn't able to run a lot of items 'cos the menu wasn't working right, it wasn't until I ran update.php that the menu was restored.

patch in #56 is for Drupal 7, not Drupal 6. In any case, you might need a menu rebuild - which it sounds liek you triggered via update.php

Re-test of drupal.menu-link-save-plid-13.patch from comment #13 was requested by kurokikaze.

StatusFileSize
new1.62 KB
FAILED: [[SimpleTest]]: [MySQL] 18,896 pass(es), 44 fail(s), and 14 exception(es).
[ View ]

This is what remains - basically back to and (almost) the same patch I originally posted in Aug 2009.

Only added one tiny tweak to also account for a given/specified plid that results in no $parent, in which case the regular link path traversal should happen.

This patch makes 60% sense, and 40% not. Will possibly fail. Not sure how to resolve the remaining 40% yet.

Status:Needs review» Needs work

The last submitted patch, drupal.menu_.64.patch, failed testing.

thank you for all of the time and hard work that has been put into this issue. i have been struggling with it for just a couple months and thought i would lose my mind! so glad to find this and realize i'm not alone! please keep the updates coming...

*update*
i just found out that the culprit of my menu jumbles was actually drush v2.1. the issue would only occur when running drush update (not updatedb or updatecode, just update). after a ridiculous amount of wasted time, i got the cue to upgrade drush. i'm now running drush v3.0-rc2 and have not been able to replicate the menu issue! hope this helps...

subscribing...

If I'm not totally wrong here then at least one of the tests actually tests a wrong behavior...

in testMenuNodeForWidget(), we create a menu link with the following code

<?php
   
// Edit the node and create a menu link.
   
$edit = array(
     
'menu[enabled]' => 1,
     
'menu[link_title]' => $node_title,
     
'menu[weight]' => 17,
    );
   
$this->drupalPost('node/' . $node->nid . '/edit', $edit, t('Save'));
?>

And then we check if the link is displayed. However, we actually have set navigation:0 as the default parent a few lines above

<?php
   
// Change default parent item to Navigation menu, so we can assert more
    // easily.
   
$edit = array(
     
'menu_parent' => 'navigation:0',
    );
?>

The Navigation menu is however not enabled by default. We pass then the following to menu_link_save():

array ( 'enabled' => 1, 'mlid' => 0, 'module' => 'menu', 'hidden' => 0, 'has_children' => 0, 'customized' => 0, 'options' => array ( 'attributes' => array ( 'title' => 'pPc9vavd', ), ), 'expanded' => 0, 'parent_depth_limit' => 8, 'link_title' => 'pPc9vavd', 'parent' => 'navigation:0', 'weight' => '17', 'plid' => '0', 'menu_name' => 'navigation', 'link_path' => 'node/1', 'external' => 0, 'updated' => 0, )

As you can see, plid is set to 0. The old code tried to load the parent with mlid 0, failed and did not use any parent at all. The new code does that too but then correctly looks for a new parent in the navigation menu. When I change the menu parent default to 'main-menu:0', this passes the test.

Debugging the other test fails now...

Status:Needs work» Needs review
StatusFileSize
new2.65 KB
PASSED: [[SimpleTest]]: [MySQL] 25,635 pass(es).
[ View ]

Ok, the other fails are correct. The simplified approach in #64 does not differentiate between menu items that we *want* to show up in the top-level (added or moved) through the UI or modules. To be exact: it is not possible to do so, the provided data may be exactly the same.

Because of that, I'm proposing the following: We only guess the parent for system (hook_menu() provided) links, because they are the only ones that are supposed to be parented by path by default. I can create a path to /profile/whatever/I/want, place that below /forum. Now, if I disable the forum module and /forum is removed, my custom link would be re-parented below /profile (which might even be disabled). This is just a blind guess and does imho confuse more than it helps. Instead, we should imho let the user choose to re-parent it as he want and move it to the top for now.

Another solution would be to let the link correctly bubble up the hierarchiy, but I don't think that is technically possible since we can't access that information. Example: if we have linkA, linkA/linkB (hierarchy is only virtual not based on path!) and linkA/linkB/linkC and then something removes linkA/linkB, re-parent linkA/linkB/linkC to linkA. I've just tried that through the UI and it works but I suppose menu.module is re-parenting linkC before deleting linkB.

After all, I'd say this is an edge-case of wrong API usage. Our policy as far as I understand is that we don't guarantee anything if the API is used in a wrong way. For example, if you define $form['some_field']['attributes']['class'] = 'string instead of array', Drupal 7 dies with an ugly, not helpful PHP Fatal Error. How is this any different?

Don't get me wrong, I'm all for handling this as smooth as possible, but I'd say we do this just because we're nice and want to provide a good DX, not because it's a bug.

Anyway, the attached patch implements my proposal above and also fixes the wrong test. The tests that failed in #64 pass now, let's see about the rest...

This makes sense. Re-parenting of custom menu links happens through other means + functions, so limiting this to links derived from router items makes perfectly sense.

subscribing...

sub

Status:Needs review» Needs work

The last submitted patch, drupal.menu_.69.patch, failed testing.

Status:Needs work» Needs review

#69: drupal.menu_.69.patch queued for re-testing.

Bump, looking for reviews :)

#69: drupal.menu_.69.patch queued for re-testing.

To which Drupal version does this patch apply? I tried 6.16, but I get a lot of rejects for menu.inc.

I found this issue because I am building Menu Wizard to allow admins to change all hook_menu() properties through the interface (the code itself uses hook_menu_alter()). When I change a local task to a normal item, it appears under its grandparent rather than under its parent. This is because menu_link_save() tries to find the mlid of the item's parent, but after that the parent gets a whole new mlid, orphaning the child item. Somewhere else its plid gets set to the mlid of its grandparent. I tested this with menu links created by System.module.

Is this the same thing that this code reports? It’s saying that I have 88 orphaned links (including all the links in orphaned branches).

<?php
/*
* Implementation of hook_menu()
*
*/
function menu_validator_menu() {
  $raw = array();
  _menu_validator_flatten_tree(menu_tree_data(db_query("SELECT * FROM {menu_links} ml")),$raw);
  $sanitized = array();
  foreach(menu_get_names() as $name) {
    _menu_validator_flatten_tree(menu_tree_all_data($name),$sanitized);
  }
  $orphans = array();
  foreach($raw as $mlid => $link) {
    if (!isset($sanitized[$mlid])) {
      $orphans[$mlid] = $link;
    }
  }
  if (count($orphans)) {
    watchdog('menu_validator',
      t('Menu data corrupted; %orphans orphans'),array('%orphans' => count($orphans)),
      WATCHDOG_CRITICAL);
  }
}
function _menu_validator_flatten_tree($branch, &$links) {
  foreach($branch as $link_branch) {
    $links[(int)$link_branch['link']['mlid']] = $link_branch['link'];
    if ($link_branch['below']) {
      _menu_validator_flatten_tree($link_branch['below'],$links);
    }
  }
}

#69: drupal.menu_.69.patch queued for re-testing.

@Xano: This patch is against D7.

@Daniel Norton: No, that "validation" code looks completely bogus. I'm not sure what it tries to validate, and whether that makes sense in any way, but it's definitely not related to this issue.

@Berdir: All that's needed here seems to be an approval by @pwolanin.

Status:Needs review» Needs work
Issue tags:+Needs tests, +D7 upgrade path

I'm constantly hitting this issue all over again via other D7 issues currently. I'm quite sure that many users will face this bug when upgrading to D7, as many router items have changed, but the menu system fails to properly recognize and update them.

Latest patch looks RTBC already. However, we still need to turn the manual testing procedure outlined in #41 into tests.

Priority:Normal» Major

I tried to write a test for this, but I can't get it to work as expected without the patch. Meaning, the menu items below administration aren't messed up. Am I missing a step in the test, maybe visiting the menu admin page or something like that?

<?php
/**
* Implements hook_menu_alter().
*/
function menu_test_menu_alter(&$items) {
  if (
variable_get('menu_test_break_admin', FALSE)) {
   
$items['admin']['type'] = MENU_CALLBACK;
  }
}
  function
testMenuReParenting() {
   
$admin_user = $this->drupalCreateUser(array('administer site configuration', 'administer permissions', 'administer users'));
   
$this->drupalLogin($admin_user);
   
// Assign user to the administer role.
   
$roles = array(
     
'roles[3]' => TRUE,
    );
   
$this->drupalPost('user/' . $admin_user->uid . '/edit', $roles, t('Save'));
   
// Break /admin by changing it to a callback.
   
variable_set('menu_test_break_admin', TRUE);
   
menu_rebuild();
   
$this->drupalGet('');
   
// Make sure the "Administer" link is no more.
   
$this->assertNoText(t('Administer'));
   
// Restore original menu.
   
variable_del('menu_test_break_admin');
   
menu_rebuild();
   
$this->drupalGet('');
   
// Make sure the "Administer" link shows up again.
   
$this->assertText(t('Administer'));
  }
?>

This works and hides the Administer link after the first rebuild and displays it again but that's it.. no other menu items show up in the top level or something like that...

mmm... it would really help if we would have regular patches here :-/

If we keep on not being able to reproduce this bug, then I guess my patch in #631550: Stale + improper logic for MENU_VISIBLE_IN_TREE and MENU_VISIBLE_IN_BREADCRUMB fixed it already. If that is really the case, this patch remains as a performance improvement -- the menu should actually rebuild much faster on sites having plenty of custom menu links (i.e. not derived from router paths).

I was able to replicate this bug after applying the patch from #735692: Unclear which themes blocks are edited - shown as secondary tabs without primary parents

After additionally applying this patch + rebuilding the menu, the stale/invalid menu links disappeared! So this patch is still needed and still working.

...so, according to #81, all we need to get this to RTBC is #41 converted to tests. Right? Isn't Sascha in #83 trying to do exactly that? He seems to be stuck at something. I wish I knew how to write tests so I could help.

Status:Needs work» Needs review

#69: drupal.menu_.69.patch queued for re-testing.

StatusFileSize
new3.74 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 550254-with-upgrade.patch.
[ View ]

Combining this with the upgrade path test hunk from #932134: No upgrade path for MENU CALLBACK API change to see what happens.

Status:Needs review» Needs work

The last submitted patch, 550254-with-upgrade.patch, failed testing.

+  // If this link defines a parent link id, try to find it in the current menu
+  // links. If found, we use it for the re-parenting process below. This parent
+  // link validation also needs to be done for links specifying a plid of 0
+  // (zero), since {menu_links} may contain stale/bogus data caused by a
+  // re-parenting process that went wrong in a previous rebuild.

I just don't get that (and the corresponding code change).

The rest of the patch (basically automatically reparenting links that points to invalid parents) seems perfectly valid.

I think I understand what the lenghty comment in #90 is all about: you want to first try to load the menu link (optionally) specified by the plid, then if this fails (because the plid was not specified or because it was pointing to an invalid link), want to reparent automatically.

That's fine for me, but:

(1) we don't need 4 lines of comments to explain that
(2) we certainly don't need to try loading a plid of 0

So, just change the isset($link['plid']) by a !empty($link['plid']) and be done with it.

Status:Needs work» Needs review

#88: 550254-with-upgrade.patch queued for re-testing.

Status:Needs work» Needs review

I have the same error, when I upgrade from D6 to D7.
You can see the duplicated issue #934956: Upgrade D6->D7 Beta1 don't show any content type.

upgrading the D6 to D7 CVS 10-10-2010 don't fix my problem.

i try to investigate more, and I see that in the D6 I have this:

(admin/build/menu-customize/navigation)

In D7 upgrade, I have this:
(admin/structure/menu/manage/navigation)

(admin/structure/menu/manage/management)

So, in the D6, I have the Create content and the content node types in the navigation menu
When I upgrade to D7, I have the Create content in the navigation menu and the content node types in the management menu without parent.

Tópico forum = forum
Votação = poll

I need a patch that apply's

Status:Needs review» Needs work

The last submitted patch, 550254-with-upgrade.patch, failed testing.

Issue tags:-Needs tests
StatusFileSize
new7.38 KB
FAILED: [[SimpleTest]]: [MySQL] 26,201 pass(es), 5 fail(s), and 0 exception(es).
[ View ]

Back to the basics. Here is a (kind of) comprehensive test for automatic menu reparenting.

This will fail. Let's discuss how to fix it.

StatusFileSize
new11.92 KB
PASSED: [[SimpleTest]]: [MySQL] 26,204 pass(es).
[ View ]

And here is the test + a fix proposal.

+++ includes/menu.inc
@@ -2890,42 +2890,8 @@ function menu_link_save(&$item) {
-    $query = db_select('menu_links');
-    // Only links derived from router items should have module == 'system', and
-    // we want to find the parent even if it's in a different menu.
-    if ($item['module'] == 'system') {
-      $query->condition('module', 'system');
-    }
...
+  // Try to find a parent link. If found, assign it and derive its menu.
+  $parent = _menu_link_find_parent($item);
@@ -3047,6 +3013,76 @@ function menu_link_save(&$item) {
+function _menu_link_find_parent($menu_link) {

Hey, I actually thought of something very similar to this :)

Technically, the current re-parenting logic is only valid for links that have been derived from router items, which means 'module' == 'system'...

Technically, modules like Book and Menu definitely could have their own re-parenting logic...

So this sounds like a hook_menu_link_parent() to me.

(Actually, I think that menu.module even contains some code somewhere that re-parents menu links when one is changed or deleted through the UI.)

Powered by Dreditor.

But then again, that new logic looks pretty solid already, and likely works for non-system links, too.

However, not sure about the special-casing of 'plid' = 0. Each time I removed that, it fixed some very hard to reproduce bugs in the re-parenting logic. Note that #631550: Stale + improper logic for MENU_VISIBLE_IN_TREE and MENU_VISIBLE_IN_BREADCRUMB contains additional details on that.

However, not sure about the special-casing of 'plid' = 0. Each time I removed that, it fixed some very hard to reproduce bugs in the re-parenting logic. Note that #631550: Stale + improper logic for MENU_VISIBLE_IN_TREE and MENU_VISIBLE_IN_BREADCRUMB contains additional details on that.

If plid is explicitely 0, it's a top-level link. End of story.

If there are bugs elsewhere, we can and should fix them elsewhere.

StatusFileSize
new12.04 KB
PASSED: [[SimpleTest]]: [MySQL] 26,203 pass(es).
[ View ]

So this sounds like a hook_menu_link_parent() to me.

That would be a feature request, and I'm not sure it has a lot of value. This re-parenting logic is just a corner case: what do we do when the data structure of the menu is inconsistent. We have seen in the D5 to D6 migration (the book module issues) how important it is to keep the structure consistent if we can.

What we failed to do during D5 to D6, I hope we can do here. Let's please not derail this issue.

Rerolled the patch to fix an incomplete comment.

I test #100 path, but I don't know how to test it.

I press save in the module page. But don't fix nothing.. Will I have to change the menu manualy or upgrade d6-d7 again?

I just tested the upgrade path to D7 for the first time. Got loads of wrongly parented links afterwards.

Tried the latest patch, rebuilt menus -- no (visible) difference.

Additionally commented out the special plid = 0 condition, rebuilt menus -- most (but not all) stale links vanished.

--
Actually, now that I think of it. This issue and patch was and is primarily about stale links that happen to mistakenly end up on the top-level of a menu, so the 'plid' = 0 condition is a very important factor.

Actually, now that I think of it. This issue and patch was and is primarily about stale links that happen to mistakenly end up on the top-level of a menu, so the 'plid' = 0 condition is a very important factor.

Again, no, we cannot do that. plid = 0 means top level. End of story. We cannot "auto-magically" change that.

@Damien: There's no magic involved at all. The code must only affect links of 'module' = 'system'. Those always have to follow the rules, regardless of whether plid is 0, 2, or NULL.

Also, this issue has a focus. While I can relate to your thinking, and one could state that plid can never be 0 when it should not, we cannot blatantly ignore the problem space. The problem space is that a previous menu link re-parenting process already re-parented links wrongly. And that is what needs to be taken into account.

StatusFileSize
new12.34 KB
PASSED: [[SimpleTest]]: [MySQL] 27,552 pass(es).
[ View ]

Added more comments to the test via @chx.

While I can relate to your thinking, and one could state that plid can never be 0 when it should not, we cannot blatantly ignore the problem space. The problem space is that a previous menu link re-parenting process already re-parented links wrongly. And that is what needs to be taken into account.

If menu_link_save() is called with 'plid' = 0, it means that the caller intended for the menu links to be at the top-level. You cannot workaround this fact. I'm sympathetic to your problem, but that's not something that can be solved in menu_link_save()

Status:Needs review» Reviewed & tested by the community

That's better. I was worried someone might think that forcing is 'allowed' hopefullly the words 'database crash' will stop them. The rest is, well, fine.

Status:Reviewed & tested by the community» Needs review

I suggest to read the original description of this issue. There is no caller involved when plid happens to be mistakenly 0 for 'module' = 'system'. This also happens to be the case when upgrading to D7.

Status:Needs review» Reviewed & tested by the community

sun, first of all, i was confused by the original post when i imeddiately rtbc'd this. (I thought you can't define top level items in the rotuer but it's only % that is invald)

Now, Damien's patch might fix a different issue -- I think it does -- but it's a valid fix.

Your issue is impossible to fix because, as you said in the original post "The issue is not directly replicable" therefore it is impossible to roll a bugfix. If it's not reproducible, how can we fix and test the fix? If you can, please post a clear summary of how to reproduce and there will be fix. This I promised you and I hope I can keep it: if you do not post vague claims but concrete problems then a fix happens quick.

Also, as I said a few times already, you cannot just throw away plid, even if module = system. You *can* move a menu link derived from a menu router everywhere in the hierarchy, *including* at top-level (ie. plid = 0). You cannot just randomly decide that all top-level links needs to be reparented automatically because you feel they should not be there.

Status:Reviewed & tested by the community» Needs work

All of you are missing the entire point.

You *can* move a menu link

I'm not moving anything. If you would have read the original post, then you would have understood that any module can arbitrarily hi-jack the menu system without even knowing. Not even knowing that something like a 'plid' key exists somewhere.

Menu links that are derived from router items always have to maintain proper parents, regardless of whether we assume that the underlying data is valid or not.

I can only guess that my earlier comment must have led to this confusion. Of course, the current plid check makes sense for any link that is _not_ 'module' = 'system'. For those links, 0 always means 0. But for 'module' = 'system', that is not necessarily true.

@chx: Learn reading and listening instead of insulting people.

Consider me out of this issue.

I not know what's happen in this issue, but I think that we all have to self control..

I have this issue, because (all the time) when I update D6 to D7 the menu allways is migrated wrong.
Now reading the issue you say that is impossible to fix? I think that is possibly to fix, lets say, if the upgrade is moving the parent from place and not the children

Status:Needs work» Reviewed & tested by the community

Sorry, my follow-up above was poor and not adequate. Apologies for that.

The original issue is primarily about menu links that are derived from menu router items. These can exist in various hierarchies. The corresponding menu links are automatically generated out of the menu router item hierarchy.

Menu router items can change over time.

  • Their type can change. Which potentially moves them out of the hierarchy - or back into it.
  • Their ancestors can change. Which potentially requires to relocate them to stay in the hierarchy.
  • Their menu name can change. Which potentially requires to relocate them in the hierarchy.

When the menu router is built, such changes are already accounted for. However, the corresponding menu links already exist. They potentially need to be updated to reflect their new intended location in the hierarchy (as long as they have not been customized).

Unlike other links, menu links that are derived from menu router items have to be fully re-evaluated upon a rebuild of the menu. Regardless of their current location.

--
An arbitrary example of a possible workflow:

  1. A module is installed.
      $items['top/level'] = array(
      );

    Should the automatically generated link for the path 'top/level' live at the top-level of the menu?

  2. Another module is enabled, or the existing module has been updated and changes.
      $items['top'] = array(
      );
      $items['top/level'] = array(
      );

    Now, should the link for the path 'top/level' still live at the top-level of the menu?

  3. Change the type of the path 'top' to a MENU_CALLBACK.

    Still on the top-level?

  4. Revert the type changes.

    Still on the top-level?

--

But yeah. Although I actively tested this patch on an upgraded site, on which this bug appeared, and already stated that the current patch does not fix the bug, I must be wrong. Meanwhile, I agree that menu_link_save() is not the right location to fix this nebulous bug. Apologies again.

However, I don't really have a clue what kind of bug the current patch is fixing. A description/summary would likely be helpful for core maintainers.

Lastly, this issue should revert to needs work afterwards, as the original bug still needs to be fixed, and yes, it still has to be cleanly reproduced, too.

An arbitrary example of a possible workflow:

[... long description: in a nutshell: if parent appears while parent/child already exists, parent/child is not reparented under parent]

This is pretty easy to solve. We need to force a reparenting as soon as the existing menu link is not customized. This fix (probably a one-liner) should live in _menu_navigation_links_rebuild().

Please open a separate issue for this. There are probably plenty of bugs and expectation discrepancies in this whole menu-links-derived-from-menu-router update process, because it's just a very hard process. We can fix those, but only one at the time and with the required associated tests.

Well, this very issue has always been focused on updating menu links derived from menu router. E.g., #41 and other follow-ups contain in-depth details for trying to cleanly reproduce and fix the original bug; i.e., the topic of this issue. So we either commit this patch here and move back to needs work, or the current patch has to be moved to a separate issue. I'm fine either way, as long as this issue maintains its focus and we do not lose the entire information and analysis that already exists.

Coming here via #934956: Upgrade D6->D7 Beta1 don't show any content type

I've upgraded a D6 install to D7 but existing nor new content types (created in the D7 environment) are presented on node/add:

"You have not created any content types yet. Go to the content type creation page to add a new content type."

When I directly type the url node/add/story I am able to create content, just node/add doesn't list existing content types.

Probably related: Top level admin categories are still D6 style after upgrade: Content management, Site building, Site configuration, User management, Reports

I applied this patch before starting the upgrade. Before upgrading I disabled contrib, set to english language and even did a manual menu_rebuild to reset some of my custom menu item labels in D6.

I have the same problem, with this patch.

I don't know if the upgrade problem(my original issue #934956: Upgrade D6->D7 Beta1 don't show any content type) have anything to do with the menu_link_save() method, or with the upgrade method that create the management type menu.

Since that when I manual correct the menu items, it save correctly..

I am having this issue for some time now in D6, but I didn't know if it was a core issue or caused by some defective module (or a combination of conflicting ones). I was waiting for it to either get solved by any possibly defective module upgrade or by having this fixed in 7.x and then backported to 6.x. Since this is taking too long, I need to ask if it is the same issue (so I can be monitoring this issue + testing patches).

...whow! was that a long prologue or what? To the issue then:

In two different but identical (only difference is the themes being used) production setups of D6.19, with latest dev versions of most common used modules, I am seeing this weird behavior in one of them while not on the other. It involves admin_menu and upgrade_status and how the respective menu entry of the second is displayed in the menu created by the first. In one of the setups I see this structure:

Reports
- ...
- Available updates
- - List
- - Settings
- - Upgrade status
- Status report

In the second setup (the one with the issue) I see:

Reports
- ...
- Available updates
- - List
- - Settings
- Status report
- Upgrade status

Notice how the 'Upgrade status' entry was move from its correct parent (Available updates) to the top-level menu (Reports).

And the question here is: Is this the same issue as described here, or do I need to file a new one for it?

#105: 550254-reparenting.patch queued for re-testing.

Status:Reviewed & tested by the community» Active

Well, I confess I don't really follow what all's going on here, but it's clear Damien's patch is testing the hell out of reparenting and things are still working afterwards. :P From the looks of it though, sun's original bug report is still open.

Committed #105 to HEAD, and remarking to "active".

Status:Active» Needs review

#2: drupal.menu-link-save-plid.patch queued for re-testing.

@vensires, why queued for re-testing one patch that will not work, since a is allready commited that changes and more things in #105 ????

Misclick... Finger over mouse, heavy finger, click. never mind.

Status:Needs review» Active

Subscribing, and resetting back to active based on webchick's comments in #120.

I still suffer from this in latest 7.x dev of core.

Am I right that this bug is the cause of the problem described in http://drupal.org/node/997444? That forum topic describes the problem that when you remove the 'Add Content' menu item, or move it to another menu tree, Drupal tells you "You have not created any content types yet" which is completely inadequate.

Version:7.x-dev» 7.0

I think this is related.

After creating a navigation link such as add-link, adjust hierarchy via parent-link setting. When editing the actual content and saving, the "menu-link" does not accurately reflect the navigation parenting structure. By default if there is a navigation link, the "parent-link" for the content is active, thus on save the "navigation menu" link disappears, and the content link is added to the "main menu".

Scenario
1. Create new "basic content", with Menu Settings "provide a menu link" is unchecked. Save.
2. Add new "navigation" link to side menu navigation, for this new basic content.
3. Edit "basic content". Note that under Menu Settings "provide a menu link" is checked, but the "parent-link" is not accurate, and "Main Menu" is selected rather than "Navigation" as the root. Unable to save without breaking any "Navigation" sub links.

Status:Active» Needs review

Duplicate menu items particularly for All menu's.

Structure
  Menus
      Add Menu
      Main Menu
      Main Menu -> (hover shows sublinks)
      Management
      Management -> (hover shows sublinks)
      Navigation
      Navigation -> (hover shows sublinks)
      User Menu
      User Menu -> (hover shows sublinks)
      Settings

Flushing menu caches does nothing. Installed on vanilla D7.0 with only native modules.

Status:Needs review» Active

I confirm #128 too.

Is this part of this issue, or should I open a new one for it:

  • Fresh Drupal 7.0 install with Standard profile. Edit the "Add content" menu link from the Navigation menu. Change its parent to the Management menu. So far so good. It's no longer in the Navigation menu block (as desired), but going to node/add shows you the node types available.
  • Empty the Drupal cache (causing a menu rebuild). Now all the child links (e.g., node/add/article) are back in the Navigation menu, but now as top-level links, while the "Add content" link is still in the Management menu. Now when you go to the node/add page, it tells you there's no content types.

@effulgentsia: That's only partially related. The menu rebuilding logic for links derived from router items that have been customized is an entirely different can of worms — which almost no one wants to touch. While it's possible that the committed patch of this issue might have unintentionally introduced the issue you mentioned (which worked in the past, AFAICT), let's move that into a separate issue and link to it from here, please.

The menu rebuilding logic for links derived from router items that have been customized is an entirely different can of worms — which almost no one wants to touch.

Yeah, totally, I had the same experience when I was trying to find a solution for "Optimize _menu_navigation_links_rebuild()".

If we could agree on a well-defined spec for "how to deal with customized items", then I guess it would be possible to make a clean rewrite of the menu_links rebuild process, solving both this reparenting problem and the performance problem.

I can confirm #127. Subscribing.

StatusFileSize
new28.61 KB

apparantly http://drupal.org/node/934956 thread is a duplicate of this thread so i'll post here.

my menu item node/add gives me this You have not created any content types yet. Go to the content type creation page to add a new content type.

going directly to node/add/country works as it should.

after creating a new content type called "cities" node/add directs to node/add/cities
after creating and deleting and creating and deleting a bunch of random content types nothing seems to work as it should...

then for some reason, chrome gives me this

This webpage has a redirect loop.
The webpage at http://localhost/describe/node/add/qqqqqqqqq has resulted in too many redirects. Clearing your cookies for this site or allowing third-party cookies may fix the problem. If not, it is possibly a server configuration issue and not a problem with your computer.

i cant delete some menu items that i created. so i reset them, now i have 3 node/add items that i cant delete and they all tell say "You have not created any content types yet."

ive tried rearranging and nesting in different orders and FINALLY for some reason i just happen to nest all of the node/add/xxx under one of the node/add items and it works. ive disabled the other two node/add's

Today I had basically the same experience as #131. I moved "Add content" to the Main menu, where it was working fine, then I moved it to a new Secondary menu I had previously created. This was a day or two ago. Today I created a new content type, and after doing so encountered the "You have not created any content types yet" from all approaches to /node/add.

Moving "Add content" back to the Navigation menu, and moving all the content types there back under "Add content", fixed the situation, but I would really like to move "Add content" elsewhere.

A weird thing that happened concurrently with all this: I had been running the affected dev site in a raw subtheme of Danland. At or around the same time that I initially moved "Add content", a problem with logging in cropped up. Suddenly, logging in put all users on their account page, instead of back on the page they were on before the login. In trying to fix this, I switched to Bartik, which did the trick (I haven't gotten into theming the site yet, so it mattered little). Today, at the same time that node/add stopped working, logging in started putting me at /users/[my_username] again. And that effect is persisting though the node/add issue is "fixed" by reconstructing the Navigation menu structure.

I just tried moving "Add content" from the Navigation menu to my Secondary menu, and it worked like I'd expect, moving all its children with it, and things work as they should, at least for the moment.

A couple differences between this move and the initial time I moved "Add content":
The first time, I moved it to the Main menu (where the children did not follow it, but it still worked). Then I changed themes. Then I moved it to my Secondary menu.

More:
I just created a quick content type for testing (a content type with all default settings). The new content type does not show up at /node/add, but it does show up at /admin/structure/types. Editing the content type to manually put it under "Add content" on my Secondary menu did not help. My new content type's machine name was "testy_type". node/add/testy_type did not work. I changed the machine name to just "testy". Now, node/add/testy works, but it still won't show up at node/add. Clearing all caches hasn't helped.

Status:Active» Needs review

drupal6.menu-link-save-plid.patch queued for re-testing.

subscribe

*subscribe*

Same issue for me, subscribing.

Version:7.0» 8.x-dev
Priority:Major» Critical

Actually, @effulgentsia's example test case in #131 is indeed valid and may be more relevant to this issue than I originally thought.

Steps to reproduce:

  1. Move node/add to a different menu. For example, Management.
  2. Create some new node types.

Actual result:

SELECT menu_name, mlid, plid, link_path FROM menu_links WHERE link_path LIKE 'node/add%'
menu_name;mlid;plid;link_path
"management";"5";"1";"node/add"
"navigation";"410";"0";"node/add/media-gallery"
"navigation";"201";"0";"node/add/page"

The expected result is obvious.

Priority:Critical» Major

Recapping Sun's #142:

Steps to reproduce:

Move node/add to a different menu. For example, Management.
Create some new node types.

Actual result:
SELECT menu_name, mlid, plid, link_path FROM menu_links WHERE link_path LIKE 'node/add%'

menu_name;mlid;plid;link_path
"management";"5";"1";"node/add"
"navigation";"410";"0";"node/add/media-gallery"
"navigation";"201";"0";"node/add/page"

The expected result is obvious.

I actually did this *exact* thing on a site just a few days ago. When moving the "node/add" item to management (I was trying to get them to show up in admin_menu), all the child elements ("node/add/blog", etc.) don't get moved along with it. However all the menu items still continue to work and it's easy enough to correct the problem manually (though quite tedious) by moving every item individually.

There isn't any actual data loss here though and overall this seems like an huge inconvenience rather than a huge problem. I'm re-categorizing issues based on the discussion in #1050616: Figure out backport workflow from Drupal 8 to Drupal 7 (which would make critical and major bugs prevent any new development of Drupal core).

This is a nasty one that has a long history. Once you get duplicated "Add content" menu items, the only way to get rid of them is to manually delete entries from the database. Several forum posts describe how:
http://drupal.org/node/421350#comment-2131174
http://drupal.org/node/388014#comment-1888346

Here are the steps to reproduce what was described in #135:

  1. Fresh Drupal 8.x-dev (August 6, 2011) with Standard profile.
  2. Edit the "Add content" menu link from the Navigation menu. Change its parent to the Management menu. It now shows up in Management with "Article" and "Basic page" correctly nested. (No cache clearing done).
  3. Create new content type "Test"
  4. As a new Drupal user with no knowledge of this bug, I would expect it to show up in the Management menu together with "Article" and "Basic page". Instead it shows up as top level item in Navigation. Meanwhile "Article" and "Basic page" have disappeared from Management, and now also show up as top level items in Navigation. "Add content" remains alone in Management.
  5. I move all 3 items manually to the Management menu below "Add content".
  6. I decide that I no longer need the Content type "Test", so I delete it.
  7. I go to the Management menu, but the corresponding menu item "Test" hasn't been deleted from there as expected. The content type is also still present on the "Add content" page.
  8. I click the "Reset" link to the right of the "Test" menu item. It disappears but now I have a duplicated "Add content" menu item in Navigation.
  9. I create a new content type "Test 2". It appears as menu item below the duplicated "Add content" in Navigation. However, clicking both "Add content" links show the same page with two content types: Article and Basic page.

Result when running SELECT menu_name, mlid, plid, link_path FROM menu_links WHERE link_path LIKE 'node/add%' in PhpMyAdmin:

menu_name       mlid    plid    link_path
-----------------------------------------
management 6 0 node/add
navigation 269 0 node/add
shortcut-set-1 234 0 node/add
management 237 6 node/add/article
management 238 6 node/add/page
navigation 270 269 node/add/test-2

Until this problem gets resolved, maybe a help text could be provided somewhere in the Menu UI warning users about moving the "Add content" item out of the Navigation menu? At least that could spare new users some frustration.

Thanx for the updated status Lars.

Now, issue descriptions (post #0 at the top of the page) are actually summaries that can be edited/updated! This one still points to the initial patch as a proposed solution -way outdated- and I guess needs to be synced with all this latest info. #142 and/or/combination-of #145 above are worth being copy-pasted in the issue summary along with a link to the latest patch.

PS: ...AAMOF, I think that everyone should develop a habit of updating the issues' summaries each time they upload a newer patch and set it to NR.

Status:Active» Needs review

Hi klonos, thank you for pointing this out. I completely agree. I don't consider myself qualified to make a good summary for this issue since I am having a very hard time understanding what is going on until #131. It was #142 by sun that made me decide this was the right issue to post the information in #145.

However, below is an outline for a possible new summary. I hope those with more knowledge than me can fill in the missing parts and correct any mistakes:

<h3>Problem/Motivation</h3>
<ul>
<li>If the node/add menu item is moved to another menu than Navigation, its children items are not moved with it. See <a href="/node/550254#comment-4442028">comment #142</a>for details.</li>
<li>Moving the node/add menu item outside the Navigation menu is not properly registered in the Database, since newly created content types are not grouped under node/add in its new loacation, but instead inserted as top level items in the Navigation menu.</li>
<li>Manually moving a content type's menu item outside the Navigation menu is possible, but if the content type is later deleted, the menu item is not. It can be manually deleted by clicking the "Reset" link, but this causes a duplicated "node/add" item in the Navigation menu which can only be deleted by editing the database. See <a href="/node/550254#comment-4833566">comment #145 for details.</li>
</ul>
<h3>Proposed resolution</h3>
<ul>
<li>Some of the problems outlined in the original post (see below for details) were solved by the patch provided in <a href="/node/550254#comment-3560482">#105</a>, and committed to HEAD by webchick in (<a href="/node/550254#comment-3733096">#120</a>).</li>
<li>The module responsible for the automatic creation or deletion of menu items for content types needs to do a db query to check where the menu items of the existing content types are located. If duplicate entries exist, it should make an intelligent guess on which is the "right" one.</li>
<h3>Remaining tasks</h3>
(reviews needed, tests to be written or run, documentation to be written, etc.)
<h3>User interface changes</h3>
(new or changed features/functionality, modules added or removed, changes to URL paths, changes to user interface text)
<h3>API changes</h3>
(API changes/additions that would affect module, install profile, and theme developers, including examples of before/after code if appropriate)
<h3>Original report by sun</h3>
(Paste Sun's original post here)

Status:Needs review» Active

Setting back to active.

Status:Needs review» Active

subscribe

Subscribe.

sub

Subscribing. After upgrading a D6 site to D7, the node/add page shows "You have not created any content types yet" yet I can create content types by going to the pages directly and I can edit the content types without issue.

Issue tags:+needs backport to D7

Tagging for backport.

sub

Issue summary:View changes

added new version of summary

Status:Active» Needs work

I added comment #147 as the new summary for this. #144 and 145 have good info too.

Choice quote:

The module responsible for the automatic creation or deletion of menu items for content types needs to do a db query to check where the menu items of the existing content types are located. If duplicate entries exist, it should make an intelligent guess on which is the "right" one.

This issue: http://drupal.org/node/1009982, is also closely related. Whatever code is added to seek out the menu items for adding each content type, should also be used to generate the list on node/add...which also breaks when you start moving these menu items around.

I am facing this issue currently. Found some mentions on the forums as well:
http://drupal.org/node/1218606

Not sure if the issue is related, but I also had no Administrator role created during update. (raised this as a separate issue: #1457516: Upgrade from D6 to D7: No Administrator role created (missing))

Could we remove the ->condition('ml.menu_name', $item['menu_name']) from the query? That would fix the node/add problem which is what is killing people. I on't know how this function is used but it seems like that menu-name check could be relaxed.

Priority:Major» Critical

@#144:

However all the menu items still continue to work and it's easy enough to correct the problem manually (though quite tedious) by moving every item individually.
There isn't any actual data loss here...

I don't agree with this.
It might be possible to manually move normal menu links back.
But it is impossible to manually move menu callbacks back!

For instance, on a clean install,running:

SELECT *
FROM `menu_links`
WHERE `link_path` LIKE '%admin/content%'

Returns:
"management";"8";"1";"admin/content";"admin/content";"Content";"a:1:{s:10:"attributes";a:1:{s:5:"title";s:24:"Find and manage content.";}}";"system";"0";"0";"0";"0";"-10";"2";"0";"1";"8";"0";"0";"0";"0";"0";"0";"0";"0"
"management";"23";"8";"admin/content/node";"admin/content/node";"Content";"a:0:{}";"system";"-1";"0";"0";"0";"-10";"3";"0";"1";"8";"23";"0";"0";"0";"0";"0";"0";"0"

On a site where menu links were messed up because of this bug, then manually reparented:

"management";"8";"1";"admin/content";"admin/content";"Content";"a:1:{s:10:"attributes";a:1:{s:5:"title";s:32:"Administer content and comments.";}}";"system";"0";"0";"1";"0";"-10";"2";"0";"1";"8";"0";"0";"0";"0";"0";"0";"0";"0"
"management";"23";"1";"admin/content/node";"admin/content/node";"Content";"a:0:{}";"system";"-1";"0";"0";"0";"-10";"2";"0";"1";"23";"0";"0";"0";"0";"0";"0";"0";"0"

In this site, "admin/content" was properly manually reparented to mlid:1 (admin).
But the "admin/content/node" callback was then automatically reparented to mlid:1 too instead of mlid:8.
(Because of this, for instance, admin_menu will show two "Content" menu items.)
There is no way to fix this other when directly edit db. Data is broken: it is a critical bug.

Actually, the example in my previous post was applying to D7.
Here is an example that applies to D8:
Text formats admin/config/content/formats is a good case as this item only contains callbacks and local tasks.
1. Move the Text formats item to a menu different from Management.
=> At this point, everything is still OK.
2. Flush the menu cache or do anything that makes menu links being rebuilt.
=> Text formats has lost its children (you can see it by the list bullet turned into a circle instead of an arrow)
3. Try to reset the Text formats item or to move it back to its original place.
=> It doesn't fix the problem: its children are not properly re-parented back.
The only way to fix this is to manually delete all admin/config/content/formats links from the menu_links table and to flush the menu cache.

Status:Needs work» Needs review
StatusFileSize
new2.9 KB
PASSED: [[SimpleTest]]: [MySQL] 35,066 pass(es).
[ View ]

Fixing this bug requires to change things in several files.
First I think that moshe weitzman is right at #161. I don't understand why this condition is necessary.
Removing it seems to fix the bug but breadcrumb test fails, revealing a hack in user.module, that requires 5 lines of comments!

<?php
  $items
['user/%user'] = array(
   
'title' => 'My account',
   
'title callback' => 'user_page_title',
   
'title arguments' => array(1),
   
'page callback' => 'user_view_page',
   
'page arguments' => array(1),
   
'access callback' => 'user_view_access',
   
'access arguments' => array(1),
   
// By assigning a different menu name, this item (and all registered child
    // paths) are no longer considered as children of 'user'. When accessing the
    // user account pages, the preferred menu link that is used to build the
    // active trail (breadcrumb) will be found in this menu (unless there is
    // more specific link), so the link to 'user' will not be in the breadcrumb.
   
'menu_name' => 'navigation',
  );
?>

If we don't want 'user' to show up in the breadcrumb, setting its type as MENU_VISIBLE_IN_TREE instead of MENU_NORMAL_ITEM is the right thing to do.
Doing so, there is still 1 remaining fail: we need then to update the hardcoded (!) mlid 6 in menu.test by mlid 5.
<?php
$this
->assertBreadcrumb("admin/structure/menu/item/5/edit", $trail);
?>

Here is the patch.
Obviously, it is not fully complete as it lacks new tests but I want to see if it passes the tests and get some feedback, so marking it as Needs review for now.

Status:Needs review» Needs work

What a nice surprise - thanks @anrikun for working on this! :)

A couple of notes though:

  1. The fact that you're able to remove that menu_name condition and no tests are failing means that this part of the menu link re-parenting logic is not tested at all. Removing the condition means that links being automatically derived from module router paths no longer adhere to the menu_name defined in code. While I can see how that could be able to fix a range of issues, the removal can easily cause false-positive matches in the re-parenting logic. For example, if there is a second menu link to the same path in the {menu_links} table. Apparently, the re-parenting lookup query is limited to module = 'system' (which denotes auto-generated links from router paths), so a false-positive would be limited to that -- and since a router path can only exist once, and thus only be auto-generated once, it might be safe to assume that there actually is no chance for a false-positive. But nevertheless, I'd like to see a some more verbose analysis of all possible scenarios and conditions related to the menu_name limitation in the re-parenting process (before vs. after comparison?) to make sure we don't train-wreck Drupal sites by committing this patch.
  2. The test change affects the breadcrumb assertion on administrative menu management pages, but I don't see any changes to router item definitions for those paths in this patch, which means that we're changing a test assertion without corresponding functional change. I'd like to see a better explanation for why that change is required. As you pointed out already, we can change the test to not use a hard-coded mlid but query for it instead. In any case, however, we need to know and be sure why exactly that mlid is different with this patch, and that this change in behavior is actually correct.
  3. That 5 line comment in user_menu() originates from my patch in #907690: Breadcrumbs don't work for dynamic paths & local tasks #2. I'm pretty sure that I tested every possible combination of router item hacks in-depth. AFAICS, you're changing the type definition of /user instead of the menu_name of user/%user. This possibly has unintended consequences with regard to the breadcrumb and/or active trail on user/password, user/register, or user profile pages when accessing them as anonymous or authenticated user. The /user ("My account" or "%user's account") link is expected to appear in the breadcrumb and be in the active trail on those pages. Unfortunately, there are quite some variations/permutations to test here. To verify and double-check the current behavior, we might have to enhance the breadcrumbs test or add a separate testUserBreadcrumbs method to assert these using the generatePermutations() method. An example for that can be found in CommentInterfaceTest::testCommentLinks() and related/invoked test methods. (Maybe we have a simpler example in the meantime, dunno)
  4. +++ b/core/includes/menu.inc
    @@ -1833,7 +1833,7 @@ function menu_navigation_links($menu_name, $level = 0) {
    -      $l = $item['link']['localized_options'];
    +      $l = isset($item['link']['localized_options']) ? $item['link']['localized_options'] : array();

    This change looks bogus and, in any case, unrelated to this issue.

Status:Needs work» Needs review
StatusFileSize
new1.93 KB
PASSED: [[SimpleTest]]: [MySQL] 35,419 pass(es).
[ View ]

Here is a better patch. Let's see if it passes the test.

StatusFileSize
new650 bytes
FAILED: [[SimpleTest]]: [MySQL] 35,406 pass(es), 16 fail(s), and 0 exception(s).
[ View ]

Hm. Alter hook implementations are generally discouraged within Drupal core itself; they're primarily supposed to be used by contributed and custom modules to change the default behavior. Altering the effects of another alter hook isn't precisely easy. So a patch along the lines of #164 would rather be preferred.

But first of all, I'd like to know which tests are actually failing with the essential change only.

Status:Needs review» Needs work

The last submitted patch, drupal8.menu-link-parent.167.patch, failed testing.

@sun: thank you for your useful feedback.

Just wanted to answer to #165: I confirm that 2. and 4. were unnecessary changes. I had done them because tests failed on my local computer but this had to do with my config only.

And about my patch at #166: actually, it is not normal that it passes tests, as even if breadcrumb looks OK with the patch, 'My account' is still in the active trail and marked as active when it should not.
So a test about this is missing in current D8. But that is a different story I guess.

Sub

Status:Needs work» Needs review
StatusFileSize
new1.33 KB
PASSED: [[SimpleTest]]: [MySQL] 37,110 pass(es).
[ View ]
new650 bytes
FAILED: [[SimpleTest]]: [MySQL] 36,504 pass(es), 8 fail(s), and 0 exception(s).
[ View ]

Let's summarize this issue:
1. Removing the menu_name condition seems to be the solution to fix the current bug.
2. But doing so, we need to fix the problem with user breadcrumb, which is not directly related to this issue.
That's why I've opened a separate issue/patch: #1564388: "My account" link is never in the active trail

@sun:
After looking a bit at core, it appears that alter hooks are used everywhere, even in user.module itself. I'm convinced that implementing hook_menu_breadcrumb_alter() is the correct API-way to do what we want here, that is, altering breadcrumb :-)

About what I wrote at #169 ('My account' is still in the active trail and marked as active when it should not.), forget about it.
I have thought about this again, and what I described as a incorrect behaviour is actually what we should expect: even if not in the breadcrumb, the 'My account' menu link itself should be marked as 'in active trail' (See #1564388 about this). Implementing hook_menu_breadcrumb_alter() fixes this too.

I'm posting here both patches for testing.

Adding a tag because of note 1 at #165.

Adding accidentally removed tags back.

A quick fix for D7

It's likely to take time before this gets RTBC, committed and backported to D7.
So for people like me who can't wait, here is a quick fix for D7.
For safety, it should be used only on new sites being developed, not existing ones.

Thank you Henri!

I too wish this moved a bit faster. So many patches to maintain over so many installations :/

We're looking at this issue during core mentoring, and it would be helpful if someone familiar with the issue could provide some instructions as to what should be tested manually. Thanks!

@xjm: manual tests should be done concerning 1. at #165 to check that there is really no chance for a false-positive.

It looks like we're going to move forward with the alter hook proposal by @anrikun in #1564388: "My account" link is never in the active trail

StatusFileSize
new650 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch menu_links_not_properly_reparented-550254-182.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Now that #1564388: "My account" link is never in the active trail is fixed/committed, let's re-roll and re-test patch at #171.

Thanks! Unless I'm mistaken, we should be able to add a test for this change?

E.g., might be quite simple actually:

- First action in the test is to verify that a particular sub-tree of links appears where it should.

- Second, hi-jack the menu link tree records for the particular sub-tree, either through the menu UI (possibly resetting the 'customized' flag afterwards) or through a manual db_update(), changing the menu_name and related depth and other columns.

- Then execute a menu_router_rebuild() and verify that the hi-jacked links re-appear in their expected locations.

The test-only patch should then reveal that this patch actually fixes the re-parenting.

Assigned:Unassigned» corvus_ch

StatusFileSize
new2.01 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 550254-158.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I tried to write a test to reproduce the described behavior but failed. Meaning that in my test the link item is moved from one menu to another including the children.

The existing tests contains the following.

<?php
   
// Start over, forcefully delete the parent from the database, simulating a
    // database crash. Check that the children of parent are now top-level.
   
$links = $this->createLinkHierarchy($module);
   
// Don't do that at home.
   
db_delete('menu_links')
      ->
condition('mlid', $links['parent']['mlid'])
      ->
execute();
   
$expected_hierarchy = array(
     
'child-1-1' => 'child-1',
     
'child-1-2' => 'child-1',
     
'child-2' => FALSE,
    );
   
$this->assertMenuLinkParents($links, $expected_hierarchy);
?>

What is tested here looks exactly what we are looking for. So my expectation is that we need to look inside the menu module and not inside menu.inc.
I haven't checked it out but I expect that menus module does delete a menu link forcefully and create a new on when one moves a menu link. Can somebody confirm that theorie?

Assigned:corvus_ch» Unassigned

sorry, havent read through all of this so maybe missed something.. but am guessing one of the happy outcomes of this is to fix the mess that admin menu now has under Structure (Views, Custom Formatters, etc menu items not parented correctly).

If that's the case; then just reporting that the patch in #175 does not work.

patch applied clean to 7.16, did a menu cache clear. no change in admin menu.

Priority:Critical» Major

While this is a strange bug, I don't think it's critical and there's not really an explanation here of why it is. It puts your site in a funny site but it's recoverable. So moving back to major.

@catch: the explanation is here (#162). It's not fully recoverable unless you manually edit database.

#185: 550254-158.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Needs tests, +D7 upgrade path, +needs backport to D7

The last submitted patch, 550254-158.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.12 KB
PASSED: [[SimpleTest]]: [MySQL] 57,149 pass(es).
[ View ]

I rerolled the patch so it should apply

#192: 550254-192.patch queued for re-testing.

This reroll in 192 only contains the test patch from 185. Need to have someone post the patch in 192 as a test only patch, and combine it with 182 for the actual patch.

Test patch at #192 passes.
The opposite should be expected. Without #182, it should fail. And pass with #182.

Status:Needs review» Needs work

Per #195

Status:Needs work» Needs review

Does this mean the issue is now not reproducible in 8.x? Could someone upload a D7 patch and test it?

@chx
The patch for D7 is at #175
Unfortunately, it can't be tested as this issue still targets D8.

I am struggling with this issue in D6., impossible t get my numerous navigation menu items back at their proper place.

I migrated all menu items in a dump menu - actually clearing the navigation menu - and enabled admin menu : the mess is back.
Is there a patch in this thread that could help?

We should substantially redo links for 8 and make the parenting much more explicit, so I would postpone this or push it back to 7.

Issue summary:View changes

html fix