Menu items - "Has children" flag is not reset on menu reordering

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

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

1. Create two menu items at the root level.
2. Drag the second item so it becomes a child of the first item, and save the form.
3. Drag the second item above the first item, so that the weight of the first item changes from 50 to 49. Save the form.

The parent item has its 'has_children' flag still set to 1.

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.

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

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

#6: 955378-6-menu_link-reorder-has_children-test_only.patch queued for re-testing.

It's been over a month, let's see if the patch is still valid.

6: 955378-6-menu_link-reorder-has_children.patch queued for re-testing.

Straight reroll.

Patch is good. Flag "has_children" is resetted properly.

Thanks :)

Re-roll because of \Drupal\menu\Tests\MenuTest.

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

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

What about older versions?

Can we confirm whether or not this is still a problem in drupal 7.

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

Yes, this issue still exists in 7.x (7.34).

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

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

Fixed path in patch.

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

And here is one with the test and patch, that will hopefully pass

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

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

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

There is some debug code left in patch #29 in themes/seven/template.php.

Re: #29

+++ b/themes/seven/template.php
@@ -1,5 +1,16 @@
+    //kpr($form);
...
+function clr_debug_form(&$form, &$form_state) {
+  //kpr($form_state['values']);
+}

I even put my initials, whoops!

Addresses #34. There should be no changes to Seven. [Edit - sorry about the test on the interdiff, wrong extension in my haste]

Removed D6 backport tag. Setting to RTBC per #32 and #33.

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

Item 1
- Item 2

to:

Item 2
- Item 1

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?

Here's a patch that does that. In my brief manual testing, all scenarios seemed to work.

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.

It looks good to me overall, but it does seem a little repetitive, for example:

+     * In order to reproduce the following steps are needed
+     * 1. reset all weights, isoloating two menu items
+     * 2. Make x menu item a child of y
+     * 3. Move x to root with lighter weight than y
+     * 4. Make y a child of x
+     * 5. Move y to root with ligther weight than x

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?

Re-testing this since that looks like a bogus test failure.

Applied 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

As 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 still 1, 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.