Problem/Motivation

Any menu link that uses the internal: URL scheme, including views, links to , and /contact, will be force-reset to the top level of its menu on cache rebuild.

Steps to reproduce:

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.

Screenshot - view setting specifying parent menu item:

view setting specifying parent menu item

Screenshot - main menu tree displaying all menu items at the same (top) level:

main menu tree displaying all menu items at the same (top) level

It doesn't matter what the parent menu item is. This occurs regardless of the parent item's depth -- i.e. if the parent is 3 levels deep, the view's menu item will still end up at the top level.

If you then edit the menu item directly, change nothing, and save it, it will then acquire the correct parent.

For menu links from views, the issue is compounded by the fact that views rebuilds the menus anytime any view is saved (see #941970: Views rebuilds the menu more than it needs to) so it means you're often "fixing" the menus.

This may be related to #2202493: views_menu_link_defaults() does not set a parent for links although that appears to have been fixed some time ago.

Proposed resolution

Add checks for internal/custom menu links prior to force-reset of menu items to the top.

Write tests.

Remaining tasks

Review.

User interface changes

None.

API changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because of the way it garbles your menus.
Issue priority Critical because it results in data loss.
Prioritized changes The main goal of this issue is developer/site editor usability.

Original report:

Steps to reproduce:

- Standard install.
- Create two pages and place them both in the main menu at the top level.
- Create a view with a page. Add a menu link to the view's page display, setting the parent item to the first node's menu item.
- Visit admin/structure/menu/manage/main to see the menu tree.

Expected result: The two node menu items are at the top level, with the view menu item underneath of (a secondary menu item for) the first node menu item.

Actual result: All three menu items are at the top level.

Files: 
CommentFileSizeAuthor
#43 interdiff.txt3.44 KBdawehner
#43 2468713-42.patch7.59 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,461 pass(es). View
#38 internal_custom_menu-2468713-38.patch6.75 KBStryKaizer
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,422 pass(es). View
#38 interdiff.txt3.94 KBStryKaizer
#30 menu-links-rebuild-2468713-30.patch6.76 KBjhedstrom
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 108,220 pass(es). View
#30 menu-links-rebuild-2468713-30-TEST-ONLY.patch1.81 KBjhedstrom
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,293 pass(es), 4 fail(s), and 0 exception(s). View
#30 interdiff.txt1.81 KBjhedstrom

Comments

dawehner’s picture

Component: views.module » menu system

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’s picture

Yeah, I thought it might have to do with the intermixing of menu links set from nodes and menu links set from views.

Thanks for investigating this. I'll keep following this and help test patches and such if I can.

dawehner’s picture

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

StryKaizer’s picture

FileSize
3.92 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,577 pass(es), 2 fail(s), and 0 exception(s). View

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.

StryKaizer’s picture

Status: Active » Needs review
FileSize
3.92 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,652 pass(es), 1 fail(s), and 0 exception(s). View

Renamed patch to make clear this is only the test

rootwork’s picture

This is great, thank you. Unfortunately I probably don't have the skills to write a test, so I'm glad you have StryKaizer.

Status: Needs review » Needs work

The last submitted patch, 5: test_only-menu_links_created_in-2468713-5.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 5: test_only-menu_links_created_in-2468713-5.patch, failed testing.

The last submitted patch, 4: menu_links_created_in-2468713-4.patch, failed testing.

dawehner’s picture

Issue tags: +VDC

.

paulmckibben’s picture

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.

rootwork’s picture

Interesting, thanks for that testing. I agree that it's bizarre that the menu item still apparently has its parent item set correctly -- all it takes is re-saving the menu item and it will be correctly placed.

paulmckibben’s picture

Priority: Normal » Major
Status: Needs work » Needs review
FileSize
1.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

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.

rootwork’s picture

This is so exciting, because this has been a MAJOR pain for me in development (and yes yes, I know, betas aren't stable and I shouldn't expect them to be).

I will test this out shortly but just wanted to say thanks so much for your work addressing this. Was this work from the sprint?

paulmckibben’s picture

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.

tanc’s picture

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.

rootwork’s picture

Status: Needs review » Reviewed & tested by the community

Confirmed that this applies cleanly and fixes the bug. It's a beautiful thing.

Setting this to RTBC though I imagine it will have to get some reviewing by D8 component maintainers.

jhedstrom’s picture

Status: Reviewed & tested by the community » Needs work

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

rootwork’s picture

Status: Needs work » Needs review
FileSize
4.95 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,577 pass(es). View
3.92 KB

Reroll of #14 with the test from #5 included.

rootwork’s picture

Title: Menu links created in views will only appear at the top level of a menu, regardless of the view settings » Internal/custom menu links force-reset to the top of their menus on cache rebuild
Issue summary: View changes

Updated the issue summary and title for clarity. I also added a beta evaluation, though I've never done that before so please check my work...

The last submitted patch, 14: menu-links-2468713-7.patch, failed testing.

jhedstrom’s picture

Status: Needs review » Reviewed & tested by the community

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

rootwork’s picture

Great!

Note @paulmckibben's request to credit the patch (from #14) to Nathan James @tnathanjames in addition to himself.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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?

harings_rob’s picture

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
shaktik’s picture

Assigned: Unassigned » shaktik
shaktik’s picture

Assigned: shaktik » Unassigned
jhedstrom’s picture

Assigned: Unassigned » jhedstrom

I'll work on the test change.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
1.81 KB
1.81 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,293 pass(es), 4 fail(s), and 0 exception(s). View
6.76 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 108,220 pass(es). View

This adds a test to MenuTreeStorageTest as suggested above.

The last submitted patch, 30: menu-links-rebuild-2468713-30-TEST-ONLY.patch, failed testing.

christianmgrupp’s picture

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.

bircher’s picture

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.

paulmckibben’s picture

Status: Needs review » Reviewed & tested by the community

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 30: menu-links-rebuild-2468713-30.patch, failed testing.

Status: Needs work » Needs review
geertvd’s picture

Status: Needs review » Needs work

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

StryKaizer’s picture

Status: Needs work » Needs review
FileSize
3.94 KB
6.75 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,422 pass(es). View

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

geertvd’s picture

Status: Needs review » Reviewed & tested by the community

The changes look good, setting RTBC

catch’s picture

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

catch’s picture

Priority: Major » Critical

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

rootwork’s picture

Issue summary: View changes

Updated the beta evaluation with the new priority. Thanks catch.

dawehner’s picture

Assigned: dawehner » Unassigned
FileSize
7.59 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,461 pass(es). View
3.44 KB

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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs subsystem maintainer review

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

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

  • alexpott committed 5cd3b68 on 8.0.x
    Issue #2468713 by StryKaizer, jhedstrom, rootwork, dawehner,...

  • alexpott committed 0a57817 on 8.0.x
    Issue #2468713 by StryKaizer, jhedstrom, rootwork, dawehner,...
  • alexpott committed 6374e30 on 8.0.x
    Revert "Issue #2468713 by StryKaizer, jhedstrom, rootwork, dawehner,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

kay_v’s picture

Status: Closed (fixed) » Needs work

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.

catch’s picture

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

kay_v’s picture

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
kay_v’s picture

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

kay_v’s picture

Status: Needs work » Closed (fixed)

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