Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.This is a regression introduced by #2350797: Orphaned menu links when nodes are deleted if menu_link_ui is not enabled.
Problem
To reproduce:
1. Create a custom menu link that points at /admin/people
2. Create a user.
3. Delete (not disable) the user.
4. The custom menu link is deleted.
This appears to affect all cases where the custom menu link points at a route which is exposed as a link template of the deleted content entity, regardless of whether that link template is actually parameterised to that specific entity.
(Note that it doesn't happen with /node, even if you install the rest/hal/basic_auth modules. The link template "create" does point at "/node", but a menu link pointing at "/node" is resolved to view.frontpage.page_1 rather than entity.node.create due to the format restriction. Therefore $node->uriRelationships() will not pick it up and it will not be deleted. It is likely that if the Node entity were to expose `/admin/content` as a collection link template in the same manner as the User entity does, custom /admin/content menu links would also be deleted on node deletion.)
Proposed resolution
In menu_link_content_content_entity_predelete, check that the route parameters of the link actually include the entity. If they don't, it's a static link template (eg. collection) and should not be deleted.
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | drupal-2926897-2.patch | 1002 bytes | cburschka |
| #6 | drupal-2926897-6.patch | 1.04 KB | cburschka |
| #6 | drupal-2926897-6-interdiff.txt | 705 bytes | cburschka |
| #7 | drupal-2926897-7-testonly.patch | 1.37 KB | cburschka |
| #7 | drupal-2926897-7.patch | 2.41 KB | cburschka |
Comments
Comment #2
cburschkaNot sure about this one. It assumes that the parameter for the entity is named after the entity type, which is usually correct, but can be overridden in the route definition.
If we want to be completely correct/paranoid about this, we would need to load and inspect the actual route definition, check if any of its parameters are upcast to the entity in question, and then check if the value of that parameter actually matches the entity we're deleting.
Also needs test.
Comment #3
tstoecklerWow, this is a very interesting one. What your patch doesn't catch is revision routes (assuming they don't also contain the base entity ID). Maybe we should add a check for a
$entity_type_id . '_revision'parameter, as well?Comment #4
cburschkaFrom what I can tell, revision-specific routes can't actually be handled by the code currently because the revision parameter is not provided anywhere. Without that parameter, we likely can't find any menu links.
(In fact, revision-specific link templates currently cause a crash because Entity::uriRelationships() doesn't catch the MissingMandatoryParameterException, which is a separate bug: #2924338: Entity::uriRelationships() throws exceptions if an URL cannot be generated because of missing mandatory parameters.)
Comment #5
cburschkaWait, I missed a special case - the
revisionrelationship itself automatically gets the revision parameter set by the entity. So I definitely should be handling that case in the patch, yes.The problem I described just affects extra relationships like revision-delete or revision-revert (which hopefully shouldn't have any menu links pointing at them anyway).
Comment #6
cburschkaComment #7
cburschkaThe test is actually just a few extra lines in the existing test function for the original issue.
Comment #10
cburschkaFixed by #2939692: menu_link_content_entity_predelete deletes "collection", "add-form", etc. links