Drupal should warn if your node delete will cause menu item re-parenting

I have added some logic that tests whether the child links are present for the link that we are deleting. If we have child elements then it shows a message(may not be correctly defined now) that this link contains child links which will be effected due to the deletion of the parent link.I am attaching the screenshots as a kickstart to this.

We can similarly do this after the successful deletion of the link as well as described in the issue.

@Utkarsh_33
This isn't about when menu items are deleted. It's about when nodes (or I guess other entities too) within the menu are deleted.

joaopauloc.dev → made their first commit to this issue’s fork.

Hi folks, I have two questions.

First, the warning message will be displayed only for a node that has a menu item as a parent? or should we warn users since the content has any kind of reference in menus? If yes I need to update the implementation.

In Drupal 11 the menu link is also deleted if the content is deleted. We need to update the message, I thought something like

This page has 3 menu children. These menu links will be deleted too if you continue.

Thanks.

@joaopauloc.dev
I like how you moved to ContentEntityDeleteForm.php so that this will apply to all content, not just nodes. But I'm not sure that getQuestion() is the right place for this. There's several returns in that function, and you've just added one more. The original MR used buildForm(), and that might be a better choice. Especially since we want to also use a list.

First, the warning message will be displayed only for a node that has a menu item as a parent? or should we warn users since the content has any kind of reference in menus?

Only warn when an entity is being deleted, and that entity has a menu item, and that menu item has child menu items.

In Drupal 11 the menu link is also deleted if the content is deleted. We need to update the message, I thought something like: This page has 3 menu children. These menu links will be deleted too if you continue.

Yes, the menu item being deleted is what causes the problem that this ticket is trying to solve. But are you saying that in D11 the _children_ menu items are also blindly deleted??? I can't get https://simplytest.me/ to build D11 so that I can confirm. But if that's the case, then that's a separate critical issue of content destruction. I'm guessing that's _not_ happening.

I've updated the ticket description to have clearer messaging. Can you take a look?

Thank you @dalin for the feedback.

I'll adjust the menu to warn only on the criteria that you mentioned. Also, fix the getQuestion things that you mentioned.

Regarding the menu item also been deleted I'll confirm one more time but I'm almost sure that was deleted too.

I'll work on this issue until this weekend and let you know.
thanks.

Hey @dalin, I could confirm that only the menu item of the content was deleted.
Working on the issue...

Updated the implementation to show the warning message before and after the content is deleted.
Missing unit tests and fixing probably unit test issues.

The MR 5807 applied cleanly. The patch working as expected. Attached screenshot of manual testing.

What will happe, if there will be hundreds of child menu items? I suppose this will not look good. Do we need to print the child menu items?

I think we need to add a test for this new feature, so that we can confirm that the extra message is here, when the menu item has children and that the message is not there, when deleting a menu item without child items. Thanks!

Working on this recently.

I took a look at the existing MR5807, and found some issues in my review:

• The moderation state change looks irrelevant.
• The changes here should work with any entity, not just nodes.
• Shouldnt get menu items by menu link content entity to get descendants, via entity queries.
• The menu renders lists with HTML in t-strings, Markup elements, etc.

Rather than add feedback to the MR, I've started from scratch as I dont think the code is usable.

Notes on the new implementation at MR6479:

• Uses a similar method to `menu_ui_get_menu_link_defaults()` to determine the menu link of the entity in context.
• Renders with the standard menu builder. Which uses a proper list element instead of markup
• Reworded warning message, use singular labels instead of _content_
• Used formatplural, depending on number of child menu items.
• Created a internal class service, without service id. Instead of using the old classResolver pattern.
• In line with hooks, the new form alter is not overridable:
  • the class been finalised.
  • no service ID.
  • This was chosen as it's possible to easily unfinalise or add a service ID in the future (new issue) if we decide thats needed. Doing the reverse is not possible without dealing with backwards compatibility and deprecations.
• update the used `getTranslationFromContext` method with a generic, so PHPStan knows its @return-ing the same entity type as is passed into the parameter.
• Test coverage.

Screenshots

Single menu item child

Multiple menu item children

Message after delete:

Re: @poker10 feedback in #17,

> What will happen, if there will be hundreds of child menu items?

The message will only return the direct children. Not multiple levels down.

> I think we need to add a test for this new feature

Added. I think the tests are comprehensive.

Added notes to review. Remains _Needs review_.

Removed "node" references from issue summary.

I've discovered that this also includes a menu entity delete. Updating the description accordingly. I updated the remaining tasks.

IMO we don't need to do anything special if there are lots (a hundred?) direct children. The list will be long. So be it. But this will almost never happen in real life.

IMO we should still show something for menu items that the user doesn't have access to. They should be included in the total count, but in the list we should show "access denied" or something similar.

I've discovered that this also includes a menu entity delete.

Agree this is a case worth covering, updated the MR and tests to acommodate Menu Link Content entities.

we don't need to do anything special if there are lots (a hundred?) direct children. The list will be long. So be it. But this will almost never happen in real life.

I think it'd be more common than "almost never", but I think since the menu system is already capable of rendering the menu items to the front end in runtime, then it will also be fine for backend tasks.

Nothing further required for the item.

IMO we should still show something for menu items that the user doesn't have access to. They should be included in the total count, but in the list we should show "access denied" or something similar.

I disagree, I dont think theres a case in Drupal where we inform the user there is some secret data the user doesnt know about but is affected by an operation, except when permission to do the operation straight up denies the user.

To me, only exposing data relevant to the user is sufficient.

----

Changes in MR.

Seems next remaining task is to agree on wording. UX team should have a say in that.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

samit.310@gmail.com → made their first commit to this issue’s fork.

Fix conflicts and chnages in test file with D11 compatibility.

Thanks
Samit K.

joaopauloc.dev → closed merge request !5807

After rebasing merge request (MR), it seems that PHPStan is flagging issues in StringTranslationTrait.php .