Problem/Motivation

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

Proposed resolution

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

Remaining tasks

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

User interface changes

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

API changes

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

---

Original report by sun

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

Symptom

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

Details

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

Cause

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

    to invoke the re-parenting process.

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

Solution

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

    to invoke the re-parenting check.

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

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

CommentFileSizeAuthor
#222 menu_links_not_properly_reparented_plus_user_active_trail-550254-222-D7-do-not-test.patch2 KBcolinstillwell
#202 550254-psr4-reroll.patch2.05 KBxjm
#192 550254-192.patch2.12 KBjrglasgow
#185 550254-158.patch2.01 KBcorvus_ch
#182 menu_links_not_properly_reparented-550254-182.patch650 bytesanrikun
#175 menu_links_not_properly_reparented_plus_user_active_trail-550254-175-D7-do-not-test.patch1.91 KBanrikun
#171 menu_links_not_properly_reparented-550254-171.patch650 bytesanrikun
#171 user_active_trail-1564388-1.patch1.33 KBanrikun
#167 drupal8.menu-link-parent.167.patch650 bytessun
#166 menu_links_not_properly_reparented-550254-166.patch1.93 KBanrikun
#164 menu_links_not_properly_reparented-550254-164.patch2.9 KBanrikun
#135 wrongmenu.png28.61 KBtinny
#105 550254-reparenting.patch12.34 KBDamien Tournoud
#100 550254-reparenting.patch12.04 KBDamien Tournoud
#96 550254-reparenting.patch11.92 KBDamien Tournoud
#95 550254-reparenting-test.patch7.38 KBDamien Tournoud
#88 550254-with-upgrade.patch3.74 KBcatch
#69 drupal.menu_.69.patch2.65 KBBerdir
#64 drupal.menu_.64.patch1.62 KBsun
#56 drupal-menu.56.patch3.58 KBsun
#55 drupal-menu.48.patch3.31 KBsun
#48 drupal-menu.48.patch3.31 KBsun
#46 drupal-menu.46.patch7.17 KBsun
#43 drupal.menu-reparenting.43.patch2.42 KBsun
#41 drupal.menu-reparenting.patch2.14 KBsun
#17 plid-not-0-550254-17.patch650 bytespwolanin
#13 drupal.menu-link-save-plid-13.patch2.43 KBsun
#10 drupal.menu-link-save-plid-10.patch2.29 KBsun
#6 drupal.menu-link-save-plid-6.patch2.41 KBsun
#5 drupal6.menu-link-save-plid-d6.patch1.76 KBsun
#2 drupal.menu-link-save-plid.patch876 bytessun
drupal6.menu-link-save-plid.patch667 bytessun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

Status: Needs review » Reviewed & tested by the community

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

sun’s picture

Version: 6.x-dev » 7.x-dev
FileSize
876 bytes

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.

chx’s picture

Yeah, good one.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

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

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

sun’s picture

Status: Needs work » Needs review
FileSize
1.76 KB

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.

sun’s picture

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

sun’s picture

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

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

Status: Needs review » Needs work

The last submitted patch failed testing.

pwolanin’s picture

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

sun’s picture

Status: Needs work » Needs review
FileSize
2.29 KB

Another try.

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

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

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Closed (works as designed) » Needs review
FileSize
2.43 KB

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.

sun’s picture

Priority: Normal » Critical

Tests pass. Code review needed.

To clarify further.

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

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

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

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

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

To summarize:

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

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

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

sun’s picture

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

pwolanin’s picture

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

pwolanin’s picture

FileSize
650 bytes

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.

sun’s picture

Priority: Critical » Normal

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

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

pwolanin’s picture

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

see code comment...

sun’s picture

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'])) {
Leeteq’s picture

Subscribing.

pwolanin’s picture

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

moshe weitzman’s picture

marcushenningsen’s picture

subscribing

translector’s picture

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

Subscribing

sun’s picture

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

.

TCRobbert’s picture

Subscribing.

Gary Feldman’s picture

Subscribing

mcrittenden’s picture

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?

adamo’s picture

Subscribing.

entr3p’s picture

Subscribing.

rsvelko’s picture

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

pwolanin’s picture

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

smk-ka’s picture

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

donquixote’s picture

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.

donquixote’s picture

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

donquixote’s picture

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.

sun’s picture

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

pwolanin’s picture

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

