If I edit a menu item that has children, and select a new parent, the children move to the old parent of the menu item that was moved.

An illustration!
In the following example, if I 'edit' 'Submenu' and change it's 'Parent item' to 'Menu 2', then all of its menu items move to be under 'Menu 1', so that this:

Menu 1
  |
  +--Submenu
       |
       +--Menu item 1
       |
       +--Menu item 2
       |
       +--Menu item 3

Menu 2

becomes this:

Menu 1
  |
  +--Menu item 1
  |
  +--Menu item 2
  |
  +--Menu item 3

Menu 2
  |
  +--Submenu

The way it currently stands, the only way to move an entire tree is to drag and drop it, but that doesn't work when moving a tree from one menu to another. If I want to move the entire tree to a new menu, I have to edit each and every item and manually set them into their old positions in the new menu.

Files: 
CommentFileSizeAuthor
#31 mysql_is_braindead_die_die_die.patch9.64 KBchx
PASSED: [[SimpleTest]]: [MySQL] 23,237 pass(es).
[ View ]
#26 408482-25.patch10.72 KBdixon_
PASSED: [[SimpleTest]]: [MySQL] 23,159 pass(es).
[ View ]
#25 408482-24.patch10.67 KBdrifter
PASSED: [[SimpleTest]]: [MySQL] 23,135 pass(es).
[ View ]
#21 408482-21.patch11.07 KBdrifter
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#16 menu_link_move_children-7.patch12.68 KBc960657
PASSED: [[SimpleTest]]: [MySQL] 18,113 pass(es).
[ View ]
#14 menu_link_move_children-7.patch16.74 KBc960657
FAILED: [[SimpleTest]]: [MySQL] 18,108 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#13 menu_link_move_children-6.patch12.12 KBc960657
PASSED: [[SimpleTest]]: [MySQL] 17,583 pass(es).
[ View ]
#12 menu_link_move_children-5.patch12.12 KBc960657
FAILED: [[SimpleTest]]: [MySQL] 17,582 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#11 menu_link_move_children-4.patch12.11 KBc960657
Passed: 14948 passes, 0 fails, 0 exceptions
[ View ]
#7 menu_link_move_children-3.patch12.12 KBc960657
Failed: 14878 passes, 9 fails, 336 exceptions
[ View ]
#5 menu_link_move_children-2.patch12.08 KBc960657
Failed: 13882 passes, 3 fails, 0 exceptions
[ View ]
#1 menu_link_move_children-1.patch12.09 KBc960657
Failed: Failed to apply patch.
[ View ]

Comments

c960657’s picture

Status:Active» Needs work
StatusFileSize
new12.09 KB
Failed: Failed to apply patch.
[ View ]

_menu_link_move_children() tried to move the child elements in an efficient but very complicated way, but failed. This patch makes the function much simpler. Considering that menu items aren't moved that often, I think this is a good trade-off.

c960657’s picture

Status:Needs work» Needs review
sun’s picture

oadaeh’s picture

@c960657: The patch did not work for me against the 7.x-dev released on September 21, 2009 at 05:10.

c960657’s picture

StatusFileSize
new12.08 KB
Failed: 13882 passes, 3 fails, 0 exceptions
[ View ]

@oadaeh: Hmm, it works for me. What happens instead?

This is just a reroll.

Status:Needs review» Needs work

The last submitted patch failed testing.

c960657’s picture

Status:Needs work» Needs review
StatusFileSize
new12.12 KB
Failed: 14878 passes, 9 fails, 336 exceptions
[ View ]

Reroll.

pflame’s picture

@c960657 The patch worked for me, I used drupal7 from cvs.

mattyoung’s picture

.

Status:Needs review» Needs work

The last submitted patch failed testing.

c960657’s picture

Status:Needs work» Needs review
StatusFileSize
new12.11 KB
Passed: 14948 passes, 0 fails, 0 exceptions
[ View ]

Reroll.

c960657’s picture

StatusFileSize
new12.12 KB
FAILED: [[SimpleTest]]: [MySQL] 17,582 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

Reroll.

c960657’s picture

StatusFileSize
new12.12 KB
PASSED: [[SimpleTest]]: [MySQL] 17,583 pass(es).
[ View ]
c960657’s picture

StatusFileSize
new16.74 KB
FAILED: [[SimpleTest]]: [MySQL] 18,108 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

Status:Needs review» Needs work

The last submitted patch, menu_link_move_children-7.patch, failed testing.

c960657’s picture

StatusFileSize
new12.68 KB
PASSED: [[SimpleTest]]: [MySQL] 18,113 pass(es).
[ View ]

