Problem

When you delete a menu link (directly or through deleting a node with menu link and fireing the hook menu_link_content_entity_predelete), if the menu link had any children, their reference to the parent gets orphaned in the MenuLinkContent entity, but is remapped to the grandparent in the menu_tree table by the MenuTreeStorage.
Depending on the data displayed you will see the orphaned children menu links moved up 1 level, or pointing to the root menu. In the database the menu_link_content_data.parent column contains a reference to a non-existing entity. This can cause errors and problems.

Steps to reproduce

  1. clean install D8.6 dev
  2. create 3 nested menu links in a menu, e.g. in Main Navigation
  3. delete the 'middle' menu link
  4. look at /admin/structure/menu/manage/main: this should show link 3 as child of link 1
  5. edit link 3: this should show the parent link as the menu root, not link 1

Proposed resolution

Take the logic of remapping parent references to the grandparent as used in \Drupal\Core\Menu\MenuTreeStorage::delete and apply this to \Drupal\menu_link_content\Entity\MenuLinkContent::preDelete too.

Remaining tasks

  • Create a patch
  • Add automated tests

User interface changes

None.

API changes

None.

Data model changes

After deleting MenuLinkContent entities any children will have their parent refs remapped to their grantparent in both data tables: menu_tree and menu_link_content_data.

CommentFileSizeAuthor
#11 2951548-11.patch2.98 KBfinne
#11 2951548-11-test-only.patch1.98 KBfinne
#9 2951548-9.patch2.75 KBfinne
#9 2951548-9-test-only.patch1.76 KBfinne
#6 2951548-6.patch2.88 KBfinne
#2 Screen Shot 2018-03-09 at 14.59.09.png7.42 KBfinne
#2 Screen Shot 2018-03-09 at 14.53.39.png6.42 KBfinne
#2 Screen Shot 2018-03-09 at 14.52.08.png7.47 KBfinne
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

finne created an issue. See original summary.

finne’s picture

finne
Primary language Dutch
Location Amsterdam
CreditAttribution: finne at Dx Experts commented
Issue summary: View changes
FileSize
Screen Shot 2018-03-09 at 14.52.08.png7.47 KB
Screen Shot 2018-03-09 at 14.53.39.png6.42 KB
Screen Shot 2018-03-09 at 14.59.09.png7.42 KB
finne’s picture

finne
Primary language Dutch
Location Amsterdam
CreditAttribution: finne at Dx Experts commented
Issue summary: View changes
finne’s picture

finne
Primary language Dutch
Location Amsterdam
CreditAttribution: finne at Dx Experts commented
Issue summary: View changes
finne’s picture

finne
Primary language Dutch
Location Amsterdam
CreditAttribution: finne at Dx Experts commented
Issue summary: View changes
finne’s picture

finne
Primary language Dutch
Location Amsterdam
CreditAttribution: finne at Dx Experts commented
FileSize
2951548-6.patch2.88 KB

Created patch for this issue, including test.

finne’s picture

finne
Primary language Dutch
Location Amsterdam
CreditAttribution: finne at Dx Experts commented
Status: Active » Needs review
idebr’s picture

idebr CreditAttribution: idebr at ezCompany commented
Status: Needs review » Needs work

Menu link content children are re-reparented correct after the patch.

+++ b/core/modules/menu_link_content/tests/src/Kernel/MenuLinkContentDeleteTest.php
@@ -0,0 +1,59 @@
+use Drupal\Core\Menu\MenuTreeParameters;
+use Drupal\Core\StringTranslation\TranslatableMarkup;
...
+use Symfony\Component\Routing\Route;

However, there are a few unused statement in your test.

finne’s picture

finne
Primary language Dutch
Location Amsterdam
CreditAttribution: finne at Dx Experts commented
FileSize
2951548-9-test-only.patch1.76 KB
2951548-9.patch2.75 KB

Rerolled patch and added test-only patch.

finne’s picture

finne
Primary language Dutch
Location Amsterdam
CreditAttribution: finne at Dx Experts commented
Status: Needs work » Needs review
finne’s picture

finne
Primary language Dutch
Location Amsterdam
CreditAttribution: finne at Dx Experts commented
FileSize
2951548-11-test-only.patch1.98 KB
2951548-11.patch2.98 KB

improved test to actually test if the correct parent ID is set.

The last submitted patch, 9: 2951548-9-test-only.patch, failed testing. View results

The last submitted patch, 11: 2951548-11-test-only.patch, failed testing. View results

idebr’s picture

idebr CreditAttribution: idebr at ezCompany commented
Status: Needs review » Reviewed & tested by the community

Test coverage looks good now, settings to RTBC.

alexpott’s picture

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

I tested this on Drupal 7 add can confirm this is a regression.

Committed and pushed 2cdab9c011 to 8.6.x and 9e3a46258e to 8.5.x. Thanks!

  • alexpott committed 2cdab9c on 8.6.x 
    Issue #2951548 by finne, idebr: Restore Menu Link parent references when...

  • alexpott committed 9e3a462 on 8.5.x 
    Issue #2951548 by finne, idebr: Restore Menu Link parent references when...

Status: Fixed » Closed (fixed)

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