Problem/Motivation
The db column "has_children" in the table "menu_links" is a flag which marks if a menu item is a parent menu item. Its default is 0, i. e. no children. Once another menu item becomes a child, e. g. by creating or moving, the flag is set to 1, i. e. has children. When all children are moved away from its parent, the parent's "has_children" flag doesn't change, i. e. it still remains 1 which is wrong.
The problem comes with theming as menu item with children obtain the CSS class "collapsed". Obviously there's a work around in the theme, but the attached core patch addresses the problem of updating the parent menu item's "has_children" status when moving child menu items. Deletion and creation of child menu item should update the flag as expected.
The bug occurs when using drag and drop ordering, and when the child item is moved above the parent item, causing the weight of the parent item to change. When this happens, the triangle icon that indicates a menu item has a child does not disappear. In addition, the css class of the menu item that used to be a parent will retain the li class="collapsed".
Steps to repeat:
- Install Drupal
- Create new menu -- Home/Administration/Structure /admin/structure/menu
- Give the new menu a title and save
- Add new menu item at root level
- Add menu to desired block area -- Home/Administration/Structure /admin/structure/block
- View site to make sure that the new menu item in the new menu doesn't have a triangle next to it showing it to be a parent
- Go back to the new menu on the menus page -- Home/Administration/Structure /admin/structure/menu
- Select your new menu by clicking edit menu to the right of the menu's name
- Add a new menu item and make it a child of the first menu item by dragging it underneath the original menu item and to the right
- View site to make sure that the first new menu item in the new menu has a triangle next to it showing it to be the parent of the second new menu item
- Go back to the new menu on the menus page -- Home/Administration/Structure /admin/structure/menu
- Select your new menu by clicking edit menu to the right of the menu's name
- Move the second new menu item to the left to make it on the same level as the first menu item and no longer its child, then drag it above the first menu item in the list. Ensure that something about the first menu item is changed also in order to force it to be resaved on submit (this sometimes happens automatically via drag and drop, but not always, so to ensure it will happen you can click "show row weights" and manually adjust that item's weight to a different value than it had before).
- View site. The first menu item no longer has children, so it should no longer have a triangle next to it showing it to be a parent. However, with this bug, it will still have the triangle.
Proposed resolution
If the child item is saved before the original parent, it will correctly update the 'has_children' flag of the parent (see moveChildren()). When the parent is saved later on it will overwrite it with the original value again.
We have to make sure the original parent item is saved before the child item. This has to be solved in MenuFormController::submitOverviewForm(). Here the order is determined in which the items are saved.
Remaining tasks
Test written and needs reviews.
Original report by [username]
The db column "has_children" in the table "menu_links" is a flag which marks if a menu item is a parent menu item. Its default is 0, i. e. no children. Once another menu item becomes a child, e. g. by creating or moving, the flag is set to 1, i. e. has children. When all children are moved away from its parent, the parent's "has_children" flag doesn't change, i. e. it still remains 1 which is wrong.
The problem comes with theming as menu item with children obtain the CSS class "collapsed". Obviously there's a work around in the theme, but the attached core patch addresses the problem of updating the parent menu item's "has_children" status when moving child menu items. Deletion and creation of child menu item should update the flag as expected.
Comment | File | Size | Author |
---|
Comments
Comment #1
gooddesignusa CreditAttribution: gooddesignusa commentedwow I didn't think this would of been the issue. I was in the process of setting up the css for my menu and some menu items had the wrong class names. Most were correct but some were wrong. I assumed it was something on my end. I applied the patch but it doesn't seem to fix the couple menu items that are having the wrong class name. I tried reordering the menu to see if that was need. Also cleared the cache.
I'm using D6.20 and when I applied the patch it said "Hunk #1 succeeded at 2233 (offset -5 lines)." Not sure if that can help.
Comment #2
pfrenssenThis bug is still present in Drupal 8. The bug occurs when using drag and drop ordering, and when the child item is moved above the parent item, causing the weight of the parent item to change. Steps to repeat:
The parent item has its 'has_children' flag still set to 1.
Comment #3
pfrenssenIf the child item is saved before the original parent, it will correctly update the 'has_children' flag of the parent (see moveChildren()). When the parent is saved later on it will overwrite it with the original value again.
We have to make sure the original parent item is saved before the child item. This has to be solved in MenuFormController::submitOverviewForm(). Here the order is determined in which the items are saved.
Comment #4
pfrenssenHere's a patch. This still needs a test, but let's see that it does not break anything. The order in which menu items are saved is critical.
Comment #6
pfrenssenWriting a test for this was a bit harder than I expected. The bug only occurs after the menu items have been reordered in the DOM using the tabledrag functionality, and Simpletest does not have any helpers for DOM manipulation. I managed to get it done with some inspiration from WebTestBase::parse().
Comment #7
pfrenssen#6: 955378-6-menu_link-reorder-has_children-test_only.patch queued for re-testing.
Comment #8
pfrenssenComment #9
pfrenssenIt's been over a month, let's see if the patch is still valid.
Comment #10
pfrenssen6: 955378-6-menu_link-reorder-has_children.patch queued for re-testing.
Comment #11
pfrenssenStraight reroll.
Comment #12
BladeduPatch is good. Flag "has_children" is resetted properly.
Thanks :)
Comment #13
XanoRe-roll because of \Drupal\menu\Tests\MenuTest.
Comment #14
bannorb CreditAttribution: bannorb commentedI tried to understand the issue and summarize it.
This patch worked for me using simplytest.me.
The triangle indicating menu item children is correctly not showing and the css li class="collapsed" has correctly been removed for menu items that once had children but now longer have children.
I had to take different steps than the ones outlined in the issue summary because I couldn't drag a menu item up. That bug has nothing to do with this issue but we might want to open a separate issue for that.
What I did was:
I created a new menu that appeared in sidebar first to test this patch. While going through the steps, I was unable to move the child item above the parent as described in the summary and comment #2. I was able to move it down below another menu item and to the left in order to break the parent/child link.
Patch screenshots:
Comment #15
bannorb CreditAttribution: bannorb commentedComment #17
bannorb CreditAttribution: bannorb commentedI rechecked this issue using simplytestme.com and also on my localhost with the latest Drupal 8 version and no patch.
Steps to reproduce:
1. Create articles for testing.
2. Create menu 3 menu links for testing.
3. Move test link 2 to be a child of test link 1.
4. Move test link 2 back to not be a child of test link 1.
5. Check to see if triangle next to test link 1 is not there.
I tested this on both Drupal8 installations (simplytestme.com and localhost) using 2 methods:
1. Drag test link 2 to the right to be a child of test link 1.
2. Click on the edit link for test link 2 and make it a child of test link 1 by using the drop down menu under Parent Link.
3. The triangle next to link 1 was gone as it should be.
I was unable to reproduce this problem. I think this issue can be closed.
Comment #18
rooby CreditAttribution: rooby commentedWhat about older versions?
Can we confirm whether or not this is still a problem in drupal 7.
Comment #19
dcam CreditAttribution: dcam commentedI can confirm that this issue still exists in 7.x. I just tested it and the triangle list icon remains when the child link is moved using the menu admin tabledrag.
The test is potentially still useful for 8.x to prevent the problem from reoccurring, but it may be possible to forward-port the test after 7.x is fixed. I don't know what the rule is on that. The test may have to go into 8.0.x first.
Comment #20
azovsky CreditAttribution: azovsky commentedYes, this issue still exists in 7.x (7.34).
Comment #21
pfrenssenComment #22
Screenack CreditAttribution: Screenack as a volunteer and commentedI am experiencing this issue with 7.41. Thanks to the discussion, above, I was able to address the errant classing by deleting and recreating the "misbehaving" menu items.
Comment #23
chrisrockwell CreditAttribution: chrisrockwell commentedI ported this patch and it is working for me;
however I'm very new to writing tests and while trying to figure out how to write one for this, I discovered that /modules/menu.test isn't being ran, it seems that menu.test within simpletest is running instead. Should the test be written there and submitted as a patch for simpletest? Or am I just missing something?I'm going to work next on a test.Comment #25
chrisrockwell CreditAttribution: chrisrockwell commentedFixed path in patch.
Comment #26
chrisrockwell CreditAttribution: chrisrockwell commentedComment #28
chrisrockwell CreditAttribution: chrisrockwell commentedI'll preface this with, this is my first attempt at a test and would love feedback. I found it particularly difficult to create a failing test because it took me awhile to find out how I could reproduce manually at-will. For this reason, the test might be unnecessarily complicated. If I simply updated the test in #11 for d7 it would not fail.
So, here is a patch that I expect to fail on
$this->assertMenuLink($item4['mlid'], array('has_children' => 0));
Comment #29
chrisrockwell CreditAttribution: chrisrockwell commentedAnd here is one with the test and patch, that will hopefully pass
Comment #31
chrisrockwell CreditAttribution: chrisrockwell commentedI may have submitted the second patch too quickly as the last recorded test activity is incorrect: the last submitted patch did pass. Do I need to do anything here?
Comment #32
karolinam CreditAttribution: karolinam commentedI was able to reproduce the issue before applying the patch on Drupal 7.50.
I tested patch d7-menu-reset-has-children-955378-29.patch, from comment #29. Patch fixes the issue. Moving a child above it's parent works correctly. The 'has_children' flag is updated correctly and css li collapsed class is removed from menu item. Other cases, such as removing a child by shifting it to the left, moving it down, or disabling the child menu link work as well.
Code still needs to be reviewed.
Comment #33
szeidler CreditAttribution: szeidler at Ramsalt Lab commentedI also stumbled across that issue, when one client of us reported the issue.
I can confirm that path #29 solves the problem, using Drupal 7.50.
Comment #34
Chris Burge CreditAttribution: Chris Burge commentedThere is some debug code left in patch #29 in themes/seven/template.php.
Comment #35
chrisrockwell CreditAttribution: chrisrockwell commentedRe: #29
I even put my initials, whoops!
Comment #36
chrisrockwell CreditAttribution: chrisrockwell commentedAddresses #34. There should be no changes to Seven. [Edit - sorry about the test on the interdiff, wrong extension in my haste]
Comment #38
chrisrockwell CreditAttribution: chrisrockwell commentedComment #39
Chris Burge CreditAttribution: Chris Burge commentedRemoved D6 backport tag. Setting to RTBC per #32 and #33.
Comment #40
stefan.r CreditAttribution: stefan.r commentedComment #41
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI tested this patch and it causes a regression in the case where a parent and child are reversed - in other words, if you go from:
to:
all at once via drag-and-drop, everything gets completely broken after saving. I think that's because the patch violates the lengthy code comment at the top of menu_overview_form_submit() which explains that parents always need to be saved before their children.
As a possible alternative fix, if the bug here is essentially that we're sometimes passing incorrect 'has_children' data into menu_link_save(), maybe we can just not pass that property in and always let menu_link_save() recalculate it?
Comment #42
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedHere's a patch that does that. In my brief manual testing, all scenarios seemed to work.
It looks good to me overall, but it does seem a little repetitive, for example:
Not sure that steps 4 and 5 actually add anything, since they're the reverse of steps 2 and 3?
One thing I discovered is that reproducing this can be tricky because when you move x out from underneath y, triggering the bug requires not only making sure x appears before y in the list, but also requires making sure that some property of y changes. If you don't change anything about y, the code won't bother trying to resave y, so the bug isn't triggered. If you use drag-and-drop alone, sometimes that will have the effect of changing y's weight but sometimes it won't, so it can make the bug appear sporadically. However, if you click "show row weights" and then manually change y's weight, you can reproduce it every time. I am updating the issue summary too, since the steps to reproduce there were missing that and were inaccurate otherwise also.
Maybe with that bit of info it's possible to get the tests down to the first three steps only? Also, the code comment above is an inline code comment, so to follow coding standards it should use the
//
comment syntax. And instead of the step 1/2/3 which is a little hard to follow when reading the code below it, it might be better to have a couple sentences at the beginning explaining conceptually what is being tested, then leave the details of each step to the code comment that appears right above the code that follows that step?Comment #43
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedComment #45
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedRe-testing this since that looks like a bogus test failure.
Comment #46
achen44 CreditAttribution: achen44 commentedApplied the latest patch d7-menu-reset-has-children-955378-42.patch and the bug still occurs: "collapsed" class remains on the parent menu after unpublishing a child node.
Steps to reproduce:
1- Make menu_child a child of menu_parent
2- Unpublish menu_child
3- An orphan arrow(caused by CSS selector .collapsed class) will show up beside menu_parent even though no published child is present
Comment #47
szeidler CreditAttribution: szeidler at Ramsalt Lab commentedAs far as I can see is #46 another closely related issue. As far as I was able to reproduce it, the patch #42 is solving the issue of reordering the menu items.
The problem described in #46 is another one, that I also encountered: Menu items for nodes, that were unpublished get automatically hidden in the menu UI. If you unpublish the node with a previously enabled menu item, the menu item remains active in the database (which might be correct), but gets hidden in the UI.
During the menu rendering it simply checks for the flag
'has_children'
of the parent item, which is still1
, because of the hidden but active childrens.That all in all is quite confusing for the content manager, because depending on the theme the parent menu item uses the "collapsed" class, although there is actually nothing to expand.