Warn user when entity 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.

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.

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.

Thanks for working on this. What will happen, if there will be hundreds of child menu items? I suppose this will not look good. Do we need to output the child menu items in the message?

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.

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.

Fix conflicts and chnages in test file with D11 compatibility.

Thanks
Samit K.

After rebasing MR, it seems that PHPStan is flagging issues in StringTranslationTrait.php .

I have checked and fixed the pipeline failure, and rebased the MR . Also checked the last feedback is already addressed, so moving this Back to NR. Kindly review.

We discussed this issue at #3387665: Warn user when entity delete will cause menu item re-parenting. That issue will have a link to a recording of the meeting.

Thanks for working on this issue. At least one attendee has run into exactly this problem, and there were a lot of child menu items involved.

For the record, the attendees at the usability meeting were @benjifisher, @rkoller, @simohell, and @worldlinemine.

When you tag an issue for usability review, please make it easy for the usability team to review the issue. Update the issue summary:

• The "Proposed resolution" section should describe all the changes made in the issue.
• The "User interface changes" should show the existing UI and the proposed UI.
• The "Remaining tasks" should include one explaining the usability issue(s).

Most of the time, I prefer to have plain text in the "Proposed resolution" section and screenshots in the "User interface changes" section.

You can also attend the weekly usability meeting to present an issue.

Usability review

We have a few recommendations:

1. Shorten the text in the confirmation form.
2. Suggest changing the parent of the menu item before deleting it.
3. Special treatment when deleting custom menu links.
4. Follow-up issue: provide an option to delete the child menu items.

Shorten the text

There is room for improvement, but this is the best we could do during the meeting. After the standard "This action cannot be undone.", either

Deleting this [content item] will make these [count] child menu items top level:
...

or

Deleting this [content item] will move these [count] child menu items under [parent link]:
...

Make adjustments for singular or plural, as the current MR already does.

Suggest changing the parent

A good status message starts by describing the situation, but then it suggests what to do about it. In this case, a common action will be to change the parent of the menu item that is going to be deleted: then, after deletion, all the child menu items will be children of the new parent.

Changing the parent item can be done from the menu page (where all the items in the menu are listed, with a click-and-drag interface), from the edit page for the individual menu link (where the parent can be selected from a list) or from the edit page of the content item. We feel that the click-and-drag interface is problematic. There might be permission issues with editing the menu item, but that gives the most flexibility.

If the user is deleting a content entity (not a custom menu link ... see the next point) and the suggestion is to edit the related menu link, then it would help to give a link to that edit page.

We do not have suggested text, but this information can go after the list of affected child items.

Special treatment when deleting custom menu links

Custom menu links are content items, so the warning gets added to the confirmation form when deleting a custom menu link. So far, so good! But the text on the current MR, when deleting a custom menu link, includes "The menu items for this custom menu link", which does not make sense. Pay attention to the case where the content item being deleted is a custom menu link.

With the text proposed earlier, that entire sentence is removed, so maybe we do not have to worry about it. But keep this case in mind while continuing to work on this issue.

Follow-up issue

Sometimes, the user will want to delete the child menu items instead of move them to a different parent. There are a lot of things to be considered here, so we recommend adding a follow-up issue to discuss how to implement this. Maybe the right thing to do is turn the menu page into a form that allows bulk operations. Maybe there should be an option on the confirmation form, or the edit form for a custom menu link, to delete children. Should that include all descendants? How can we make it clear whether we are deleting content entities or their related custom menu links?

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

Usability review

We discussed this issue at #3420206: Drupal Usability Meeting 2024-02-16. The link to the recording of the meeting is: https://youtu.be/GeLGUAqrMTQ

For the record, the attendees at the usability meeting were @AaronMcHale, @benjifisher, @rkoller, @simohell, and @worldlinemine.

