Try to simplify checks in AdminNegotiator

From [33500687]:

FWIW, I have no idea why that whole check exists. There is no possibility that user roles don't have a storage handler? This check was added all the way back in #1954892: Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system in 2013 when AdminNegotiator was introduced, there was no similar thing before that in the code that it replaced.

This was added a very, very long time ago. I think this is a good Novice issue to remove the hasHandler() check. If anything fails we can investigate but that seems very unlikely.

We can also remove the injected EntityTypeManager property and from the constructor, that will require some constructor changes for BC, that's a bit more complicated.

I'm going to pick up this one. It seems that we can safely remove the check. The negotiator and the storage is provided by one and the same module which is part of the core and can't be installed.
I would also add some improvement to check the route first - there is no way to trigger any permission checks if the rote is not an admin route (the route object is available from request, so it's no heavy resources operation).
For the entity type manager, we have to keep it as it is (the constructor parameter) but the deprecation notices must be added, I will try to handle it in the scope of the issue. I'm not sure what should be the target version after this will be completely removed.

Don't worry about target version - work on 11.x and leave issues at 11.x-dev even until RTBC, the committers will decide what versions are applicable.

I think the target version question is about the deprecation message, I'd go with 12.0 for now.

Performance is likely a wash, both should be fast. permissions are loaded at this point too.

For BC on the property, you can use \Drupal\Core\DependencyInjection\DeprecatedServicePropertyTrait, see \Drupal\Core\File\FileSystem for an example. The constructor tends to be awkward when it''s not the last argument. See \Drupal\user\Plugin\views\filter\Permissions::__construct for an example. allow both types, if it's the wrong one, trigger a deprecation message.

I've opened MR with all the needed changes, but I still work on tests. Some unexpected issues takes place and I'm not sure how it's related to our changes, I'm still investigating it.
Regarding the deprications I used the proposed trait to handle the deprecated service.
BTW I confirmed that checking the permissions is slower then checking the routes, both numbers are quite low, so it's not something that anyone can notice, but it is always good to reduce the unnecessary operations. The logical implication is the same, if the route is not the admin route, then there is no reason to check any permissions.

I've updated all the tests, seems like the number of cache get's has changed because of the routing which is now checked as first. The cron UI test was wrong from the very beginning because the intention was to create the admin user to validate some text in the admin panel, but in fact the regular user was created with just a single permission and it's !== admin user.

Nice to see the cache gets go down, I think this might have been how @berdir found this in the first place.

I think the scope of the fix is too wide. I think the changes in the tests and to the constructor are not needed?

I think only AdminNegotiator::applies and the changes to StandardPerformanceTest should stay.

I'm not sure about this though, so @sayco don't change this yet until we get confirmation what the right path forward is.

I've addressed all of the comments I guess, but the pipeline keeps failing because of the cron tests, I'm not sure what we are going to do about it - I suppose it's out of the scope of the current issue.

I think this looks good now, there seems to be a failure in CronRunTest, is this a known random fail?

Do we need a CR for this?

Strange, I have seen CronRunTest fail a few times recently, but this seems to make it more likely. I can't reproduce locally. Looking at https://issue.pages.drupalcode.org/-/drupal-3501727/-/jobs/4142293/artif..., it says cron was run one second ago.

it's some race condition between the login, which triggers cron and deleting it, as cron is likely still running when we delete state, and then immediately sets it again.

\Drupal\Tests\system\Functional\System\CronRunTest::testAutomatedCron above already uses setWaitForTerminate, I think testCronUI() should do the same.

We'll have to fix this either here or a separate issue and postpone it on it.

There was only one test failing (when I viewed the pipeline). One functional test. I re-ran it and pipeline is all green. Just one test failing is pretty good especially if it passes on 1st re-run. But maybe you @sayco re-ran tests lots of times prior to me?

Rebased.

But maybe you @sayco re-ran tests lots of times prior to me?

@oily I intentionally left it in a failed state, to prove that the tests tend to fail on that test case.
It's good to see that it is all green now!

We'll have to fix this either here or a separate issue and postpone it on it.

@berdir the Cron tests should be handled in a separate issue IMO, but it shouldn't be a blocker for this ticket (they seem to be unrelated, especially if it's a known failure)

The testing pipeline result is binary, it either passes or not, it doesn't matter if it's just one test or 100. I've seen CronRunTest fails recently as well before this. But if there is a chance that this issue increases the chance for that test to fail randomly then that needs to be investigated.

It's a useful improvement, but it is not important enough to risk even more disruption due to random fails, it's bad enough as it is. I do believe the fix should be relatively easy. It might be better to open a new issue and do that separately though.

@berdir How about we leave things as is but try a few rebases and see how many of them result in crontest failure?

That way we could answer your question

But if there is a chance that this issue increases the chance for that test to fail randomly

.

It looks like someone already ran the 100 times test: https://git.drupalcode.org/issue/drupal-3501727/-/jobs/4144236

We can run that a few more times and link the job runs here.

Yes, I did that, but I don't know if just running that specific test has the same kind of load situation. did it twice and didn't see the fail.

I've repurposed an old issue about CronRunTest random fails to fix the new one: 🐛 CronRunTest::testAutomatedCron Active

Still not 100% sure if this doesn't somehow make the fail more like as I think it failed at least twice in the pipelines in this MR. I'm fairly certain the change in the other issue fixes this and it's less risky to commit that first, then this issue.

This should also be rebased as 📌 Aggregate cache operations per bin to allow for more specific asserts Active got in, but i'd prefer/recommend to wait until 📌 Review cache bin and cache tags of access policy caching Active is in, as that will change things again around caching, as mentioned in the review.

Both of those issues are now, so this needs a rebase to resolve the conflict, I'd expect that this no longer has an impact on performance test metrics now.

Should I rollback the changes to the metrics I did and we will see if the test runs fine without the changes?

I would start with that, you'll have a conflict on that anyway

Ok I rolled back the changes and the pipeline fails, so the change was indeed needed, the CacheGetCount is reduced
├ Failed asserting that 81 is identical to 82.

I changed the value back to 81 and let's see the pipeline, I suspect that it will fail on the other key in $expected array in the same test.

Ok it succeeded, so it seems like only CacheGetCount was reduced

Added one comment about the construct deprecation, then this is ready, also investigated why the cache get count changes.

Looks good to me now.

Has merge conflicts.

Done, I hope this time we'll manage to push it forward before anything else is merged to dev branch :)

Committed/pushed to 11.x, thanks!

catch → closed merge request !11002

Automatically closed - issue fixed for 2 weeks with no activity.