[backport] Don't hide permissions local tasks on bundles when no permissions are defined

Seems not having a return caused a CC failure.

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.

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.

For test update.

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

The issue that @catch wanted to link to is #3306434: Fix access checks for bundle permissions to avoid triggering a config validation error.

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.

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.

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

Patch #5 fixes the problem described in https://www.drupal.org/project/drupal/issues/3306434 that started happening when updating from 10.2 to 10.3

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?

Left a comment in the MR.

- 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`

Summary appears to be updated so removing that tag.

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

I do not see how the two issues are related, but #3306434: Fix access checks for bundle permissions to avoid triggering a config validation error 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?

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 #3306434: Fix access checks for bundle permissions to avoid triggering a config validation error. (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.

> 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)

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.

It looks as though we will replace the call to getConfigEntitiesToChangeOnDependencyRemoval() with findConfigEntityDependenciesAsEntities() or findConfigEntityDependencies() in #3306434: Fix access checks for bundle permissions to avoid triggering a config validation error. 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 #3459723: Bypass slow access checks to do that, with a variant of @larowlan's code sample in #3306434-14: Fix access checks for bundle permissions to avoid triggering a config validation error.

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.)

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.

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

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.

This issue overlaps with #3306434: Fix access checks for bundle permissions to avoid triggering a config validation error 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).

Now that #3306434: Fix access checks for bundle permissions to avoid triggering a config validation error is fixed, we can un-postpone this issue.

We need remove the changes in the MRs that were already made by #3306434. I left comments on the MRs.

I am curious how much #3306434 helps the performance, if at all. Should this issue still be prioritized as Major?

Rebased. To answer #37, we'll need to profile this on a real-world site with lots of contact forms and also lots of other configuration, possibly both before/after the other issue.

Doing a test on our install profile with about 1300 config objects and still on 10.2, without either patch, testing on the edit page of a contact form, a single call to getConfigEntitiesToChangeOnDependencyRemoval() is about 100ms/17% and 16MB memory. With the patch applied from the other issue, it's reduced to 50ms/10%/15MB. Memory isn't changed much because it still needs to load all config objects. That's about 35% of total memory usage.

Better, but still considerable overhead and it will get worse the more config you have and IMHO well worth the price of having a visible manage permissions local task that might be empty to save 10%+ time and 30%+ memory on every manage fields/display/forms page that shows those local tasks, without even talking about the extra overhead of admin_toolbar and so on doing, the same thing many times for many contact forms.

Hi, great this is solved! Where can I find the best patch to use now on Drupal 10.3.1?
Is it https://www.drupal.org/files/issues/2023-09-01/3384600-5.patch
Or will there very soon be a Drupal 10.3.2 with this patch on deck?
Greetings and again thanks for solving it,

I was going to update the status of this issue to NR, but then I saw that there is a failing test. It is the one we added in #3306434: Fix access checks for bundle permissions to avoid triggering a config validation error. We knew we would have to update that test, but we forgot to do it. (See Comment #34 here.)

When we actually remove the access function, I think we can remove some parts of the functional test. Or maybe we should do it now, since the pages we are testing do not (after the changes here) use the access function.

I can review the code, which is the R in RTBC. But someone else will have to test (T) since I do not have a site that is having performance issues. See Comment #38 for the sort of detail we need. It would be good to get some performance data for

1. Drupal 10.3.0 or 10.3.1.
2. Drupal 10.3.x, since #3306434 is now Fixed.
3. Drupal 10.3.x with the changes here, or the branch from the MR

.

@Summit, I will repeat the advice that @Berdir gave in #3306434-75: Fix access checks for bundle permissions to avoid triggering a config validation error:

See https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... and make sure you read those warnings carefully. Do *not* use the URL directly in composer patches.

Drupal 10.3.2 should be released on 2024-08-07. As I said above, this issue needs to be tested first. If that is done soon, then there is a good chance that this issue will be part of the 10.3.2 release. (See https://www.drupal.org/about/core/policies/core-release-cycles/schedule#....)

I updated the test, I don't think we need to remove anything, now the code that would have triggered the error only runs on the manage permissions page, so it IMHO makes sense to visit that. I had to update the assertion on the dblog page since there is no access denied log entry anymore of course.

Not sure we need more functional testing, we had a confirmation back in #14 and fundamentally, this is still the same change, it was RTBC before too, we really just removed some changes that are already done and updated the test a bit.

I agree that we do not need additional functional tests.

According to https://www.drupal.org/about/core/policies/core-change-policies/drupal-d..., we do need

A unit test proving the deprecation notice will be triggered when the deprecated code is called. Use a @group legacy annotation in conjunction with calls to

$this->expectDeprecation()

Back to NW for that.

The second MR for 10.3.x still needs updates.

@benjifisher @Berdir I've added an expectDeprecation check on the unit test that covers \Drupal\user\Form\EntityPermissionsForm::access().

Does the deprecation in EntityPermissionsRouteProviderWithCheck also need a test?

I also updated the 10.3.x branch. Setting to NR.

The MR for 10.3.x looks good to me. I would still like to get some manual testing (performance profiling).

Does the deprecation in EntityPermissionsRouteProviderWithCheck also need a test?

Yes, I think so. If we actually remove the class in Drupal 12, then we have to be sure that we have been generating deprecation notices in Drupal 11. Otherwise there is a chance that someone has been using the class and their code will break without warning when they upgrade to D12.

Also, looking at EntityPermissionsRouteProviderWithCheck, I am reminded that there is a comment at the top of the file that refers to EntityPermissionsRouteProvider. There is a similar comment in that file referring to this one. We should remove that comment. While you are at it, search the codebase for EntityPermissionsRouteProviderWithCheck in case there is something else we missed.

Then again, the deprecation test may not be needed. The text on https://www.drupal.org/about/core/policies/core-change-policies/drupal-d... was just updated. It now reads,

A test is required when there is backwards compatibility logic (BC). This does not apply to constructor argument BC layers. Use a @group legacy annotation in conjunction with calls to

    $this->expectDeprecation()

A test is not needed to test that the deprecation error is triggered.

I am not sure what counts as "BC logic", but I do not think there is any in this issue.

The issue status is still NW for cleaning up the comment (see #46). And we still need manual testing.

I wrote a test for the deprecation before seeing the latest comment above. I won't commit it but am including it here in case.

I added it to core/modules/comment/tests/src/Kernel/CommentTypeValidationTest.php. Do let me know if there's a better location.

<?php

declare(strict_types=1);

namespace Drupal\Tests\comment\Kernel;

use Drupal\comment\Entity\CommentType;
use Drupal\Core\Entity\EntityTypeInterface;
use Drupal\Core\Entity\EntityTypeManagerInterface;
use Drupal\KernelTests\Core\Config\ConfigEntityValidationTestBase;
use Drupal\user\Entity\EntityPermissionsRouteProviderWithCheck;

/**
 * Tests validation of comment_type entities.
 *
 * @group comment
 */
class CommentTypeValidationTest extends ConfigEntityValidationTestBase {

  ...[code omitted]...

  /**
   * Test route provider deprecation.
   *
   * @covers \Drupal\user\Entity\EntityPermissionsRouteProviderWithCheck
   *
   * @group legacy
   */
  public function testEntityPermissionsRouteProviderWithCheck(): void {
    $this->expectDeprecation('Drupal\user\Entity\EntityPermissionsRouteProviderWithCheck is deprecated in drupal:10.4.0 and is removed from drupal:12.0.0. Use EntityPermissionsRouteProvider instead. See https://www.drupal.org/node/3384745');

    // Mock the constructor parameters.
    $prophecy = $this->prophesize(EntityTypeInterface::class);
    $prophecy->getPermissionGranularity()
      ->willReturn('entity_type');
    $entity_type = $prophecy->reveal();
    $prophecy = $this->prophesize(EntityTypeManagerInterface::class);
    $prophecy->getDefinition('entity_type')
      ->willReturn($entity_type);
    $entity_type_manager = $prophecy->reveal();
    $bundle_form = new EntityPermissionsRouteProviderWithCheck($entity_type_manager);

    // Mock the method parameters.
    $prophecy = $this->prophesize(EntityTypeInterface::class);
    $entity_type = $prophecy->reveal();

    $class = new \ReflectionClass(EntityPermissionsRouteProviderWithCheck::class);
    $instance_method = $class->getMethod('getEntityPermissionsRoute');
    $instance_method->invoke($bundle_form, $entity_type);
  }
}

This might not be the best approach but it does trigger the deprecation and passes testing on my local. I have updated the comment to remove reference to EntityPermissionsRouteProviderWithCheck. grep doesn't bring up any other mentions of it in comments.

Re: manual testing, @benjifisher do you want benchmarks or more people to test and confirm here?

We IMHO had enough manual testing in earlier comments. I did profiling that it's a bit faster now, but still an issue and we have reports in the admin_toolbar issue that someone saw 20s+ page loads, I don't know if that's with the other issue or not, but either way, it's obviously still a major issue.

> I am not sure what counts as "BC logic", but I do not think there is any in this issue.

It's an interesting case. IMHO, BC Logic is code that for example maps old arguments to new. But also arguably that deprecated code itself still works, and for that, we don't have a test right now. That means we could accidently break EntityPermissionsRouteProviderWithCheck and wouldn't notice it, as nothing in core uses it anymore. I think the unit test above, given that it was basically already written now, is useful to include and we should also extend it to assert the response, that the correct access check was set on the response.

From #49:

Re: manual testing, @benjifisher do you want benchmarks or more people to test and confirm here?

From #50:

We IMHO had enough manual testing in earlier comments.

I want to see benchmarking, preferably from someone who did not work on the code fix. I would love to see some results from the person who saw 20-second page loads. As I said in #40, I would like to see a comparison of 10.3.1 (or 10.3.0), the current 10.3.x (including the fix for #3306434), and the feature branch. Comment #38 gave some results based on 10.2, without mentioning any terrible page-load times.

After #3306434, we know there will be less logging, which should improve the performance a bit. Probably we will still need this issue, but I would like to confirm that.

That said, I am not stopping someone else from declaring this issue RTBC.

I do not have a strong opinion about whether the test is needed, so I will defer to @Berdir in #50.

If we do add a test, then it should be a unit test, not a functional test, according to the doc I mentioned. That means it should not assert the response. (We already have that in Drupal\Tests\user\Functional\UserPermissionsTest.) We are testing a class in the user module, so I think the correct namespace is Drupal\Tests\user\Unit (directory core/modules/user/tests/src/Unit/). Possibly create the sub-directory/child namespace Entity, with just the one test.

I generally do not use ReflectionClass to access protected methods in tests. Instead, look for a public method that uses the protected one: in this case, getRoutes() (defined in the parent class) calls getEntityPermissionsRoute(), so call that.

You should not have to mock $entity_type twice.

I do not like the variable name

$bundle_form = new EntityPermissionsRouteProviderWithCheck($entity_type_manager);

I would use $route_provider instead, or just

$this->expectDeprecation(...);
(new EntityPermissionsRouteProviderWithCheck($entity_type_manager))
  ->getRoutes($entity_type);

(Put expectDeprecation() right before the line that triggers the deprecation, not at the top of the test method.)

I made a small suggestion on the MR: keep a little more of the comment. You can tweak the wording if you like, or use the "Apply suggestion" button to use my version.

One minor thing about the version this is deprecated in, will ping @catch to confirm.

I want to see benchmarking, preferably from someone who did not work on the code fix. I would love to see some results from the person who saw 20-second page loads. As I said in #40, I would like to see a comparison of 10.3.1 (or 10.3.0), the current 10.3.x (including the fix for #3306434), and the feature branch. Comment #38 gave some results based on 10.2, without mentioning any terrible page-load times.

I doubt that people will provide that. We know lots of sites are affected, but not many know how to do profiling, we're clearly struggling to even get basic confirmations that the current version fixes their problems despite 34 followers here and 70+ in the other issue.

Most importantly, the change is not complex. There is no new code, no complexity, no caching overhead or anything like that to balance. We have an expensive call, we remove it. We know it's slow (not terrible, but still slow) already on 10.2, and we know it gets worse with every single confg entity a site has and start from 10.3, it gets called many times too, multiplying those slow calls.

This was also already RTBC by @catch in #33.

To follow-up to that, @catch confirmed in slack:

> yeah deprecation in 11.1, then a 10.3-11.0 backport patch with no deprecation I think.

I agree with @catch, well said!

Given @Berdir and @catch's profiling comments and my updates to the deprecation (10.4-to-11.1), I'm setting this to NR.

@bsuttis:

Thanks for updating the MR. The changes look good to me.

@catch:

I may have been careless when I used the term "benchmarking" in #51. I am not asking for an automated performance test. All I want is one user who experiences this problem to say one of two things:

I tested on 10.3.0 (or 10.3.1) and it took X seconds to load a page. I tested on 10.3.x, and it got a little better: Y seconds. I tested with the feature branch for this issue, and it was much better: Z seconds.

or

I tested on 10.3.0 (or 10.3.1) and it took X seconds to load a page. I tested on 10.3.x, and it got much better: Z seconds. I tested with the feature branch for this issue, and it was still about Z seconds.

In other words, there are two parts to RTBC: the review (R) and the test (T). I have done the R, but I cannot do the T, so someone else has to.

Hiding patches for MR.
Scratched out remaining tasks as seems consensus is to deprecate EntityPermissionsRouteProviderWithCheck

Ran test-only feature to check test coverage

https://git.drupalcode.org/issue/drupal-3384600/-/jobs/2347056

So that's verified.

Did a manual test very similar to actual tests
Went to admin/structure/comment/manage/comment/permissions
Verified I get an access denied
Applied the MR
Went to admin/structure/comment/manage/comment/permissions again
Got an empty table.

Appears as though all threads have been addressed.

Will go on a limb and mark this but didn't do a benchmark test

@catch:

I tried to rebase 3384600-10.3.x-3384600 onto 10.4.x, but gave up. I do not have permission to close MR 8694, but I did hide it on this issue.

I cherry-picked MR 8488 onto 10.4.x and pushed both 10.4.x and the new 3384600-10.4 branch to the issue fork, and I opened MR 9500. Someone else can take the next steps.

I notice that you did not publish the change record (CR). Is that an oversight or do you want to backport to 10.4.x first?

If we want to backport this to 10.4.x, we can do so but would need to remove all the deprecations (and test coverage of the deprecations).

This is the first time I have run into this. I guess the point is that we do not want deprecations in 10.4 that are not in 11.0?

The CR currently has

Introduced in branch: 11.x
Introduced in version: 10.4.0

I think the version should be updated to 11.1.0.

Cross-posted from https://www.drupal.org/project/admin_toolbar/issues/3459723#comment-1580... upon request:

I spent the past couple of weeks troubleshooting (e.g. database tuning) some extremely slow page loads on admin pages. Today I found this thread and I think it solved my problem.

I'll try to address the questions from @benjifisher in the previous comment although the Drupal versions are different.

I chose a random user profile and recorded the following in Firefox dev tools (network tab, cache disabled). All values are for the first line in the dev tools output -- initiator: document, type: html. There are a lot of other elements (js, css) loading after that but they're all pretty fast:

All tests were done with Drupal 10.3.5, and admin_toolbar 3.5.0. Rebuilt cache before each of these:

@catch: I agree FWIW:

Although it looks like as long as one of the change from here or the other issue, that times are within margin of error at least as measured by the full page request.

@catch confirming I rebased and removed the deprecation code for the 10.4.x branch. There are failing tests but they aren't related to these changes.

10.4 MR appears to have several test failures. Need to have a green pipeline before marking RTBC.

@smustgrave what's the protocol when the fails appear to be non-MR related? For example, https://git.drupalcode.org/issue/drupal-3384600/-/jobs/3051456 shows a fail in Drupal\Tests\Core\Recipe\RecipeQuickStartTest::testQuickStartRecipeCommand

Is this an indication the 10.4.x branch that was rebased on is not up-to-date? Or a problem with the 10.4.x branch? I'm pretty sure I rebased on its latest commit at the time (dc4a5f9e0a0).

Most likely an issue with the 10.4 branch on this ticket. Doubt the 10.4.x branch on head has all those failures.

@smustgrave I rebased again (2 new commits since yesterday) and latest test is now green: https://git.drupalcode.org/issue/drupal-3384600/-/pipelines/310491

One small thing to address.

10.4 is no longer supported. As a major bug fix this can be backported to 10.6, so changing version and updating title.

I read @berdir comment in the MR and this also needs a followup to address the comment in \Drupal\user\Entity\EntityPermissionsRouteProvider.

To rebase an MR to another branch read https://www.drupal.org/node/3344224/revisions/12990041/view#s-rebasing-a-merge-request-to-a-new-base-branch