donquixote’s picture

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

sun’s picture

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

A test module should cover:

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
2.42 KB

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.

sun’s picture

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

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
7.17 KB

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

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

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
3.31 KB

Re-rolled. Still missing tests.

hanoii’s picture

Subscribe

joshk’s picture

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.

indytechcook’s picture

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.

mcrittenden’s picture

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

servantleader’s picture

Subscribing
Big fan of seeing this fixed

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
3.31 KB

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

sun’s picture

FileSize
3.58 KB

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.

Jolidog’s picture

Subscribing

AdrianB’s picture

Subscribing

donquixote’s picture

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

pwolanin’s picture

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

DarrellDuane’s picture

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.

pwolanin’s picture

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

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

sun’s picture

FileSize
1.62 KB

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

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

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

Status: Needs review » Needs work

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

supersteph’s picture

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

klonos’s picture

subscribing...

Berdir’s picture

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

    // 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

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

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.65 KB

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

sun’s picture

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.

klonos’s picture

subscribing...

jannalexx’s picture

sub

Status: Needs review » Needs work

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

choster’s picture

Status: Needs work » Needs review

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

GuyPaddock’s picture

Berdir’s picture

Bump, looking for reviews :)

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

Xano’s picture

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.

Daniel Norton’s picture

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);
    }
  }
}
Berdir’s picture

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

sun’s picture

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

sun’s picture

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

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

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

sun’s picture

Priority: Normal » Major
Berdir’s picture

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?