During followup discussions for the usability meeting on friday, we’ve realized that we had already discussed this issue a few months ago in february; unfortunately, it slipped through, and we haven’t had posted any summary yet. Apologies. :(

But the two reviews complement each other quite well, because back then we haven’t had the time to get into the whole wordsmithing part. And as a disclaimer, I haven’t rewatched the entire recording, but just focused on close to the end, where we usually summarize our conclusions. So in addition to the points @benjifisher summarized in #3387665-37: Warn user when entity delete will cause menu item re-parenting the following aspects should be considered:

In the examples and screenshots shared on this issue, the number of listed children on the confirmation dialoge modal and the warning message is rather small. But one of the attendees raised the concern, based on his own experiences, that the odds are high to have a very large set of menu items with many children and grandchildren. By providing a list of menu items, that list will become unwieldy fast, if all affected menu items are being listed. Providing some logic that displays a list with equal or less than for example three items, while hiding the list with more than three items might overcomplicate things. So the recommended approach we had a consensus on was to only to provide a summary on the confirmation dialog modal and the warning message (micro copy wise the proposal from the “shorten the text” section in #3387665-37: Warn user when entity delete will cause menu item re-parenting would work), the URLs should be moved to a log message. That log message should be created containing the list of affected URLs, so the information about the made change is available persistently for easier recovery, and the retracing of the applied steps, if necessary.

And it was noted that the following scenario is currently no covered by the MR yet - if a node is being deleted and that node is contained in more than one menu, the user is currently not being informed about that fact. Should that detail be moved to a followup issue or already be worked on within the scope of this issue?

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

Hi folks.
I updated the issue summary adding a screenshot with the User interface changes as recommended in comment #37.
Also, when I tested the MR I found the following issue.

Finally, update the modal message with the comments suggested in comment #37. Regarding this message, I would like to suggest another one when there is only one item, for the singular message we could use something like:
Deleting this @singular_label will move its child menu item to the top level:
What do you guys think?

Hi @benjifisher and @rkoller, thanks for reviewing this issue.

These are my suggestions for the points Suggest changing the parent, Special treatment when deleting custom menu links.
For the Suggest changing the parent

The link will be opened in another tab. What do you guys think?

Regarding the Special treatment when deleting custom menu links
I changed the text message as you guys suggested.

So I went into test

Drupal 11.x
Added an article under Main navigation (Article 1)
Added an article under the one I just created. (Article 1.1)
Went to delete Article 1
I never received a warning or anything.

Hey @smustgrave.
Thank you for reviewing this one.
I followed your steps, and I could see the warning messages.
Do you remember where you were when you tried to remove the node?
Follow the images.

Node page

Delete from node edit page

Delete action on content list page

All the messages above also appear on the menu delete page/action.

Yea I did not get that. But can retest in the morning

Since we have a new form alter hook class https://www.drupal.org/node/3499495
I changed the attribute class on MenuUiEntityDeleteFormHooks to use the FormAlter instead of Hook class.

Appears to need a rebase.

Can we look into and potentially close #2309531: Add a test for re-parenting of menu links. if there is sufficient overlap.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.

Tested the MR on local and observed as below. Looks like MR is working as expected. Good for RTBC.

Without Patch:

With Patch(on MR):

Menu Structure - Before delete:

Warning for the Menu delete:

Success message - After delete:

Menu Structure - After delete:

Still 3 open threads can those be closed?

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.

I followed the Steps to reproduce recommendations, and the solutions seem to work correctly.

Drupal core 11.x-dev

Before the patch:

After applying the patch:

I'm checking all RTBC issues to ensure their new tests use attributes. Because these changes are minor and the tests are passing I'm going to leave the issue at RTBC.

@arunkumark and @eric.vvf thanks for testing. To set an issue to RTBC it also needs a complete review which includes a code review and to confirm that the change passes the Core gates.

On reading the comments I see that there has been no response to #53, which is to review the open threads in the MR to decide if they can be closed. I also made a comment on the MR, the out of scope change should be remove.

There are several items in the remaining tasks that are open. Have they been resolved.?

I did not thoroughly review the 2 Usability reviews here to confirm that the MR implements the recommendations.

I updated credit.

Setting to NW.

In a Slack conversation about credit email I learned nicxvan received an email saying they were credited but that is not reflected in the contrib record now. Perhaps I added them and then removed then. I don't know. In any case adding them now.

As a reminder, the person committing will review credit.

Gave a high level overview and noted a couple things that need to be addressed for performance reasons.

All comments addressed.

appears to be feedback on the MR. Resolved 2 threads though.