Problem/Motivation

When editing a menu item that has a parent menu item, if the internal path (eg, /node/123) is entered, instead of using the entity reference autocomplete, the menu item loses it's hierarchy when caches are cleared.

Steps to reproduce:

  1. Add 2 nodes, in the main menu, with one under the other
  2. Visit the menu admin form and edit the child node
  3. Replace the entity reference in the path, with node/ID
  4. Save the menu item
  5. Clear caches
  6. Reload the menu overview form and note that the hierarchy is gone

Proposed resolution

Figure out what's going on and fix.

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#22 2477337-22.patch2.69 KBdawehner
#14 menu-hierarchy-lost-2477337-14.patch3.33 KBjhedstrom
#14 interdiff.txt735 bytesjhedstrom
#12 menu-hierarchy-lost-2477337-11-TEST-ONLY.patch2.62 KBjhedstrom
#12 interdiff.txt662 bytesjhedstrom
#11 menu-hierarchy-lost-2477337-10-TEST-ONLY.patch2.65 KBjhedstrom
#7 menu-hierarchy-lost-TEST-07.patch2.34 KBjhedstrom
#7 interdiff.txt2.03 KBjhedstrom
#3 menu-hierarchy-lost-TEST-03.patch592 bytesjhedstrom
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented
Parent issue: » #2407505: [meta] Finalize the menu links (and other user-entered paths) system
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Great catch!

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented
Status: Active » Needs review
FileSize
menu-hierarchy-lost-TEST-03.patch592 bytes

This might explain part of the issue. Our hierarchy test is never run. Attached adds this test back, but it is currently failing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

LOL!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented

Nice. This is due to #2104123: Consolidate test methods in MenuRouterTest

Status: Needs review » Needs work

The last submitted patch, 3: menu-hierarchy-lost-TEST-03.patch, failed testing.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented
Status: Needs work » Needs review
FileSize
interdiff.txt2.03 KB
menu-hierarchy-lost-TEST-07.patch2.34 KB

This gets the test down to 1 fail (and no fatal errors), and I'm not sure if the remaining fail is actually correct or not. If we can get this green, it should probably be spun off into a separate issue rather than wait on a fix for this issue.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented
Status: Needs review » Active

I added #2479819: Menu hierarchy tests are not being run and am going to move this patch there, since I don't think getting these tests back in will really address this issue.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented

Status: Active » Needs work

The last submitted patch, 7: menu-hierarchy-lost-TEST-07.patch, failed testing.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented
Status: Needs work » Needs review
FileSize
menu-hierarchy-lost-2477337-10-TEST-ONLY.patch2.65 KB

This test illustrates the failure. The issue has to do with menu items only being set to 'rediscover' if the uri begins with internal.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented
FileSize
interdiff.txt662 bytes
menu-hierarchy-lost-2477337-11-TEST-ONLY.patch2.62 KB

Patch above had an unnecessary call to set the 'rediscover' field.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented

Making this change resolves the issue, but I'm guessing it is not the ideal fix.

diff --git a/core/modules/menu_link_content/src/Entity/MenuLinkContent.php b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
index 5570a2a..7d78d7b 100644
--- a/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
+++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
@@ -186,12 +186,7 @@ public static function preCreate(EntityStorageInterface $storage, array &$values
   public function preSave(EntityStorageInterface $storage) {
     parent::preSave($storage);

-    if (parse_url($this->link->uri, PHP_URL_SCHEME) === 'internal') {
-      $this->setRequiresRediscovery(TRUE);
-    }
-    else {
-      $this->setRequiresRediscovery(FALSE);
-    }
+    $this->setRequiresRediscovery(TRUE);
   }
   /**
    * {@inheritdoc}
jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented
FileSize
interdiff.txt735 bytes
menu-hierarchy-lost-2477337-14.patch3.33 KB

This fix sets rediscovery for all (I think) possible internal uris.

The last submitted patch, 11: menu-hierarchy-lost-2477337-10-TEST-ONLY.patch, failed testing.

The last submitted patch, 12: menu-hierarchy-lost-2477337-11-TEST-ONLY.patch, failed testing.

grendzy’s picture

grendzy CreditAttribution: grendzy at Metal Toad commented
Status: Needs review » Reviewed & tested by the community

Looks good. These 3 URI schemes match what's defined in \Drupal\Core\Url::fromUri.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

I've just committed #2479819: Menu hierarchy tests are not being run, so triggering a retest here just to be sure.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs review 
+++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
@@ -186,7 +186,7 @@ public static function preCreate(EntityStorageInterface $storage, array &$values
+    if (in_array(parse_url($this->link->uri, PHP_URL_SCHEME), ['entity', 'internal', 'route'])) {

I think we were specifically trying to avoid rediscovery for entity references - might need to check the issue history and/or profile what happens with this change.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Component: menu system » menu_link_content.module
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Priority: Major » Critical

Sounds like another duplicate of #2468713: Internal/custom menu links force-reset to the top of their menus on cache rebuild

+++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
@@ -186,7 +186,7 @@ public static function preCreate(EntityStorageInterface $storage, array &$values
+    if (in_array(parse_url($this->link->uri, PHP_URL_SCHEME), ['entity', 'internal', 'route'])) {
       $this->setRequiresRediscovery(TRUE);
     }

This fix is certainly wrong, because route | entity already got discovered, we know their routing information. Its probably just the MenuLink code which is wrong.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
FileSize
2477337-22.patch2.69 KB

Let's try out a test only patch.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Let's commit this test only patch, more test coverage is always good but we don't need the actual bug fix.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Category: Bug report » Task
Priority: Critical » Normal

This can't be a critical anymore.

  • alexpott committed 9dd19b4 on 8.0.x 
    Issue #2477337 by jhedstrom, dawehner, tim.plunkett, sumitmadan: Add...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed 9dd19b4 and pushed to 8.0.x. Thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

Fair point

sumitmadan’s picture

sumitmadan CreditAttribution: sumitmadan at QED42 commented

Cool, Not bug anymore. :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

Adding @sumitmadan to credit.

Status: Fixed » Closed (fixed)

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