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.

CommentFileSizeAuthor
#31 mysql_is_braindead_die_die_die.patch9.64 KBchx
#26 408482-25.patch10.72 KBdixon_
#25 408482-24.patch10.67 KBdrifter
#21 408482-21.patch11.07 KBdrifter
#16 menu_link_move_children-7.patch12.68 KBc960657
#14 menu_link_move_children-7.patch16.74 KBc960657
#13 menu_link_move_children-6.patch12.12 KBc960657
#12 menu_link_move_children-5.patch12.12 KBc960657
#11 menu_link_move_children-4.patch12.11 KBc960657
#7 menu_link_move_children-3.patch12.12 KBc960657
#5 menu_link_move_children-2.patch12.08 KBc960657
#1 menu_link_move_children-1.patch12.09 KBc960657
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

c960657’s picture

c960657 CreditAttribution: c960657 commented
Status: Active » Needs work
FileSize
menu_link_move_children-1.patch12.09 KB

_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

c960657 CreditAttribution: c960657 commented
Status: Needs work » Needs review
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

subscribing

oadaeh’s picture

oadaeh CreditAttribution: oadaeh commented

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

c960657’s picture

c960657 CreditAttribution: c960657 commented
FileSize
menu_link_move_children-2.patch12.08 KB

@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

c960657 CreditAttribution: c960657 commented
Status: Needs work » Needs review
FileSize
menu_link_move_children-3.patch12.12 KB

Reroll.

pflame’s picture

pflame CreditAttribution: pflame commented

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

mattyoung’s picture

mattyoung CreditAttribution: mattyoung commented

.

Status: Needs review » Needs work

The last submitted patch failed testing.

c960657’s picture

c960657 CreditAttribution: c960657 commented
Status: Needs work » Needs review
FileSize
menu_link_move_children-4.patch12.11 KB

Reroll.

c960657’s picture

c960657 CreditAttribution: c960657 commented
FileSize
menu_link_move_children-5.patch12.12 KB

Reroll.

c960657’s picture

c960657 CreditAttribution: c960657 commented
FileSize
menu_link_move_children-6.patch12.12 KB
c960657’s picture

c960657 CreditAttribution: c960657 commented
FileSize
menu_link_move_children-7.patch16.74 KB

Reroll (due to #682784: Once created, menu item query strings can never be deleted).

Status: Needs review » Needs work

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

c960657’s picture

c960657 CreditAttribution: c960657 commented
FileSize
menu_link_move_children-7.patch12.68 KB

Sorry, the last patch contained some unrelated changes.

c960657’s picture

c960657 CreditAttribution: c960657 commented
Status: Needs work » Needs review
oadaeh’s picture

oadaeh CreditAttribution: oadaeh commented
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

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented 
+++ 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

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
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

drifter CreditAttribution: drifter commented
Status: Needs work » Needs review
FileSize
408482-21.patch11.07 KB

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

chx’s picture

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

that's not going to fly.

chx’s picture

chx CreditAttribution: chx commented
Assigned: Unassigned » chx
Priority: Major » Critical

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

chx’s picture

chx CreditAttribution: chx commented

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

drifter CreditAttribution: drifter commented
FileSize
408482-24.patch10.67 KB

Ehm, typo, thanks chx!

dixon_’s picture

dixon_
he/him
Primary language English
Location France
CreditAttribution: dixon_ commented
FileSize
408482-25.patch10.72 KB

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

drifter CreditAttribution: drifter commented
Status: Needs work » Needs review

Posted the same second :)

dixon_’s picture

dixon_
he/him
Primary language English
Location France
CreditAttribution: dixon_ commented
Status: Needs review » Needs work

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

dixon_’s picture

dixon_
he/him
Primary language English
Location France
CreditAttribution: dixon_ commented
Status: Needs work » Needs review
chx’s picture

chx CreditAttribution: chx commented

Stop rerolling this patch it's the wrong approach.

chx’s picture

chx CreditAttribution: chx commented
FileSize
mysql_is_braindead_die_die_die.patch9.64 KB

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

hunvreus CreditAttribution: hunvreus commented
Status: Needs review » Reviewed & tested by the community

Worked perfectly.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
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

pwolanin CreditAttribution: pwolanin commented
Status: Needs review » Reviewed & tested by the community

cross post

Dries’s picture

Dries CreditAttribution: Dries commented
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.