Posted by oadaeh on March 20, 2009 at 3:29pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | menu system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | chx |
| Status: | closed (fixed) |
Issue Summary
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 2becomes this:
Menu 1
|
+--Menu item 1
|
+--Menu item 2
|
+--Menu item 3
Menu 2
|
+--SubmenuThe 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.
Comments
#1
_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.
#2
#3
subscribing
#4
@c960657: The patch did not work for me against the 7.x-dev released on September 21, 2009 at 05:10.
#5
@oadaeh: Hmm, it works for me. What happens instead?
This is just a reroll.
#6
The last submitted patch failed testing.
#7
Reroll.
#8
@c960657 The patch worked for me, I used drupal7 from cvs.
#9
.
#10
The last submitted patch failed testing.
#11
Reroll.
#12
Reroll.
#13
#14
Reroll (due to #682784: Once created, menu item query strings can never be deleted).
#15
The last submitted patch, menu_link_move_children-7.patch, failed testing.
#16
Sorry, the last patch contained some unrelated changes.
#17
#18
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.
#19
+++ 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!
#20
#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.
#21
Menu names were not being updated in patch #16, fixed. Also, rewritten the update expression syntax to fields as per #20.
#22
- _menu_link_parents_set($item, $parent);+ _menu_link_ ($item, $parent);
that's not going to fly.
#23
http://drupal.org/node/576290 depends on this so critical.
#24
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.
#25
Ehm, typo, thanks chx!
#26
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.
#27
Posted the same second :)
#28
Sorry for cross posting... My patch also includes a minor code style issue, though.
#29
#30
Stop rerolling this patch it's the wrong approach.
#31
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).
#32
Worked perfectly.
#33
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.
#34
cross post
#35
The tests pass according to the test bot (but the issue isn't updated yet). Committed to CVS HEAD. Thanks!
#36
Automatically closed -- issue fixed for 2 weeks with no activity.