Sorry, the last patch contained some unrelated changes.

c960657’s picture

Status:Needs work» Needs review
oadaeh’s picture

Status:Needs review» Needs work

I just tried the patch from comment #16 on the latest 7.0-dev version, and when I moved a menu item with sub-menu items to another menu, the sub-items did not follow it. I tried it with two different menu items to two different menus, just to be sure.

It was a brand-new, standard install (as opposed to minimal) with nothing done after the install other than apply this patch.

The patch did apply cleanly with no errors, and I can see that it was integrated with the code base correctly.

sun’s picture

+++ includes/menu.inc 21 Feb 2010 14:25:47 -0000
@@ -2924,53 +2924,42 @@ function menu_link_children_relative_dep
+    $query->expression('depth', $child_item['depth']);
...
+      $query->expression('p' . $i, $child_item['p' . $i]);

Can we build a "proper" $fields update array and assign that with $query->fields($fields) ?

137 critical left. Go review some!

sun’s picture

Title:Menu items do not follow parent when moving to new menu . . .» Menu links do not follow parent when moving
Priority:Normal» Major

#886620: Child menu links are lost in nirvana after moving parent link has been marked duplicate. This issue/patch blocks the critical issue #576290: Breadcrumbs don't work for dynamic paths & local tasks

Also, #886620 identified that this is not limited to moving links to a new menu. Links end up in nirvana when moving within the same menu, too. At the very least major, if not critical.

drifter’s picture

Status:Needs work» Needs review
StatusFileSize
new11.07 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Menu names were not being updated in patch #16, fixed. Also, rewritten the update expression syntax to fields as per #20.

chx’s picture

Status:Needs review» Needs work
-    _menu_link_parents_set($item, $parent);
+    _menu_link_ ($item, $parent);

that's not going to fly.

chx’s picture

Assigned:Unassigned» chx
Priority:Major» Critical

http://drupal.org/node/576290 depends on this so critical.

chx’s picture

Anyone following home: the original function while might be complicated did one database update instead of recursing in PHP. The Drupal 6 menu system was designed for hundreds of thousands (or even more) menu links and PHP recursion won't cut it. I am fixing the oriignal.

drifter’s picture

StatusFileSize
new10.67 KB
PASSED: [[SimpleTest]]: [MySQL] 23,135 pass(es).
[ View ]

Ehm, typo, thanks chx!

dixon_’s picture

StatusFileSize
new10.72 KB
PASSED: [[SimpleTest]]: [MySQL] 23,159 pass(es).
[ View ]

Here is a simple reroll that fixes a bad function call and a code style issue. Using this patch, it seems to fix the problem for me.

+++ includes/menu.inc 22 Aug 2010 06:04:56 -0000
@@ -2797,7 +2797,7 @@ function menu_link_save(&$item) {
-    _menu_link_parents_set($item, $parent);
+    _menu_link_ ($item, $parent);

Bad function call

+++ includes/menu.inc 22 Aug 2010 06:04:56 -0000
@@ -3016,41 +3016,36 @@ function menu_link_children_relative_dep
+    for ($i = 1; $i <= MENU_MAX_DEPTH; $i++) {
+      $fields['p' . $i] = $child_item['p' . $i];
+    }
+    ¶
+    $query->fields($fields);

Code style issue.

Powered by Dreditor.

drifter’s picture

Status:Needs work» Needs review

Posted the same second :)

dixon_’s picture

Status:Needs review» Needs work

Sorry for cross posting... My patch also includes a minor code style issue, though.

dixon_’s picture

Status:Needs work» Needs review
chx’s picture

Stop rerolling this patch it's the wrong approach.

chx’s picture

StatusFileSize
new9.64 KB
PASSED: [[SimpleTest]]: [MySQL] 23,237 pass(es).
[ View ]

Let's see this one. The DBTNG conversion missed an important comment from D6... also the query can be simplified a very little: A - (-B) is just A+B (for depth).

hunvreus’s picture

Status:Needs review» Reviewed & tested by the community

Worked perfectly.

pwolanin’s picture

Status:Reviewed & tested by the community» Needs review

Looks good to me (assuming tests pass) - chx and I discussed the underlying bug, so the only trick was for him to figure out how to properly make it work with DBTNG.

pwolanin’s picture

Status:Needs review» Reviewed & tested by the community

cross post

Dries’s picture

Status:Reviewed & tested by the community» Fixed

The tests pass according to the test bot (but the issue isn't updated yet). Committed to CVS HEAD. Thanks!

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.