Custom menu links are not updated when a node is soft-deleted

Created on 2 November 2023, about 1 year ago
Updated 8 April 2024, 7 months ago

Problem/Motivation

TrashStorageTrait delete method does not trigger pre and post delete hooks resulting in unexpected behaviors like 403 menu links.

Steps to reproduce

  • Install and configure module as normal.
  • Create a node of any type that is allowed in a menu and set " Provide a menu link".
  • Delete the node (send to trash).
  • Menu link will show linkin to the 403 page.

Proposed resolution

Change TrashStorageTrait delete method to mimic EntityStorageBase.

πŸ› Bug report
Status

Postponed: needs info

Version

3.0

Component

Code

Created by

πŸ‡―πŸ‡΄Jordan josebc

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @josebc
  • Status changed to Postponed: needs info about 1 year ago
  • πŸ‡·πŸ‡΄Romania amateescu

    Not triggering the delete hooks is by design, I tried this at some point and it created way more problems than it solved.

    The issue with menu links should be handled by trash_menu_link_content_update(). Are you using the latest version of the module?

  • Status changed to Active about 1 year ago
  • πŸ‡·πŸ‡΄Romania amateescu

    Oh I misread the problem you pointed out, it's about deleting nodes, not menu links.

    Anyway, I still think that we shouldn't trigger the delete hooks, but handle it in a similar way with how we deal with menu_link_content deletions.

  • πŸ‡―πŸ‡΄Jordan josebc

    That is a good point, but there will always be a need to reacting to content being sent to trash. Maybe we can implement a new hook/event listener (if it doesn't exist already) that is triggered when an entity is sent to trash, that way developers can choose what to do.

  • πŸ‡·πŸ‡΄Romania amateescu

    Right, invoking a hook would be good, and we already have an issue open for it: ✨ Call hook during entity restore Needs review

  • πŸ‡·πŸ‡΄Romania amateescu

    ✨ Call hook during entity restore Needs review now handles all the hooks needed, let's keep this issue open to fix the problem with menu_link_content entities related to deleted nodes.

  • πŸ‡ͺπŸ‡ΈSpain penyaskito Seville πŸ’ƒ, Spain πŸ‡ͺπŸ‡Έ, UTC+2 πŸ‡ͺπŸ‡Ί

    While working on this thought it would be cool to also restore the menu_links if they were also in trash when the related node was restored.

    But somehow

        $deletedLinks = [];
        $trashManager->executeInTrashContext('inactive', function () use ($menu_link_manager, $url,$route_parameters, &$deletedLinks) {
          $deletedLinks = $menu_link_manager->loadLinksByRoute($url->getRouteName(), $route_parameters);
        });
    
    

    didn't work as I expected and $deletedLinks comes empty. I debugged this and it is because `trash_menu_link_content_update` removes the definition, so `menu_tree` doesn't contain the link anymore. We might want to review this and provide this functionality in a follow-up.

  • @penyaskito opened merge request.
  • Status changed to Needs review 12 months ago
  • πŸ‡ͺπŸ‡ΈSpain penyaskito Seville πŸ’ƒ, Spain πŸ‡ͺπŸ‡Έ, UTC+2 πŸ‡ͺπŸ‡Ί
  • πŸ‡ΈπŸ‡ͺSweden twod Sweden

    Isn't the real issue here that the menu link is still displayed even though it points to an inaccessible URL?

  • Status changed to Postponed: needs info 11 months ago
  • πŸ‡·πŸ‡΄Romania amateescu

    Isn't the real issue here that the menu link is still displayed even though it points to an inaccessible URL?

    That's exactly the problem, and it's actually a core issue: πŸ› [regression] Do not bypass route access with 'link to any page' permissions for menu links Fixed

    I deleted a node with a menu link, then checked the following:
    - with the admin user, the menu link was still displayed
    - with the anonymous user, the menu link was not displayed

    Like @penyaskito mentioned in the MR, I doubt that deleting the associated menu link when a node is trashed is the right thing to do. There would be no way of restoring that node (if you're not using the `wse_menu` module).

    There's also the option of unpublishing the menu link when the node is trashed, but that also has the problem that we can't know if we should publish it when the node is restored.

    I'm inclined to close this as a duplicate of the core issue, any objections? :)

  • πŸ‡ͺπŸ‡ΈSpain penyaskito Seville πŸ’ƒ, Spain πŸ‡ͺπŸ‡Έ, UTC+2 πŸ‡ͺπŸ‡Ί

    I wasn't aware of that regression, thanks for pointing it!

    I've been using the PR patch on prod. Until the core issue is fixed, from an UX perspective I think it's better for end users to have to rethink the menus when they restore content than having to think about removing it from every menu it might be used when deleting something.

    So this might serve as a temp solution and we create a follow-up blocked on the core issue?

  • Just implemented this module and found the above issue to be problematic. Thanks for the MR patch until it can be ultimately solved.

    I would agree with @penyaskito in that asking our users to redo their menu structure is a better ask than not having the menu structures work at all.

  • πŸ‡·πŸ‡΄Romania amateescu

    @catapipper, since πŸ› [regression] Do not bypass route access with 'link to any page' permissions for menu links Fixed was committed to Drupal 10.3, can you please test with that patch applied instead of the one from this issue and check if it fixes your problem?

Production build 0.71.5 2024