/**
 * 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...

sun’s picture

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

sun’s picture

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.

klonos’s picture

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

sun’s picture

Status: Needs work » Needs review

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

catch’s picture

FileSize
3.74 KB

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

Status: Needs review » Needs work

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

Damien Tournoud’s picture

+  // 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.

Damien Tournoud’s picture

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.

int’s picture

Status: Needs work » Needs review

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

int’s picture

Status: Needs work » Needs review

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

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

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

(admin/build/menu-customize/navigation)

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

(admin/structure/menu/manage/management)

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

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

I need a patch that apply's

Status: Needs review » Needs work

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

Damien Tournoud’s picture

Issue tags: -Needs tests
FileSize
7.38 KB

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.

Damien Tournoud’s picture

FileSize
11.92 KB

And here is the test + a fix proposal.

sun’s picture

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

sun’s picture

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.

Damien Tournoud’s picture

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

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

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

Damien Tournoud’s picture

FileSize
12.04 KB

So this sounds like a hook_menu_link_parent() to me.

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

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

Rerolled the patch to fix an incomplete comment.

int’s picture

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?

sun’s picture

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.

Damien Tournoud’s picture

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

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

sun’s picture

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

Damien Tournoud’s picture

FileSize
12.34 KB

Added more comments to the test via @chx.

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

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

chx’s picture

Status: Needs review » Reviewed & tested by the community

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

sun’s picture

Status: Reviewed & tested by the community » Needs review

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

chx’s picture

Status: Needs review » Reviewed & tested by the community

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

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

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

Damien Tournoud’s picture

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.

sun’s picture

Status: Reviewed & tested by the community » Needs work

All of you are missing the entire point.

You *can* move a menu link

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

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

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

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

Damien Tournoud’s picture

Consider me out of this issue.

int’s picture

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

sun’s picture

Status: Needs work » Reviewed & tested by the community

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

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

Menu router items can change over time.

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

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

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

--
An arbitrary example of a possible workflow:

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

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

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

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

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

    Still on the top-level?

  4. Revert the type changes.

    Still on the top-level?

--

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

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

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

Damien Tournoud’s picture

An arbitrary example of a possible workflow:

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

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

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

sun’s picture

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.

yoroy’s picture

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.

int’s picture

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

klonos’s picture

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?

webchick’s picture

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

webchick’s picture

Status: Reviewed & tested by the community » Active

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

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

vensires’s picture

Status: Active » Needs review

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

int’s picture

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

vensires’s picture

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

EvanDonovan’s picture

Status: Needs review » Active

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

klonos’s picture

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

marcvangend’s picture

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.

CYD’s picture

Version: 7.x-dev » 7.0

I think this is related.

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

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

DRIVE’s picture

Status: Active » Needs review

Duplicate menu items particularly for All menu's.

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

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

sun’s picture

klonos’s picture

Status: Needs review » Active

I confirm #128 too.

effulgentsia’s picture

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

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

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

donquixote’s picture

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

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

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

brewthis’s picture

I can confirm #127. Subscribing.

tinny’s picture

FileSize
28.61 KB

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

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

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

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

then for some reason, chrome gives me this

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

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

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

David D’s picture

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.

David D’s picture

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.

ngottlieb’s picture

Status: Active » Needs review

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

WiredEscape’s picture

subscribe

bryrock’s picture

*subscribe*

SilviaT’s picture

Same issue for me, subscribing.

sun’s picture

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

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

Steps to reproduce:

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

Actual result:

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

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

The expected result is obvious.

pillarsdotnet’s picture

quicksketch’s picture

Priority: Critical » Major

Recapping Sun's #142:

Steps to reproduce:

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

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

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

The expected result is obvious.

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

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

LarsKramer’s picture

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

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

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

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

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

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

klonos’s picture

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.

LarsKramer’s picture

Status: Active » Needs review

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

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

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

<h3>Proposed resolution</h3>
<ul>
<li>Some of the problems outlined in the original post (see below for details) were solved by the patch provided in <a href="/node/550254#comment-3560482">#105</a>, and committed to HEAD by webchick in (<a href="/node/550254#comment-3733096">#120</a>).</li>
<li>The module responsible for the automatic creation or deletion of menu items for content types needs to do a db query to check where the menu items of the existing content types are located. If duplicate entries exist, it should make an intelligent guess on which is the "right" one.</li>

<h3>Remaining tasks</h3>
(reviews needed, tests to be written or run, documentation to be written, etc.)

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

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

<h3>Original report by sun</h3>
(Paste Sun's original post here)
LarsKramer’s picture

Status: Needs review » Active

Setting back to active.

jviitamaki’s picture

Status: Needs review » Active

subscribe

JustMagicMaria’s picture

Subscribe.

lpalgarvio’s picture

sub

mrfelton’s picture

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.

catch’s picture

Issue tags: +Needs backport to D7

Tagging for backport.

mbutelman’s picture

sub

marcvangend’s picture

sun’s picture

yoroy’s picture

Issue summary: View changes

added new version of summary

yoroy’s picture

Status: Active » Needs work

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

Choice quote:

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

mkadin’s picture

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.

giorgio79’s picture

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

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

moshe weitzman’s picture

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

anrikun’s picture

Priority: Major » Critical

@#144:

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

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

For instance, on a clean install,running:

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

Returns:

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

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

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

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

anrikun’s picture

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

anrikun’s picture

Status: Needs work » Needs review
FileSize
2.9 KB

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

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

If we don't want 'user' to show up in the breadcrumb, setting its type as MENU_VISIBLE_IN_TREE instead of MENU_NORMAL_ITEM is the right thing to do.
Doing so, there is still 1 remaining fail: we need then to update the hardcoded (!) mlid 6 in menu.test by mlid 5.

<?php
$this->assertBreadcrumb("admin/structure/menu/item/5/edit", $trail);
?>

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

sun’s picture

Status: Needs review » Needs work

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

A couple of notes though:

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

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

anrikun’s picture

Status: Needs work » Needs review
FileSize
1.93 KB

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

sun’s picture

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

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

Status: Needs review » Needs work

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

anrikun’s picture

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

JonoB’s picture

Sub

anrikun’s picture

Status: Needs work » Needs review
FileSize
1.33 KB
650 bytes

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

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

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

I'm posting here both patches for testing.

anrikun’s picture

Adding a tag because of note 1 at #165.

anrikun’s picture

Adding accidentally removed tags back.

gpk’s picture

anrikun’s picture

A quick fix for D7

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

klonos’s picture

Thank you Henri!

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

webchick’s picture

fjs’s picture

xjm’s picture

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

anrikun’s picture

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

sun’s picture

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

anrikun’s picture

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

sun’s picture

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

E.g., might be quite simple actually:

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

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

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

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

corvus_ch’s picture

Assigned: Unassigned » corvus_ch
corvus_ch’s picture

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

The existing tests contains the following.

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

    $expected_hierarchy = array(
      'child-1-1' => 'child-1',
      'child-1-2' => 'child-1',
      'child-2' => FALSE,
    );
    $this->assertMenuLinkParents($links, $expected_hierarchy);

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

corvus_ch’s picture

Assigned: corvus_ch » Unassigned
liquidcms’s picture

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

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

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

catch’s picture

Priority: Critical » Major

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

anrikun’s picture

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

kerasai’s picture

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

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

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

jrglasgow’s picture

Status: Needs work » Needs review
FileSize
2.12 KB

I rerolled the patch so it should apply

zakiya’s picture

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

disasm’s picture

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

anrikun’s picture

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

JimmyAx’s picture

Status: Needs review » Needs work

Per #195

anrikun’s picture

Status: Needs work » Needs review
chx’s picture

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

anrikun’s picture

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

jvieille’s picture

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

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

pwolanin’s picture

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

pwolanin’s picture

Issue summary: View changes

html fix

xjm’s picture

FileSize
2.05 KB

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

Status: Needs review » Needs work

The last submitted patch, 202: 550254-psr4-reroll.patch, failed testing.

pwolanin’s picture

mgifford’s picture

dawehner’s picture

Issue tags: +Needs reroll

.

Berdir’s picture

Version: 8.0.x-dev » 7.x-dev
Issue tags: -Needs backport to D7

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

mgifford’s picture

So is the most useful patch to start with be in comment #164 by @anrikun?

Alex Malkov’s picture

Subscribing

The_Bucks’s picture

I 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;

  • Ran /sites/all/modules/registry_rebuild/registry_rebuild.php
  • Restored database backup
  • Re-ran /sites/all/modules/registry_rebuild/registry_rebuild.php {No Errors}
  • Disabled "Administrative overlay" from my user profile
  • Created a new link to node/add (Delete "Add content" was not there) named "Add My Content" with the path node/add.
  • In the Home » Administration » Structure » Menus » Navigation, I Reset The "Add content" link using the button.
  • Disabled the link to "Add content"
  • Flushed All Cache
  • Re-ran /sites/all/modules/registry_rebuild/registry_rebuild.php {No Errors}
  • Reset "Add content" while it was disabled. Drupal enabled the link automatically.
  • Tried the link with and without https://

Still no luck. At the end of the day I can still..

  1. "Add new coupon"
  2. "Create Subscription"
  3. "Create order"

,but not "Add Content" of any of the types listed below.

Content types

  • Book page (Machine name: book)
  • Feed (Machine name: feed)
  • Feed item (Machine name: feed_item)
  • Invoice (Machine name: invoice)
  • Panel (Machine name: panel)
  • Product (Machine name: product)
  • Product kit (Machine name: product_kit)
  • Recurring Subscription (Machine name: uc_recurring_subscription)
  • Simple Ad (Machine name: simpleads)
  • Simple Ad Campaign (Machine name: simpleads_campaign)
  • Webform (Machine name: webform)

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.

thomas.fleming’s picture

This 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:

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

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.

  • webchick committed 48abcd8 on 8.3.x
    #550254 by Damien Tournoud, sun: Fixed some causes of menu links are...

  • webchick committed 48abcd8 on 8.3.x
    #550254 by Damien Tournoud, sun: Fixed some causes of menu links are...
merlinregis’s picture

Hi 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

merlinregis’s picture

rootwork’s picture

@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 use patch -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!

merlinregis’s picture

Hello @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

merlinregis’s picture

  • webchick committed 48abcd8 on 8.4.x
    #550254 by Damien Tournoud, sun: Fixed some causes of menu links are...

  • webchick committed 48abcd8 on 8.4.x
    #550254 by Damien Tournoud, sun: Fixed some causes of menu links are...
hey-pingu’s picture

I`ve been having a similar issue as described here: https://www.drupal.org/project/admin_menu/issues/2950890

colinstillwell’s picture

I 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

hey-pingu’s picture

The 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:

PHP Fatal error:  Cannot redeclare user_menu_breadcrumb_alter() (previously decl                                                                                        ared in <path1>/user.module:4105) in <path1>/user.module on line 4105
Drush command terminated abnormally due to an unrecoverable error.   [error]
Error: Cannot redeclare user_menu_breadcrumb_alter() (previously
declared in <path1>/user.module:4105) in
<path1>/user.module, line 4105

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.

  • webchick committed 48abcd8 on 9.1.x
    #550254 by Damien Tournoud, sun: Fixed some causes of menu links are...
joseph.olstad’s picture

patch 222 still applies ok to v7.75 Drupal core (with fuzz)

joseph.olstad’s picture

patch 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