Compatibility with other modules (custom, contrib or even core)

Created on 31 August 2018, about 6 years ago
Updated 25 July 2024, about 1 month ago

First of all, thank you for this useful module! :)

My problem: I wanted to implement hook_menu_link_content_access() in a custom module to restrict delete access to a certain menu link. I could not get my hook to be called. Here's why.

RouteSubscriber::alterRoutes() uses $route->setRequirements(). It replaces all the requirements set by core. So that the _entity_access requirement is removed, then hook_entity_access() and hook_ENTITY_TYPE_access() are not invoked.

It looks like a bad thing to me because it prevents any module from further altering the access.

I partly understand why setRequirements() was used: route requirements combine in a "andIf()" logic, so that all requirements must return "Allow" for the route access to be allowed. A simple setRequirement() would just add a new requirement, which could restrict the access but not widen it. And the aim of this module is precisely to widen the access to people who normally have no access (people who don't have the global "administer menus" permission).

One way to fix that would be to call back the overridden access checks inside our custom one (make the _entity_access inside the _custom_access one. But that would need to know in the custom check which previous checks were overridden. We know them for the core in the current state, but it could change, and contrib modules could add other checks.

Let's see if we can do better. Here's an overview of the overridden core requirements:

The three custom access checks (::menuAccess() , ::menuItemAccess() and ::menuLinkAccess()) simply return "Allow" if the user has the global "administer menu" permission or the administer permission for the given menu.

When _entity_access is the only overridden requirement, a best way to do is to leverage the existing _entity_access check to deny access if needed, through a hook_ENTITY_TYPE_access() implementation.
The good news is that it's already done for menu_link_content routes in menu_admin_per_menu_menu_link_content_access() so that the overrides #6 and #7 are already useless.
I made a quick look, and there doesn't seem to be such a hook for menu entities, so that #2 seems strange: it gives the user access to a form to edit an entity that he actually may not be able to edit. I did not check, but I guess the user won't be able to save the form if he doesn't have the permission to administer all menus. Same reasoning for #3. The missing hook implementation should be added.

For #9, #10 and #11, the override of _access_content_translation_manage removes the check for translation permission coming from the Content Translation module, which seems wrong to me. Shouldn't we simply rely on the existing requirements, as for #6 and #7? And the same problem also applies to #8. But this might change the access given on existing websites already using the module. A new major version might be needed?

Finally, I have to admit that the overrides #1, #4 and #5 are needed, I can't see another way to widen the access. But I would only replace _permission: 'administer menu', so that the linkIsResettable check stay in #5.

Here's a patch that does all of this. I hope I was clear enough and did not make any mistake...

✨ Feature request
Status

Needs review

Version

1.0

Component

Code

Created by

🇫🇷France GaëlG Lille, France

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

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • First commit to issue fork.
  • 🇧🇪Belgium DieterHolvoet Brussels

    Started a MR based on 2996548-menu_admin_per_menu_compatibility-12.patch.

  • Pipeline finished with Failed
    about 2 months ago
    Total: 174s
    #233147
  • Pipeline finished with Failed
    about 1 month ago
    Total: 165s
    #233661
  • Status changed to Needs review about 1 month ago
  • 🇧🇪Belgium DieterHolvoet Brussels

    I pushed a new approach where the old route requirements are stored in a route option, and during the access check these requirements are also checked and OR'ed with the checks of this module. This abstract approach should fix most issues.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 167s
    #234598
Production build 0.71.5 2024