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
of0
(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.
Comment | File | Size | Author |
---|---|---|---|
#222 | menu_links_not_properly_reparented_plus_user_active_trail-550254-222-D7-do-not-test.patch | 2 KB | colinstillwell |
#202 | 550254-psr4-reroll.patch | 2.05 KB | xjm |
#192 | 550254-192.patch | 2.12 KB | jrglasgow |
#185 | 550254-158.patch | 2.01 KB | corvus_ch |
#182 | menu_links_not_properly_reparented-550254-182.patch | 650 bytes | anrikun |
Comments
Comment #1
chx CreditAttribution: chx commentedWell 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.
Comment #2
sunA 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.
Comment #3
chx CreditAttribution: chx commentedYeah, good one.
Comment #4
webchickThis 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()?
Comment #5
sunThere'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.
Comment #6
sunSame 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)...
Comment #7
sunNote that this comment is a straight 1:1 copy from D6. Keep it comparable, as long as no one understands it.
Comment #9
pwolanin CreditAttribution: pwolanin commentedI 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
Comment #10
sunAnother 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).
Comment #13
sunLast 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.
Comment #14
sunTests 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.
Comment #15
sunI can finally confirm that this patch fixes the re-parenting.
Comment #16
pwolanin CreditAttribution: pwolanin commented@sun - I have had perfectly fine experience using admin_menu 6.x-1.x, so unless there's a recent regression I don't understand why it causes such angst.
The patch to move menu_rebuild off the modules page would likely help it.
I think the reason plid ends up zero is that we are setting it that way. See menu.inc line 1961 in function _menu_navigation_links_rebuild($menu)
I think it is this code that needs to be altered or fixed - this assigning plid to 0 was the least effort way to having functional code when we previously worked on fixing reparenting issues, but probably should have left a todo there.
Comment #17
pwolanin CreditAttribution: pwolanin commentednote, 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.
Comment #18
sunThat may be related, but that portion of the code only deals with (existing) items that have a menu_name mismatch:
I don't think the menu_name plays a role. It's rather the unwanted re-parenting of children when a parent (mistakenly or not) is or becomes a MENU_CALLBACK.
I'll try to come up with a test module.
Comment #19
pwolanin CreditAttribution: pwolanin commentedsee code comment...
Comment #20
sunBut you may be right to some extent. Though that may be another issue.
Currently, the re-parenting fails for the aforementioned (MENU_CALLBACK having children) condition. That I need to prove with a test module, but I was able to repeatedly replicate the behavior - for items that definitely had the same menu_name already stored in {menu_links}.
However, it's perfectly possible that this condition in _menu_navigation_links_rebuild() is the cause for the bug users reported after upgrading from 1.x to 3.x - because the menu_name changes from 'navigation' to 'admin_menu' there. That means, the condition translates to FALSE, because the 'menu_name' is not empty and it's different to the existing.
But the item did not really lose its parent. Rather, similar to the other code in my patch, we're relying on potentially stale data in {menu_links} in the database from a previous rebuild - as far as menu router items are concerned. hook_menu() passes a perfectly matching 'menu_name' for the "existing" router item, and only the old data in {menu_links} no longer applies.
Hence, a potential amendment could be to only test the menu_name if the item wasn't customized.
Comment #21
Leeteq CreditAttribution: Leeteq commentedSubscribing.
Comment #22
pwolanin CreditAttribution: pwolanin commentedIt would be nice to totally ditch "customized" and the reset functionality imho.
Comment #23
moshe weitzman CreditAttribution: moshe weitzman commentedthis is possibly a dupe of #408482: Menu links do not follow parent when moving
Comment #24
marcushenningsen CreditAttribution: marcushenningsen commentedsubscribing
Comment #25
translectorSubscribing
Comment #26
sun.
Comment #27
TCRobbert CreditAttribution: TCRobbert commentedSubscribing.
Comment #28
Gary Feldman CreditAttribution: Gary Feldman commentedSubscribing
Comment #29
mcrittenden CreditAttribution: mcrittenden commentedSubscribe. 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?
Comment #30
adamo CreditAttribution: adamo commentedSubscribing.
Comment #31
entr3p CreditAttribution: entr3p commentedSubscribing.
Comment #32
rsvelko CreditAttribution: rsvelko commentedPlease, 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."
Comment #33
pwolanin CreditAttribution: pwolanin commented@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.
Comment #34
smk-ka CreditAttribution: smk-ka commented(Post deleted, I was in the wrong issue queue.)
Comment #35
donquixote CreditAttribution: donquixote commentedWhy 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.
Comment #36
donquixote CreditAttribution: donquixote commentedTo clarify:
By "listener" I mean introducing a new hook. Such as
menu_navigation_links_rebuild could then be moved into a new (core) module - such as "system_navigation" (we want to avoid nameclashes with contrib) - as an implementation of the above hook (that would be system_navigation_menu_router_update()).
admin_menu would have its own implementation, that can be more light-weight because it does not need to take into account manually added menu links.
The very cool thing is that you can turn this module off, and thus gain a lot of speed, if you don't need the system's built-in navigation menu (which is very likely if you use admin menu).
Comment #37
donquixote CreditAttribution: donquixote commentedThe 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.
Comment #38
sun@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...Comment #39
pwolanin CreditAttribution: pwolanin commented@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.
Comment #40
donquixote CreditAttribution: donquixote commented@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.
Comment #41
sunHad 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:
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. :)
Comment #43
sunoopsie - 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.
Comment #44
sunPlease additionally note that menu tree output is currently broken in D7 - to be fixed by #566094: menu_tree_data() doesn't build proper hierarchy
Comment #46
sunThis 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.
Comment #48
sunRe-rolled. Still missing tests.
Comment #49
hanoiiSubscribe
Comment #50
joshk CreditAttribution: joshk commentedSubscribing. 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.
Comment #51
indytechcook CreditAttribution: indytechcook commentedsun, 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.
Comment #52
mcrittenden CreditAttribution: mcrittenden commented@indytechcook: the hope is that we can get it committed into D7 and then look at getting it into D6.
Comment #53
servantleader CreditAttribution: servantleader commentedSubscribing
Big fan of seeing this fixed
Comment #55
sunoh I know those failing DBLog tests. Has nothing to do with this patch. Re-uploading the latest.
Comment #56
sunAdditionally, 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.
Comment #57
Jolidog CreditAttribution: Jolidog commentedSubscribing
Comment #58
AdrianB CreditAttribution: AdrianB commentedSubscribing
Comment #59
donquixote CreditAttribution: donquixote commented@pwolanin (#39)
I made a new issue for my #36 suggestion.
#653784: Separate out menu links from hook_menu
Comment #60
pwolanin CreditAttribution: pwolanin commented@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.
Comment #61
DarrellDuane CreditAttribution: DarrellDuane commentedI 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.
Comment #62
pwolanin CreditAttribution: pwolanin commentedpatch 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
Comment #64
sunThis 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.
Comment #66
supersteph CreditAttribution: supersteph commentedthank 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...
Comment #67
klonossubscribing...
Comment #68
BerdirIf 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
And then we check if the link is displayed. However, we actually have set navigation:0 as the default parent a few lines above
The Navigation menu is however not enabled by default. We pass then the following to menu_link_save():
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...
Comment #69
BerdirOk, 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...
Comment #70
sunThis 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.
Comment #71
klonossubscribing...
Comment #72
jannalexx CreditAttribution: jannalexx commentedsub
Comment #74
choster CreditAttribution: choster commented#69: drupal.menu_.69.patch queued for re-testing.
Comment #75
GuyPaddock CreditAttribution: GuyPaddock commentedI'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...
Comment #76
BerdirBump, looking for reviews :)
#69: drupal.menu_.69.patch queued for re-testing.
Comment #77
XanoTo 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.
Comment #78
Daniel Norton CreditAttribution: Daniel Norton commentedIs this the same thing that this code reports? It’s saying that I have 88 orphaned links (including all the links in orphaned branches).
Comment #79
Berdir#69: drupal.menu_.69.patch queued for re-testing.
Comment #80
sun@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.
Comment #81
sunI'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.
Comment #82
sunComment #83
BerdirI 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?
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...
Comment #84
sunmmm... 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).
Comment #85
sunI 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.
Comment #86
klonos...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.
Comment #87
sun#69: drupal.menu_.69.patch queued for re-testing.
Comment #88
catchCombining this with the upgrade path test hunk from #932134: No upgrade path for MENU CALLBACK API change to see what happens.
Comment #90
Damien Tournoud CreditAttribution: Damien Tournoud commentedI 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.
Comment #91
Damien Tournoud CreditAttribution: Damien Tournoud commentedI 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.
Comment #92
int CreditAttribution: int commented#88: 550254-with-upgrade.patch queued for re-testing.
Comment #93
int CreditAttribution: int commentedI 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
Comment #95
Damien Tournoud CreditAttribution: Damien Tournoud commentedBack to the basics. Here is a (kind of) comprehensive test for automatic menu reparenting.
This will fail. Let's discuss how to fix it.
Comment #96
Damien Tournoud CreditAttribution: Damien Tournoud commentedAnd here is the test + a fix proposal.
Comment #97
sunHey, 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.
Comment #98
sunBut 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.
Comment #99
Damien Tournoud CreditAttribution: Damien Tournoud commentedIf 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.
Comment #100
Damien Tournoud CreditAttribution: Damien Tournoud commentedThat 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.
Comment #101
int CreditAttribution: int commentedI 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?
Comment #102
sunI 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.
Comment #103
Damien Tournoud CreditAttribution: Damien Tournoud commentedAgain, no, we cannot do that. plid = 0 means top level. End of story. We cannot "auto-magically" change that.
Comment #104
sun@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.
Comment #105
Damien Tournoud CreditAttribution: Damien Tournoud commentedAdded 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()
Comment #106
chx CreditAttribution: chx commentedThat'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.
Comment #107
sunI 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.
Comment #108
chx CreditAttribution: chx commentedsun, 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.
Comment #109
Damien Tournoud CreditAttribution: Damien Tournoud commentedAlso, 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.
Comment #110
sunAll 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.
Comment #111
Damien Tournoud CreditAttribution: Damien Tournoud commentedConsider me out of this issue.
Comment #112
int CreditAttribution: int commentedI 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
Comment #113
sunSorry, 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:
Should the automatically generated link for the path 'top/level' live at the top-level of the menu?
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.
Comment #114
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis 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.
Comment #115
sunWell, 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.
Comment #116
yoroy CreditAttribution: yoroy commentedComing 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.
Comment #117
int CreditAttribution: int commentedI 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..
Comment #118
klonosI 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:
In the second setup (the one with the issue) I see:
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?
Comment #119
webchick#105: 550254-reparenting.patch queued for re-testing.
Comment #120
webchickWell, 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".
Comment #121
vensires CreditAttribution: vensires commented#2: drupal.menu-link-save-plid.patch queued for re-testing.
Comment #122
int CreditAttribution: int commented@vensires, why queued for re-testing one patch that will not work, since a is allready commited that changes and more things in #105 ????
Comment #123
vensires CreditAttribution: vensires commentedMisclick... Finger over mouse, heavy finger, click. never mind.
Comment #124
EvanDonovan CreditAttribution: EvanDonovan commentedSubscribing, and resetting back to active based on webchick's comments in #120.
Comment #125
klonosI still suffer from this in latest 7.x dev of core.
Comment #126
marcvangendAm 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.
Comment #127
CYD CreditAttribution: CYD commentedI 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.
Comment #128
DRIVE CreditAttribution: DRIVE commentedDuplicate menu items particularly for All menu's.
Flushing menu caches does nothing. Installed on vanilla D7.0 with only native modules.
Comment #129
sunComment #130
klonosI confirm #128 too.
Comment #131
effulgentsia CreditAttribution: effulgentsia commentedIs this part of this issue, or should I open a new one for it:
Comment #132
sun@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.
Comment #133
donquixote CreditAttribution: donquixote commentedYeah, 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.
Comment #134
brewthis CreditAttribution: brewthis commentedI can confirm #127. Subscribing.
Comment #135
tinny CreditAttribution: tinny commentedapparantly 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
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
Comment #136
David D CreditAttribution: David D commentedToday 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.
Comment #137
David D CreditAttribution: David D commentedI 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.
Comment #138
ngottlieb CreditAttribution: ngottlieb commenteddrupal6.menu-link-save-plid.patch queued for re-testing.
Comment #139
WiredEscape CreditAttribution: WiredEscape commentedsubscribe
Comment #140
bryrock CreditAttribution: bryrock commented*subscribe*
Comment #141
SilviaT CreditAttribution: SilviaT commentedSame issue for me, subscribing.
Comment #142
sunActually, @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:
The expected result is obvious.
Comment #143
pillarsdotnet CreditAttribution: pillarsdotnet commentedClosed as a dup: #511804: Menu corruption occurs when disabling a module which has a customized menu item
Comment #144
quicksketchRecapping 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).
Comment #145
LarsKramer CreditAttribution: LarsKramer commentedThis 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: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.
Comment #146
klonosThanx 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.
Comment #147
LarsKramer CreditAttribution: LarsKramer commentedHi 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:
Comment #148
LarsKramer CreditAttribution: LarsKramer commentedSetting back to active.
Comment #149
jviitamaki CreditAttribution: jviitamaki commentedsubscribe
Comment #150
JustMagicMaria CreditAttribution: JustMagicMaria commentedSubscribe.
Comment #151
lpalgarvio CreditAttribution: lpalgarvio commentedsub
Comment #152
yoroy CreditAttribution: yoroy commentedI 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
Comment #153
mrfelton CreditAttribution: mrfelton commentedSubscribing. 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.
Comment #154
catchTagging for backport.
Comment #155
mbutelman CreditAttribution: mbutelman commentedsub
Comment #156
marcvangendmbutelman: Stop subscribing, start following.
Comment #157
sun#1079628: Programatically added Menu link is not removed after removing the code. looks closely related
Comment #157.0
yoroy CreditAttribution: yoroy commentedadded new version of summary
Comment #158
yoroy CreditAttribution: yoroy commentedI added comment #147 as the new summary for this. #144 and 145 have good info too.
Choice quote:
Comment #159
mkadin CreditAttribution: mkadin commentedThis 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.
Comment #160
giorgio79 CreditAttribution: giorgio79 commentedI 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))
Comment #161
moshe weitzman CreditAttribution: moshe weitzman commentedCould 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.Comment #162
anrikun CreditAttribution: anrikun commented@#144:
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:
Returns:
On a site where menu links were messed up because of this bug, then manually reparented:
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.
Comment #163
anrikun CreditAttribution: anrikun commentedActually, 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 themenu_links
table and to flush the menu cache.Comment #164
anrikun CreditAttribution: anrikun commentedFixing 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!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.
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.
Comment #165
sunWhat a nice surprise - thanks @anrikun for working on this! :)
A couple of notes though:
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.This change looks bogus and, in any case, unrelated to this issue.
Comment #166
anrikun CreditAttribution: anrikun commentedHere is a better patch. Let's see if it passes the test.
Comment #167
sunHm. 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.
Comment #169
anrikun CreditAttribution: anrikun commented@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.
Comment #170
JonoB CreditAttribution: JonoB commentedSub
Comment #171
anrikun CreditAttribution: anrikun commentedLet'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 implementinghook_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.
Comment #172
anrikun CreditAttribution: anrikun commentedAdding a tag because of note 1 at #165.
Comment #173
anrikun CreditAttribution: anrikun commentedAdding accidentally removed tags back.
Comment #174
gpk CreditAttribution: gpk commentedRelated: #1009982: node/add page misses content types when menu links are moved or disabled
Comment #175
anrikun CreditAttribution: anrikun commentedA 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.
Comment #176
klonosThank you Henri!
I too wish this moved a bit faster. So many patches to maintain over so many installations :/
Comment #177
webchick#171: user_active_trail-1564388-1.patch queued for re-testing.
Comment #178
fjs CreditAttribution: fjs commented#171: user_active_trail-1564388-1.patch queued for re-testing.
Comment #179
xjmWe'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!
Comment #180
anrikun CreditAttribution: anrikun commented@xjm: manual tests should be done concerning 1. at #165 to check that there is really no chance for a false-positive.
Comment #181
sunIt 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
Comment #182
anrikun CreditAttribution: anrikun commentedNow that #1564388: "My account" link is never in the active trail is fixed/committed, let's re-roll and re-test patch at #171.
Comment #183
sunThanks! 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.
Comment #184
corvus_ch CreditAttribution: corvus_ch commentedComment #185
corvus_ch CreditAttribution: corvus_ch commentedI 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.
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?
Comment #186
corvus_ch CreditAttribution: corvus_ch commentedComment #187
liquidcms CreditAttribution: liquidcms commentedsorry, 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.
Comment #188
catchWhile 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.
Comment #189
anrikun CreditAttribution: anrikun commented@catch: the explanation is here (#162). It's not fully recoverable unless you manually edit database.
Comment #190
kerasai CreditAttribution: kerasai commented#185: 550254-158.patch queued for re-testing.
Comment #192
jrglasgow CreditAttribution: jrglasgow commentedI rerolled the patch so it should apply
Comment #193
zakiya CreditAttribution: zakiya commented#192: 550254-192.patch queued for re-testing.
Comment #194
disasm CreditAttribution: disasm commentedThis 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.
Comment #195
anrikun CreditAttribution: anrikun commentedTest patch at #192 passes.
The opposite should be expected. Without #182, it should fail. And pass with #182.
Comment #196
JimmyAx CreditAttribution: JimmyAx commentedPer #195
Comment #197
anrikun CreditAttribution: anrikun commented#182: menu_links_not_properly_reparented-550254-182.patch queued for re-testing.
Comment #198
chx CreditAttribution: chx commentedDoes this mean the issue is now not reproducible in 8.x? Could someone upload a D7 patch and test it?
Comment #199
anrikun CreditAttribution: anrikun commented@chx
The patch for D7 is at #175
Unfortunately, it can't be tested as this issue still targets D8.
Comment #200
jvieille CreditAttribution: jvieille commentedI 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?
Comment #201
pwolanin CreditAttribution: pwolanin commentedWe should substantially redo links for 8 and make the parenting much more explicit, so I would postpone this or push it back to 7.
Comment #201.0
pwolanin CreditAttribution: pwolanin commentedhtml fix
Comment #202
xjmThis patch was rerolled to apply to HEAD and for #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4. The reroll was automated, so other changes in the codebase might still require updates for this patch. :)
Comment #204
pwolanin CreditAttribution: pwolanin commentedCan we postpone this issue until: #2256521: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module
Comment #205
mgiffordComment #206
dawehner.
Comment #207
BerdirI can't imagine that this is still an issue in the same way as 7.x was, the way menu links, parents and trees work is completely different and much more explicit in 8.x Moving back to 7.x as suggested already by @pwolanin in #201.
Comment #208
mgiffordSo is the most useful patch to start with be in comment #164 by @anrikun?
Comment #209
Alex MalkovSubscribing
Comment #210
The_Bucks CreditAttribution: The_Bucks commentedI just encounter this error today. "You have not created any content types yet. Go to the content type creation page to add a new content type."I checked every node table and all of the content types are still there when viewing from admin/structure/types and PhpMyAdmin. I can edit them, adjust settings in the "manage fields and Display", from admin/structure/types but I cannot add new content from the node/add link.
Here's what I've done so far;
Still no luck. At the end of the day I can still..
,but not "Add Content" of any of the types listed below.
Content types
Here are some specs that you might need.
Drupal 7.42
Install profile Minimal (minimal-7.42)
Any suggestion?
Crap. Last Post Finally worked.
All of your help is greatly appreciated.
Comment #211
thomas.fleming CreditAttribution: thomas.fleming at Last Call Media commentedThis issue has a long history. After reading the entire thread, I think this issue as it stands now in Drupal 7 can best be summarized by the "quick fix" patch provided by @anrikun in #175 and the summary provided in #171:
The patch provided in #175 still seems to work, but it seems to me that #1564388: "My account" link is never in the active trail is still a blocker for this issue, even though it is tangentially related. So, I am adding #1564388 as a parent issue.
Comment #214
merlinregis CreditAttribution: merlinregis commentedHi all,
It's VERY nice to see all of the "possible" fixes, but I wonder : "How does a newbie apply these fixes?"
1st guess, I right click the patch link to download it, keeping it from showing in browser. At least I think I know that right.
But then
Questions :
- 1 - Where do I upload it?
- 2 - How do I apply it?
- 3 - If the 1st patch doesn't work :
- 3.1 - Is it gonna worsten the situation?
- 3.2 - Do I need to find a way to undo the patch? Will a backup restoration work?
Thanks in advance for the help
Comment #215
merlinregis CreditAttribution: merlinregis commentedComment #216
rootwork@merlinregis Welcome to the Drupal issue queue!
The short answer to your question is: Download the patch to the directory where it should be applied, and then use your terminal to run:
patch -p1 < name_of_file.patch
If you're not familiar with the terminal on your system, you could apply the patch manually -- open the patch file and you'll see it's just a text file listing lines to remove and lines to add. But I'd recommend reading the Drupal handbook on applying patches because once you learn it, it'll be a cinch to apply patches. To summarize from that page: If you're using git, you use
git apply
, in all other situations you usepatch -p1
. If that page seems too confusing, here's an even more step-by-step walk-through.It's also a good idea to keep a record of what you've patched, so you know when you update in the future to re-patch (or, if the issue has been fixed in the update, that you no longer need to patch). The simplest way is by keeping a PATCHES.txt file. A more advanced option is to use a tool like Drush make to maintain your codebase.
General support questions like this are good to ask in two places: The Drupal Slack channels, and the Drupal IRC channels. IRC usually has more people on it, but Slack may be more familiar to you. Asking in those places will get you a quicker answer, and will help keep issues in the issue queue focused on the problem at hand. Good luck!
Comment #217
merlinregis CreditAttribution: merlinregis commentedHello @rootwork,
Thanks for the reply. Since my problem is a generalized one : being unable to create any content because it says that the types are NOT found, How do I know where to put the patch?
In my menus pages, everything that "could" be resetted has been, but I STILL have the problem.
"Vous n'avez aucun type de contenu pour l'instant. Pour ajouter un nouveau type de contenu, allez à la page de création d'un type de contenu." Which basically means you can't create any content if you don't have any types of content. Create some types of content.
FIVE minutes just before that da*** message started appearing, I created a basic page... nothing fancy, PLAIN TEXT. Then BANG! He*** came thumblin' over! :'(
This site is in production, my partner is regularly asking me to bring modifications. Until now, I'm lucky the modif's are NOT to create new content, but I want to fix this before DOES asks for anything requiring to create new content.
Thanks in advance for ANY help.
In the meanwhile, I'll be reading the 2 links provided to see what I can understand out of all this :-S
Thanks again,
merlinregis
Comment #218
merlinregis CreditAttribution: merlinregis commentedComment #221
hey-pingu CreditAttribution: hey-pingu commentedI`ve been having a similar issue as described here: https://www.drupal.org/project/admin_menu/issues/2950890
Comment #222
colinstillwell CreditAttribution: colinstillwell commentedI have recreated the patch for 7.x, with isset checks to make it more bulletproof.
In some circumstances, user_menu_breadcrumb_alter will throw notices, as it tries to look for the "module" key under $active_trail.
E.g. "Notice: Undefined index: module in user_menu_breadcrumb_alter()"
We came across this issue when using the admin_menu module.
See https://www.drupal.org/project/drupal/issues/3002696
Comment #223
hey-pingu CreditAttribution: hey-pingu commentedThe patch didn`t work for me. When I tried it with the function, "user_menu_breadcrumb_alter", I got the following error message after clearing the cache:
When I tried clearing the cache after commenting out that function, I never got an error message, but the menu items did not have the proper parents.
Comment #225
joseph.olstadpatch 222 still applies ok to v7.75 Drupal core (with fuzz)
Comment #226
joseph.olstadpatch 222 still applies ok to v7.94 Drupal core (with fuzz)
#223 appears to be a PEBKAC issue (Problem Exists Between Keyboard And Chair), reproduce PEBKAC issue by applying the patch twice to the same stack.
Patch 222 working here, needs tests