Posted by sun on August 15, 2009 at 10:53pm
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | menu system |
| Category: | bug report |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | D7 upgrade path, needs backport to D7 |
Issue Summary
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
plidof0(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.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| drupal6.menu-link-save-plid.patch | 667 bytes | Idle | FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal6.menu-link-save-plid.patch. | View details | Re-test |
Comments
#1
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.
#2
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.
#3
Yeah, good one.
#4
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()?
#5
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.
#6
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)...
#7
+++ 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.
#8
The last submitted patch failed testing.
#9
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
#10
Another try.
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).
#11
The last submitted patch failed testing.
#13
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.
#14
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.
#15
I can finally confirm that this patch fixes the re-parenting.
#16
@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.
#17
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.
#18
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.
#19
<?phpfunction _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...
#20
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'])) {#21
Subscribing.
#22
It would be nice to totally ditch "customized" and the reset functionality imho.
#23
this is possibly a dupe of #408482: Menu links do not follow parent when moving
#24
subscribing
#25
Subscribing
#26
.
#27
Subscribing.
#28
Subscribing
#29
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?
#30
Subscribing.
#31
Subscribing.
#32
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."
#33
@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.
#34
(Post deleted, I was in the wrong issue queue.)
#35
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.
#36
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).
#37
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.
#38
@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...#39
@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.
#40
@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.
#41
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:
$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.
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.
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.
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. :)
#42
The last submitted patch failed testing.
#43
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.
#44
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
#45
The last submitted patch failed testing.
#46
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.
#47
The last submitted patch failed testing.
#48
Re-rolled. Still missing tests.
#49
Subscribe
#50
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.
#51
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.
#52
@indytechcook: the hope is that we can get it committed into D7 and then look at getting it into D6.
#53
Subscribing
Big fan of seeing this fixed
#54
The last submitted patch failed testing.
#55
oh I know those failing DBLog tests. Has nothing to do with this patch. Re-uploading the latest.
#56
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.
#57
Subscribing
#58
Subscribing
#59
@pwolanin (#39)
I made a new issue for my #36 suggestion.
#653784: Separate out menu links from hook_menu
#60
@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.
#61
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.
#62
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
#63
Re-test of drupal.menu-link-save-plid-13.patch from comment #13 was requested by kurokikaze.
#64
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.
#65
The last submitted patch, drupal.menu_.64.patch, failed testing.
#66
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...
#67
subscribing...
#68
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...
#69
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...
#70
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.
#71
subscribing...
#72
sub
#73
The last submitted patch, drupal.menu_.69.patch, failed testing.
#74
#69: drupal.menu_.69.patch queued for re-testing.
#75
I'm really hoping this will solve #647064: Fatal error: Unsupported operand types when menu_links query returns no results in system_admin_menu_block() as well...
#76
Bump, looking for reviews :)
#69: drupal.menu_.69.patch queued for re-testing.
#77
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.
#78
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);
}
}
}
#79
#69: drupal.menu_.69.patch queued for re-testing.
#80
@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.
#81
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.
#82
#83
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...
#84
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).
#85
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.
#86
...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.
#87
#69: drupal.menu_.69.patch queued for re-testing.
#88
Combining this with the upgrade path test hunk from #932134: No upgrade path for MENU CALLBACK API change to see what happens.
#89
The last submitted patch, 550254-with-upgrade.patch, failed testing.
#90
+ // 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.
#91
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.
#92
#88: 550254-with-upgrade.patch queued for re-testing.
#93
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
#94
The last submitted patch, 550254-with-upgrade.patch, failed testing.
#95
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.
#96
And here is the test + a fix proposal.
#97
+++ 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.
#98
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.
#99
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.
#100
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.
#101
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?
#102
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.
#103
Again, no, we cannot do that. plid = 0 means top level. End of story. We cannot "auto-magically" change that.
#104
@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.
#105
Added more comments to the test via @chx.
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 inmenu_link_save()#106
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.
#107
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.
#108
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.
#109
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.
#110
All of you are missing the entire point.
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.
#111
Consider me out of this issue.
#112
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
#113
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.
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:
$items['top/level'] = array();
Should the automatically generated link for the path 'top/level' live at the top-level of the menu?
$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?
Still on the top-level?
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.
#114
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.
#115
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.
#116
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.
#117
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..
#118
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?
#119
#105: 550254-reparenting.patch queued for re-testing.
#120
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".
#121
#2: drupal.menu-link-save-plid.patch queued for re-testing.
#122
@vensires, why queued for re-testing one patch that will not work, since a is allready commited that changes and more things in #105 ????
#123
Misclick... Finger over mouse, heavy finger, click. never mind.
#124
Subscribing, and resetting back to active based on webchick's comments in #120.
#125
I still suffer from this in latest 7.x dev of core.
#126
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.
#127
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.
#128
Duplicate menu items particularly for All menu's.
StructureMenus
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.
#129
#130
I confirm #128 too.
#131
Is this part of this issue, or should I open a new one for it:
#132
@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.
#133
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.
#134
I can confirm #127. Subscribing.
#135
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
#136
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.
#137
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.
#138
drupal6.menu-link-save-plid.patch queued for re-testing.
#139
subscribe
#140
*subscribe*
#141
Same issue for me, subscribing.
#142
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:
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.
#143
Closed as a dup: #511804: Menu corruption occurs when disabling a module which has a customized menu item
#144
Recapping Sun's #142:
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).
#145
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:
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.
#146
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.
#147
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)
#148
Setting back to active.
#149
subscribe
#150
Subscribe.
#151
sub
#152
I wonder if #430304: Menu item loses parents when parent is a router item only and #408338: fix_menu_navigation_links_rebuild to correctly handle changing menu_name are related
#153
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.
#154
Tagging for backport.
#155
sub
#156
mbutelman: Stop subscribing, start following.
#157
#1079628: Menu link is not removed after changing the menu router item's type looks closely related
#158
I added comment #147 as the new summary for this. #144 and 145 have good info too.
Choice quote:
#159
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.