- Issue created by @josebc
- Status changed to Postponed: needs info
about 1 year ago 3:53pm 2 November 2023 - π·π΄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 3:56pm 2 November 2023 - π·π΄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 8:05pm 15 November 2023 - πΈπͺ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 11:02am 15 December 2023 - π·π΄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 displayedLike @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?