Internal/custom menu links force-reset to the top of their menus on cache rebuild

Interesting!

The problem is caused by an interested property of the menu system. It rebuilds its menu links based upon the defined menu links coming from either static menu links or views, but not from custom menu links. Given that core/lib/Drupal/Core/Menu/MenuTreeStorage.php:164 will set the children to a link which doesn't exist,
so upon save, things are totally ignored.

I think one fix would be to special case, when the parent doesn't existing in the $definitions and then somehow update the entry in the DB itself.

Moving to the menu system component, because its root problem is in the underlying menu system component.

@rootwork
One thing we certainly will need is a test, would you be interested in writing one?

Attached you'll find a test for this issue.

Since this is only a test without a fix, the last noassert will fail until fixed.

Renamed patch to make clear this is only the test

.

This problem appears to be more generic than just views. It seems to apply to any menu link that uses the internal: URL scheme, including links to <front> and /contact.

I have reproduced this issue in beta10. An easy way to reproduce this issue:
1. Create a custom menu.
2. Create a node (or use an existing node).
3. Create a menu link, title "Parent", that points to the node from #2.
4. Create a menu link, title "Child", with parent "Parent", linked to '/contact'.
5. Confirm that Child is the child of Parent.
6. Rebuild the cache.
7. Edit your custom menu.
8. Observe that Child is now at the top level, next to Parent.

Furthermore, if you edit "Child," it still has its parent set to "Parent". If you save "Child", it will be a child of "Parent" again, until the next cache rebuild.

We have a patch for this issue. Please credit this patch to Nathan James (https://www.drupal.org/u/tnathanjames) in addition to myself. He did most of the work--I just provided occasional insight and testing.

Also, I'm raising the priority to major, since this issue severely corrupts menus each time the cache is rebuilt. This is untenable for anyone who uses the menu system. I'd really like to set this to Critical because D8 should not release with this bug, but I don't think it quite meets the definition for critical.

I wish I were at the sprint. Sadly, I had to miss Drupalcon this year, though tnathanjames is there (he wrote this fix on the plane). This was to address an issue for a D8 site we're building for a client.

Thank you Paul and Nathan for posting that patch. It was starting to drive me nuts and the patch seems to have fixed the issue for me. Will report back if I notice any issues.

The patch in #14 didn't include the test from #5.

I think this is back to RTBC now that the patch includes the test.

It's great that we have an integration test but is it possible to test this change as part of Drupal\system\Tests\Menu\MenuTreeStorageTest? This is so that one test encompasses all of the functionality of the class under test?

Still an issue with patch.

Steps:

1. Have a menu with some item.
2. Add a view, add it to the menu.
3. Edit the menu and drag it to another place.
4. Flush cache
5. Views menu item resets position. Show as expanded also resets

I'll work on the test change.

This adds a test to MenuTreeStorageTest as suggested above.

I did not run the SimpleTest, but the patch appears to fix the problem described above on a site we are demoing Drupal 8 with.

A related bug also exists for menu items that come from yaml files.
#2548009: Overridden menu links lose parent, expanded and enabled (are disabled) status on cache clear
The test patch from there unsurprisingly also fails with this patch, but the underlying problem is probably related.

Confirmed that the patch at #30 fixes this issue.
(However, it does not fix the related issue linked in #33)

This fixes the issue but I have some small nitpicks.

1. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
   @@ -177,8 +177,17 @@ public function rebuild(array $definitions) {
   +        // Check for a parent that is not loaded above since only internal
   +        // links are loaded above.
   

   "links" should go to the first line.

2. +++ b/core/modules/views/src/Tests/Plugin/MenuLinkTest.php
   @@ -0,0 +1,96 @@
   +  public static $testViews = array('test_menu_link');
   ...
   +  public static $modules = array('views', 'views_ui', 'user', 'node', 'menu_ui', 'block');
   ...
   +    $this->adminUser = $this->drupalCreateUser(array('administer views', 'administer menu'));
   ...
   +    $this->drupalCreateContentType(array('type' => 'page'));
   ...
   +    $options = array(
   

   Can we change all these to short array notation to be more consistent with the rest of the file.

3. +++ b/core/modules/views/src/Tests/Plugin/MenuLinkTest.php
   @@ -0,0 +1,96 @@
   +    // Alter the view's menu link in view page to use the menu link from the node as parent.
   

   Exceeds 80 characters

4. +++ b/core/modules/views/src/Tests/Plugin/MenuLinkTest.php
   @@ -0,0 +1,96 @@
   +    ], t('Apply'));
   

   Do we need t() here? This is not testing anything multilingual.

5. +++ b/core/modules/views/src/Tests/Plugin/MenuLinkTest.php
   @@ -0,0 +1,96 @@
   +    // Test if the node as parent menu item is selected in our views settings
   

   Should end with a "."

6. +++ b/core/modules/views/src/Tests/Plugin/MenuLinkTest.php
   @@ -0,0 +1,96 @@
   +    // Test if the primary menu item (node) is visible, and the secondary menu item (view) is hidden
   

   Exceeds 80 characters, should end with a "."

Patch from #30with remarks from #37 addressed in this patch

The changes look good, setting RTBC

This is both views and menu so assigning to Daniel for review but tagging since pwolanin would be good on the menu bit.

And this is data loss every cache rebuild so bumping to critical.

I think the fix is the right thing to do. We still don't iterate over all stored menu links, as that would be too costly, but instead
we just care about the probably rare cases for "static" menu links, which have

The interdiff just changes some minor details + adds a bit more of documentation so the code is hopefully better to understand in the future.

I've tried to reproduce #26 and can't.

Committed 5cd3b68 and pushed to 8.0.x. Thanks!

Issue description from #26 still occurs if you add a 2nd view:

Steps:

Have a menu with some item.
Add a view, add it to the menu.

Add a 2nd view; add it to the menu.

Edit the menu and drag it to another place.
Flush cache
Views menu item resets position. Show as expanded also resets*

*Show as expanded did not appear to be an issue.

note: I have some uncertainty whether reopening this (critical) issue is the right course of action; I'm choosing to do so since from what I can tell the issue I'm seeing is in fact the same, and work on fixing it will benefit from the history logged in this issue. If someone has a strong opinion about creation of a new ticket, no problem from my perspective.

@kay_v that sounds similar, but please open a new issue linking to it from this one - can use the 'related issues' field to do so.

looking further at the issue and messing around with steps, a few things seem worth noting:

1. when exporting the configuration for the second View, no 'weight' attribute appears under the Menu section (no mention of weight at all)
2. if you deliberately set a weight within the View UI, it sticks even after cache is cleared
3. if you deliberately set a weight within the Menu UI, it does not stick beyond first save of the menu changes

thanks @catch - will open a new ticket and mark it as 'normal'

closing this issue and opening new one: #2695893: Views menu item resets position