GroupSitesAdminMode::isActive() should be cached

see attached MR

Looks good to me, will accept once we can run pipelines.

While the changes initially looked acceptable to me, they break the tests. Could be that the tests simply need adjusting then, but setting to NW.

I'm inclined to reject this patch. Sure it adds caching using a property, but the question is whether we need caching to begin with?

If you call ::isActive() more than only a few times during a request, you really ought to consider why you're calling it that often. You could cache the result in your implementing code or, better yet, cache the outcome of why you needed to check the admin mode. If you're calling ::isActive() from multiple places, then you should probably refactor your code so that the admin mode dependency is more centralized.

Either way, I'm not opposed per se, it's just that I don't see the overall value compared to the downsides:

• Potential stale "cache" property
• Difficult to test across multiple requests (aka browser tests)

The current test fails are because the static property is not affected by drupalGet() (which is another request) and therefore reports incorrect values according to the test cases.

I'd update the tests, as the benefits are bigger than the downsides. it removed 30k requests/min from our customer project. Augmenting the class with another class doing the cache seems like bloat to me. It's not a public property, there are getters and setters, so the class is in full control over its implementation, so you can handle all side cases inside the class.

Currently, the following calls isActive():

• The route access check for turning admin mode on or off
• The toolbar admin mode switcher
• The access policy (only runs if your permissions weren't retrieved from cache)
• The 'user.in_group_sites_admin_mode' cache context

So at best, you only call isActive() once: for the cache context. Then, based on the result it retrieves the right toolbar item and your group permissions from cache. At worst, you call it three times if your permissions weren't cached, because the switching callbacks don't render the toolbar so it's either one or the other.

I'm on the fence. Calling the private temp store between 1 and 3 times doesn't seem that crucial to warrant caching the result. But if custom code is also interested in it, it might bump that to 3 - 6 times. Any higher and I would still tell you look into the custom code first.

I'm just wondering if I am fully onboard with using a mere property here. Perhaps a memory cache would be better so we can reset it in the browser tests.

Going to try and fix this in 10.3 using MemoryCache and then postpone the issue until that version of core is released.

Seems good to me now, will set to postponed as discussed earlier.

WARNING do not use this on D10.2 or lower as the MemoryCache is not properly recognized there yet and will fall back to the default backend, often DB.

Phpcs can be ignored because by the time we get D10.3, Coder should have a new release and it includes 📌 Allow _construct() method to omit docblock Needs review

Needs CR and adjust code to point to said CR when we are ready to tag a release.