[PP-1] Don't hide permissions local tasks on bundles when no permissions are defined

Created on 31 August 2023, 11 months ago
Updated 10 July 2024, 2 days ago

Problem/Motivation

EntityPermissionsRouteProviderWithCheck has significant performance overhead when checking menu link access.

Follow-up from 📌 Return early in EntityPermissionsForm::access if the user does not have "administer permissions" Fixed .

We could drop the check for whether there are permissions or not entirely from the access check, and just show a short message on the page when no permissions for a bundle are defined.

The UX regression of showing a somewhat useless tab, is better than the regression of multi-second page loads with admin_toolbar module and etc.

Steps to reproduce

Proposed resolution

Use EntityPermissionsRouteProvider and deprecate EntityPermissionsRouteProviderWithCheck.

Work-around

Until this issue is fixed, sites that are having performance issues with the access check can implement hook_entity_type_alter(). Change the route_provider from EntityPermissionsRouteProviderWithCheck to EntityPermissionsRouteProvider. See Comment #3306434-14: [Config] Warning: Entity view display 'node.article.default': Component 'comment' was disabled because its settings depend on removed dependencies. for a sample implementation, or 📌 Bypass slow access checks Needs review .

In Drupal core, the only bundle types that currently use EntityPermissionsRouteProviderWithCheck are Drupal\comment\Entity\CommentType and Drupal\contact\Entity\ContactForm.

Remaining tasks

  1. Decide whether to fix the problem in Drupal core by deprecating EntityPermissionsRouteProviderWithCheck or to fix it in the admin_toolbar module by implementing hook_entity_type_alter().
  2. Postponed on 🐛 [Config] Warning: Entity view display 'node.article.default': Component 'comment' was disabled because its settings depend on removed dependencies. Active .

User interface changes

In some cases, users with some administrative permissions will be shown links to configuration pages that have nothing to configure. Previously, links to these pages were not shown because access to those pages was denied.

API changes

Deprecate EntityPermissionsRouteProviderWithCheck.

Data model changes

None

Release notes snippet

N/A

🐛 Bug report
Status

Postponed

Version

11.0 🔥

Component
User module 

Last updated about 21 hours ago

Created by

🇬🇧United Kingdom catch

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

Merge Requests

