Make MediaLibraryState implement CacheableDependencyInterface to remove the need for hardcoding a cache context

This look good apart from the missing test coverage, +1.

This is probably at least a major task. :)

This was opened in response to @effulgentsia's review at #3038254-60: Delegate media library access to the "thing" that opened the library. Crediting him.

This is not major, and does not affect the cacheability of any response. @effulgentsia merely pointed out that hardcoding some cache context is not semantically ideal, it'd be better for the object that causes that cache context to be added to implement CacheableDependencyInterface so that the hardcoding can be removed.

#2 should've removed the hardcoding, but did not yet. Also, this does not need test coverage. It's a pure refactoring.

+++ b/core/modules/media_library/src/MediaLibraryFieldWidgetOpener.php
@@ -76,7 +69,10 @@ public function checkAccess(MediaLibraryState $state, AccountInterface $account)
+      if ($entity_access instanceof RefinableCacheableDependencyInterface) {
+        $entity_access->addCacheableDependency($state);
+      }
+      return $entity_access;

ick - anyone know if we have an issue to avoid this dance? (unrelated to this patch of course)

Committed a19a380 and pushed to 8.8.x. Thanks!

ick - anyone know if we have an issue to avoid this dance? (unrelated to this patch of course)

No, we don't, and we can't. The reason: we didn't want to require AccessResultInterface to implement (Refinable)CacheableDependencyInterface. Put differently: we want code to be able to not care about cacheability of access result.

Or, rather: that is what we wanted a few years ago. Perhaps we feel differently about it today.

Perhaps we feel differently about it today.

Let's create a follow up to discuss this. I do think we have evolved during this time.

LGTM! Flipping to RTBC.

+++ b/core/modules/media_library/src/MediaLibraryFieldWidgetOpener.php
@@ -83,7 +76,10 @@ public function checkAccess(MediaLibraryState $state, AccountInterface $account)
-      return $process_result($entity_access);
+      if ($entity_access instanceof RefinableCacheableDependencyInterface) {

@@ -107,7 +103,11 @@ public function checkAccess(MediaLibraryState $state, AccountInterface $account)
+    $access = $entity_access->andIf($field_access);
+    if ($access instanceof RefinableCacheableDependencyInterface) {

When wouldn't this be an instance of RefinableCacheableDepdenencyInterface?

The fact this got reverted by accident indicates it probably should have had a test in the first instance.

Let's add one now to prevent a repeat regression.

Answer to my comment above is in #10

No, we don't, and we can't. The reason: we didn't want to require AccessResultInterface to implement (Refinable)CacheableDependencyInterface. Put differently: we want code to be able to not care about cacheability of access result.

The fact this got reverted by accident indicates it probably should have had a test in the first instance.

To be fair, the accidental revert in my patch would have probably reverted any tests as well. I think I forgot to git pull before creating the patch that reverted it, and somehow we all overlooked the changes.

Agreed, test seems pretty straightforward. Back to RTBC!

Committed 9674e3a and pushed to 9.3.x. Thanks!

We're in a mini-freeze for 9.2.x a the moment due to the patch release, so leaving RTBC for backport.

8.9.x is security-only now so this won't get backported there.

Cherry-picked to 9.2.x.