Problem/Motivation

menu_link_content_entity_predelete() deletes all entity URI relations, with no exception for link that do not relate to the particular entity. For example, "collection", "add-form", "add-page", etc. links.

Steps to reproduce:
- create a menu link pointing to /admin/people on any menu
- delete any user (not "disable" but "delete")
Expected result: menu link still present in the menu.
Actual result: menu link is gone.

Proposed resolution

menu_link_content_entity_predelete() should exclude links that do not refer to the entity being deleted.

CommentFileSizeAuthor
#8 2939692-8.patch3.4 KBLeksat
#3 2939692-3.patch3.55 KBLeksat
#3 2939692-3--test-only.patch2.13 KBLeksat
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Leksat created an issue. See original summary.

Leksat’s picture

Leksat CreditAttribution: Leksat at Amazee Labs commented
Issue summary: View changes
Leksat’s picture

Leksat CreditAttribution: Leksat at Amazee Labs commented
Status: Active » Needs review
FileSize
2939692-3--test-only.patch2.13 KB
2939692-3.patch3.55 KB

The last submitted patch, 3: 2939692-3--test-only.patch, failed testing. View results

Leksat’s picture

Leksat CreditAttribution: Leksat at Amazee Labs commented
Priority: Normal » Critical

Hmm, according to Priority levels of issues, this should be Critical:

Cause loss/corruption of stored data.

Even if it's not critical, it's very annoying bug because it's really difficult to catch it. For example, bug reports from customers may look like:
- menu reverts itself every 1 hour
- menu item disappeared after deployment
It took us some time to find menu_link_content_entity_predelete().

blazey’s picture

blazey CreditAttribution: blazey at Amazee Labs commented
Status: Needs review » Reviewed & tested by the community

Wow, this is really critical. Thanks for finding and fixing @Leksat.

The bug has been reproduced on a clean drupal 8 instance. The patch fixes it. Code changes are minor and they conform to coding standards. The test is provided and targets the issue well, so moving to RTBC.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Needs review 
+++ b/core/modules/menu_link_content/menu_link_content.module
@@ -89,7 +89,17 @@ function menu_link_content_path_delete($path) {
+    if (isset($link_templates[$rel]) && strpos($link_templates[$rel], $entity_type_placeholder) === FALSE) {
+      // We should not delete links that do not relate to this exact entity. For
+      // example, "collection", "add-form", etc. To detect whether or not a link
+      // refers to the entity, we check if [entityType] placeholder presents in
+      // the link template.
+      /** @see \Drupal\Core\Entity\EntityTypeInterface::getLinkTemplates() */
+      continue;
+    }

Can we use $url->getParameters() and check for a key of $entity->getEntityTypeId() instead of using strpos?

Leksat’s picture

Leksat CreditAttribution: Leksat at Amazee Labs commented
FileSize
2939692-8.patch3.4 KB

@larowlan, thanks for the hint!
Updated the patch. 2939692-3--test-only.patch is still valid.

blazey’s picture

blazey CreditAttribution: blazey at Amazee Labs commented
Status: Needs review » Reviewed & tested by the community

The point has been addressed.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

Crediting @blazey for manual testing and reproducing and myself for suggesting improvement to approach.

  • larowlan committed 6029e8a on 8.6.x 
    Issue #2939692 by Leksat, blazey, larowlan:...

  • larowlan committed 1e5c50e on 8.5.x 
    Issue #2939692 by Leksat, blazey, larowlan:...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Version: 8.4.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed as 6029e8a and pushed to 8.6.x.

Cherry picked as 1e5c50e and pushed to 8.5.x.

  • larowlan committed 17ebfff on 8.4.x 
    Issue #2939692 by Leksat, blazey, larowlan:...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Version: 8.5.x-dev » 8.4.x-dev

As this is a critical - cherry-picked as 17ebfff and pushed to 8.4.x

cburschka’s picture

cburschka
they
CreditAttribution: cburschka at PwC's Experience Center commented

Duplicate of #2926897: Deleting content entities also deletes collection menu items., technically; closing the other one.

Status: Fixed » Closed (fixed)

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