Return early in EntityPermissionsForm::access if the user does not have "administer permissions"

Looking at \Drupal\KernelTests\Core\Config\ConfigDependencyTest::testConfigEntityUninstallComplex we have a way to test this fairly easily.

something like this

$comment_type = CommentType::create($values)->save();
\Drupal::state()->set('config_test.fix_dependencies', [$comment_type->getConfigDependencyName()]);
\Drupal::state()->set('config_test.on_dependency_removal_called', []);
// Visit route that calls access check for comment type permissions, e.g. the comment type form with user without administer permissions.
$called = \Drupal::state()->get('config_test.on_dependency_removal_called', []);
$this->assertEmpty($called);

I still think we should consider to drop this feature (hide the permission local task dynamically if no permissions are defined) entirely. This will improve the situation for all users without administer permissions which is much better, but as such a user, you're still affected by the problem.

IMHO, having a permissions tab with a page that says "sorry, no permissions for you" would be quite acceptable for the handful of entity types that don't come with bundle permissions by default.

Just confirming that @acbramley's approach in the MR fixes our major performance issue.

However I'd also strongly agree with @Berdir, dropping this permission check altogether makes a lot of sense.

Just noting that removing it should also effectively fix #3306434: Fix access checks for bundle permissions to avoid triggering a config validation error.

See comment in the other issue. I think my suggestion to remove it changed to a pretty strong recommendation to do so based on that.

Removing the access check would not be that complicated. We need to stop using \Drupal\user\Entity\EntityPermissionsRouteProviderWithCheck, add a deprecation message on it and add a deprecation message on \Drupal\user\Form\EntityPermissionsForm::access(). I guess some tests to adjust and dealing with an empty form then.

But the problem you have will then still happen *on* the permission page, and this issue doesn't fix that either yet (it will only happen once and then once per menu link being checked). Maybe that could be the follow-up, to replace the getConfigEntitiesToChangeOnDependencyRemoval() call entirely with a new approach?

Re #9:

Removing the access check would not be that complicated.

+1 to that.

But the problem you have will then still happen *on* the permission page ...

There is a performance problem when checking many bundles (such as block types) to see whether they have related permissions. Has anyone reported a problem when checking just one?

When you are viewing the permissions form, you have to go through the same work in order to generate the form. If we want to avoid doing that twice, we could statically cache the result (when checking access) and then use the cached value when generating the form. I think that is out of scope for this issue, and I am not sure there is a need for it.

I do not know a way to cache more permanently. In theory, a change to any config item could make or break a dependency chain from a permission to a bundle.

Re #6:

Just confirming that @acbramley's approach in the MR fixes our major performance issue.

My understanding from the Slack thread is that the site where this was reported applied the patch from #2809291: Add "edit block $type" permissions to a 10.0 site, and that applying the +1/-1 patch from the follow-up issue #3315042 was also enough to fix the performance issue.

My understanding from the Slack thread is that the site where this was reported applied the patch from #2809291: Add "edit block $type" permissions to a 10.0 site, and that applying the +1/-1 patch from the follow-up issue #3315042 was also enough to fix the performance issue.

Yeah sorry I wasn't clear there, that is indeed the case. What I was commenting on here was when we reverted #3315042: Remaining tasks for "edit block $type" permissions, i.e. we were using EntityPermissionsRouteProviderWithCheck rather than EntityPermissionsRouteProvider, then when the approach in the MR in this issue was applied that our performance issues also went away.

Part of the block reorganization we are adding more permissions for blocks so this might be impacted.

> There is a performance problem when checking many bundles (such as block types) to see whether they have related permissions. Has anyone reported a problem when checking just one?

I think the performance aspect is one thing, but it's also that this is using config dependencies in a way that they are not made for IMHO. This function goes recursively through dependencies of dependencies and prepares them all for the situation that is being asked for: what to do if this config entity will be deleted.

But I'm already happy if we agree on the first step, not trying to handle this in the access check and just display a message about not having any permissions and then we'll see from there.

So do we want to go forward with the MR?

re-reading the comments

But I'm already happy if we agree on the first step, not trying to handle this in the access check and just display a message about not having any permissions and then we'll see from there.

Sounds like this is the path forward.

We already have a big performance win by applying this patch. I don't understand why this was moved back to needs work, the patch we have already accomplices most about this issue?

Moving back to needs review - the patch looks fine for what it is. I think we should open a separate issue to remove the permissions check altogether and just show a message in the case of no permissions - that seems fine and a lot better than (apparently) 12 second page loads.

Here's the follow-up #3384600: [backport] Don't hide permissions local tasks on bundles when no permissions are defined.

@borisson_ reported in the #performance slack channel that:

page loads that go from > 12 seconds to ~1 sec for logged in users on local with this patch.

Fair enough. FWIW, users *with* that permission will still see those 12s load times and I'd have preferred to solve it for those as well, but I can live with that being a follow-up.

Yeah if this hadn't already been open for six months I'd probably want to keep going here, but since it has let's stop some of the bleeding.

Committed/pushed to 11.x and cherry-picked to 10.1.x, thanks!