Comments & Activities

  • Issue created by @catch
  • Status changed to Needs review 11 months ago
  • last update 11 months ago
    Custom Commands Failed
  • 🇬🇧United Kingdom catch

    Just to get things started.

  • Status changed to Needs work 11 months ago
  • 🇺🇸United States smustgrave

    Seems not having a return caused a CC failure.

  • 🇨🇭Switzerland Berdir Switzerland

    I think instead of this we should stop using EntityPermissionsRouteProvider and deprecate it entirely. in 11.x, only comment and contact still do that, block_content now always has permissions, so I think that most people who had this issue did so because they had a lot of block content types, and that's already gone in 11.x.

    The parent class already has the permission check, we just added that as a duplicate condition so we wouldn't run the extra access checks when we knew that access is going to be denied.

  • Status changed to Needs review 11 months ago
  • last update 11 months ago
    Custom Commands Failed
  • 🇬🇧United Kingdom catch

    Oh that makes more sense. Something like this.

  • 🇨🇭Switzerland Berdir Switzerland

    Yeah. I expect we should see some test fails with this which we have to update and then add an empty message to the permission table.

  • Status changed to Needs work 11 months ago
  • 🇺🇸United States smustgrave

    For test update.

  • Pipeline finished with Failed
    21 days ago
    Total: 168s
    #205501
  • Pipeline finished with Failed
    21 days ago
    Total: 180s
    #205502
  • 🇬🇧United Kingdom catch

    Promoting this to a bug after discussion on 🐛 Don't hide permissions local tasks on bundles when no permissions are defined Needs work , this is causing WSOD on some sites.

  • Pipeline finished with Failed
    21 days ago
    Total: 594s
    #205504
  • 🇺🇸United States bkosborne New Jersey, USA

    catch - you linked to this issue instead of what you intended to

  • Status changed to Needs review 18 days ago
  • 🇨🇭Switzerland Berdir Switzerland

    The issue that @catch wanted to link to is 🐛 [Config] Warning: Entity view display 'node.article.default': Component 'comment' was disabled because its settings depend on removed dependencies. Active .

    I added an empty message to the page and updated to test to check for that instead of an access denied response.

    I also updated the call in \Drupal\user\Form\EntityPermissionsForm::permissionsByProvider() to use findConfigEntityDependenciesAsEntities() instead of getConfigEntitiesToChangeOnDependencyRemoval(), which would likely already fix that other issue, but it's IMHO still too heavy as an access operation that's run on possibly dozens of links when using admin_toolbar.

  • Pipeline finished with Failed
    18 days ago
    Total: 517s
    #207199
  • 🇨🇭Switzerland Berdir Switzerland

    deprecation message versions will need to be updated to 11.0/12.0 I guess. Leaving that for someone else while I get some sleep.

    This is an interesting one as the other issue is currently critical and this is kind of the only fix for that, but it introduces deprecations and new UI text as well. The deprecation we could in theory just drop for a 10.3 commit, the empty message seems useful enough to have, although it possibly could be improved a bit.

    I don't fully understand what exactly changed in 10.3, this already was an issue back in 9.4 when this stuff was originally added.

  • Pipeline finished with Success
    18 days ago
    Total: 680s
    #207204
  • 🇺🇸United States smustgrave

    As a bug could the issue summary be filled out some please

  • 🇸🇮Slovenia KlemenDEV

    Patch #5 fixes the problem described in https://www.drupal.org/project/drupal/issues/3306434 🐛 [Config] Warning: Entity view display 'node.article.default': Component 'comment' was disabled because its settings depend on removed dependencies. Active that started happening when updating from 10.2 to 10.3

  • 🇺🇸United States jlsegul

    When trying to apply the patch 3384600-5.patch, it errors out with "can't find file to patch at input line 14". Any idea why?

  • Status changed to Needs work 15 days ago
  • 🇺🇸United States smustgrave

    Left a comment in the MR.

  • First commit to issue fork.
  • Pipeline finished with Canceled
    13 days ago
    Total: 515s
    #212196
  • Pipeline finished with Success
    13 days ago
    Total: 476s
    #212203
  • 🇳🇱Netherlands Spokje

    - Updated deprecated/removed version numbers
    - Added `@` to `trigger_error`, which, AFAICT, is still the default for `E_USER_DEPRECATED`.
    - Added `E_USER_DEPRECATED` to `@trigger_error` in `\Drupal\user\Form\EntityPermissionsForm::access`

  • Pipeline finished with Success
    13 days ago
    Total: 618s
    #212213
  • Status changed to Needs review 13 days ago
  • Status changed to RTBC 12 days ago
  • 🇺🇸United States smustgrave

    Summary appears to be updated so removing that tag.

    Also appears all feedback has been addressed so believe this one is good.

  • 🇺🇸United States benjifisher Boston area

    I do not see how the two issues are related, but 🐛 [Config] Warning: Entity view display 'node.article.default': Component 'comment' was disabled because its settings depend on removed dependencies. Active is mentioned here (Comments #11, #12, #14), so I am adding it as a related issue. If I read the comments correctly, that issue will be fixed by the change here.

    I am fixing a typo ("Provicer" should be "Provider") here and in the change record (CR). I am also correcting the title of the CR.

    I am also filling out the "User interface changes" and "API changes" sections of the issue summary. If that is enough, then we can remove the "Needs issue summary update" issue tag.

    I am also adding a work-around to the issue summary. Has anyone considered adding that work-around to the admin_toolbar module instead of removing the EntityPermissionsRouteProviderWithCheck class from core? Are there problems from this route provider on sites that do not use admin_toolbar?

  • Status changed to Needs work 12 days ago
  • 🇺🇸United States benjifisher Boston area

    I think that replacing getConfigEntitiesToChangeOnDependencyRemoval() with findConfigEntityDependenciesAsEntities() is a good idea, but I do not see how it is in scope for this issue.

    If that change is in scope, then it should be explained under "Proposed resolution" in the issue summary.

    I think it is more likely that the change is out of scope. In fact, according to Comment #11, that change is likely to fix 🐛 [Config] Warning: Entity view display 'node.article.default': Component 'comment' was disabled because its settings depend on removed dependencies. Active . (I did a quick check, and this seems correct.) If so, then let's make the change on that issue and get it into the next patch release. This issue introduces a deprecation, so it cannot be made until the next minor release.

    In my previous comment, I suggested that, instead of deprecating EntityPermissionsRouteProviderWithCheck in Drupal core, we should implement hook_entity_type_alter() in the admin_toolbar module. I am adding an item to "Remaining tasks" to decide which approach to use.

  • 🇺🇸United States benjifisher Boston area
  • 🇨🇭Switzerland Berdir Switzerland

    > In my previous comment, I suggested that, instead of deprecating EntityPermissionsRouteProviderWithCheck in Drupal core, we should implement hook_entity_type_alter() in the admin_toolbar module. I am adding an item to "Remaining tasks" to decide which approach to use.

    I don't see why admin_toolbar is supposed to implement a workaround for core. The problem is in core, admin_toolbar doesn't cause it, it just exposes it more, but it's already there.

    And yes, the warning from the other issue also happens when you are on any page that displays the local task and even with the access check removed, still happens on the manage permissions page itself.

    > I think that replacing getConfigEntitiesToChangeOnDependencyRemoval() with findConfigEntityDependenciesAsEntities() is a good idea, but I do not see how it is in scope for this issue.

    It can be argued, but the reason we remove the access check is performance and the side effects by calling getConfigEntitiesToChangeOnDependencyRemoval(). But it's not the access check that's so slow, it's the method that is called by the access check. Just deprecating the access check alone still results in a slow and expensive manage permissions page that has side effects. And inverted, findConfigEntityDependenciesAsEntities() is slightly better but it's still a slow and memory heavy operation. There's no query we can run to find this information. Drupal has to load every single config object in the system into memory and loop over them.

    So, only both changes together really solve the problem (although not 100%, I'd like to not have to depend on config dependencies for this at all, but we'd need to introduce an new API for this)

  • Pipeline finished with Success
    8 days ago
    Total: 832s
    #216541
  • Status changed to Needs review 8 days ago
  • 🇳🇱Netherlands Spokje

    Thanks @benjifisher for the (very clear) rewrite of the IS and @berdir for explaining why the current solution is what it is, and might/could/should be the way forward.

    Putting this up for review once more, so this major issue won't get stalled/lost-in-limbo.

  • 🇺🇸United States benjifisher Boston area

    It looks as though we will replace the call to getConfigEntitiesToChangeOnDependencyRemoval() with findConfigEntityDependenciesAsEntities() or findConfigEntityDependencies() in 🐛 [Config] Warning: Entity view display 'node.article.default': Component 'comment' was disabled because its settings depend on removed dependencies. Active . If so, then we will have to update (simplify) the MR for this issue.

    Since this issue introduces a deprecation, it might not be eligible for a patch release. As a practical matter, I think it makes sense to implement hook_entity_type_alter() in the admin_toolbar module. I opened 📌 Bypass slow access checks Needs review to do that, with a variant of @larowlan's code sample in #3306434-14: [Config] Warning: Entity view display 'node.article.default': Component 'comment' was disabled because its settings depend on removed dependencies. .

    The issue summary here mentions "multi-second page loads with admin_toolbar module and etc.", and Comment #9 says, "this is causing WSOD on some sites." I have not seen that. Can someone provide steps to reproduce?

    +1 for the technical analysis in Comment #25. Unless we add a new API, the only reliable way to get the permissions related to a bundle is to use the config dependency system, and that is inherently slow.

    I will be disappointed if we give up the per-bundle permissions forms that we added in Drupal 9.4. At that time, it seemed like a good idea not to show links to forms with no permissions, and access control is the only way I could think of to implement that. I will not try to convince anyone not to remove those checks (this issue) but I will not actively support it until I know how to reproduce the serious problems. (I will also suggest alternative fixes.)

  • 🇬🇧United Kingdom catch

    @benjifisher see #3306434-21: [Config] Warning: Entity view display 'node.article.default': Component 'comment' was disabled because its settings depend on removed dependencies. #3306434-22: [Config] Warning: Entity view display 'node.article.default': Component 'comment' was disabled because its settings depend on removed dependencies. #3306434-37: [Config] Warning: Entity view display 'node.article.default': Component 'comment' was disabled because its settings depend on removed dependencies. for reports of performance problems.

    Also @berdir above:

    it's still a slow and memory heavy operation. There's no query we can run to find this information. Drupal has to load every single config object in the system into memory and loop over them.

    I've seen sites with over 1500 config entities, very easy for this to take a second or more to do.

    I think we could backport this to 10.3.x without the deprecation (the string addition seems OK - it'll be on a page that no-one has seen yet and is unlikely to visit compared to other string changes we could make).

  • 🇺🇸United States benjifisher Boston area

    I wrote,

    Since this issue introduces a deprecation, it might not be eligible for a patch release.

    On second thought, we might deprecate the slow route provider in Drupal 11.x and 10.4.x, then have a separate MR for 10.3.x that does not include the deprecation but does switch the route provider for comment_type and contact_form.

  • Merge request !8694Resolve #3384600 "10.3.x " → (Open) created by Spokje
  • Pipeline finished with Success
    5 days ago
    Total: 656s
    #218585
  • 🇳🇱Netherlands Spokje

    Added a 10.3.x MR, which is basically the diff from the 11.x MR, without the deprecations.

  • blueBriX's value based care solutions focuses on improving patient outcomes while reducing healthcare costs. It leverages data analytics, patient engagement tools, and care coordination to enhance the quality of care. This approach ensures efficient resource utilization, better patient satisfaction, and overall improved health outcomes.

  • 🇺🇸United States benjifisher Boston area

    I am restoring the issue summary. It seems that this issue was updated by a spammer, whose account is already blocked.

    @catch, thanks for the references in Comment #28.

    If we agree to handle the error message in #3306434, then we can update the MRs for this issue and they can be fixed in either order.

  • Status changed to RTBC 3 days ago
  • 🇬🇧United Kingdom catch

    Both MRs look good to me and I can't see anything else to do here.

  • 🇨🇭Switzerland Berdir Switzerland

    This issue overlaps with 🐛 [Config] Warning: Entity view display 'node.article.default': Component 'comment' was disabled because its settings depend on removed dependencies. Active now, which is making the same change, with test coverage, that test coverage was deliberately written that it will need to be adjusted when this issue lands.

    I think the other issue should be committed first at this point and then this rerolled.

    Or, remove the findConfigEntityDependenciesAsEntities() change from this completely, then we _could_ also land this first, but it will only partially address the problem that many people are currently reporting in the other issue (it will still happen, but only when you actually visit the manage permissions page).

  • Status changed to Postponed 3 days ago
  • 🇬🇧United Kingdom catch

    Ah that is fair enough, let's explicitly postpone this then.

  • 🇺🇸United States benjifisher Boston area
Production build 0.69.0 2024