SessionCacheContext class should implement CacheContextInterface

+++ b/core/lib/Drupal/Core/Cache/Context/SessionCacheContext.php
@@ -29,4 +30,11 @@ public function getContext() {
+  public function getCacheableMetadata() {

Does it makes sense to add unit test for that? or maybe kinda integration test?

Agreed with @benjifisher that that is overkill. Explicit test coverage was added at \Drupal\KernelTests\Core\Cache\CacheContextOptimizationTest, in #2512866: CacheContextsManager::optimizeTokens() optimizes ['user', 'user.permissions'] to ['user'] without adding cache tags to invalidate that when the user's roles are modified.

I think the addition of the interface should go in a separate issue. Afaics from the code because the ID does not contain a "." or ":" we'll never call getCacheableMetadata().

So do we think all cache contexts need to implement this? At the moment obviously nothing is enforcing this. I think before just implementing the interface here we need to think about the wider issue.

That's fair! I'd like that too, then we'd never have to deal with this again.

I can think of two ways to do this:

1. Add a test that verifies that every cache.context-tagged service implements CacheContextInterface or \Drupal\Core\Cache\Context\CalculatedCacheContextInterface.
2. Add an assert() in \Drupal\Core\Cache\Context\CacheContextsManager::__construct() that gets each service and asserts it implements either of those two interfaces.

@Wim Leers but is it true that we should ensure that all cache contexts have this interface? Is it wrong if they don't?

I'm not sure what you're asking. Is it absolutely necessary? No. But then we might as well remove 90% of interfaces in Drupal core. Is it useful to ensure consistency? Yes.

The thing that bothers me here is that we're adding an interface that is not enforced that enforces a method that is never called.

The thing that bothers me here is that we're adding an interface that is not enforced

Agreed. I proposed solutions for that in #7. I'd be happy to address either of those solutions (or another one) in a follow-up.

that enforces a method that is never called.

\Drupal\Core\Cache\Context\CacheContextInterface::getCacheableMetadata() definitely does get called. It just happens to never get called for the session cache context because that cache context doesn't have parent cache contexts, which means it can never be optimized away. At least not until #2453835: Allow non-intrinsic (implementation-dependent) cache context services to specify their parents via DynamicParentContextInterface::getParents() lands.

@Wim Leers I meant that it does not get called in this instance.

Okay, I understand.

That's because the interface was designed to be forward-thinking, to allow #2453835: Allow non-intrinsic (implementation-dependent) cache context services to specify their parents via DynamicParentContextInterface::getParents() to land without the need for further changes.

@Wim Leers but then that change needs to check if contexts implement the interface because as we can see - at the moment that is not enforced (and can't be without a BC breaking change). So in someways it's good to have an example in core of a context that doesn't implement an interface because this is perfectly possible in contrib.

So let's do this and revisit this after 8.5.0-alpha.

I think it makes sense to remove mostly all boilerplate code to base class because only few "contexts" override this method

@Wim Leers I think we need both options from #7

Thanks for reviving this issue, @andypost!

However, I don't understand where you're going with #18.

@Wim this probably needs separate issue but imo fits in task

Can you explain what you're doing and why? It looks very different from previous patches.

I just moved common boilerplate code to base class

I think @andypost's patch is a good idea, but it's not in scope for this issue. That should go in a new issue.

Instead #7 should be implemented. The assertion isn't that easy to implement in the __construct, because it get's an array of cache context ids, not the actual services. Loading the services to check for the services would be a big hit in performance.

I added a test, not sure about the location and if it's sufficient.

Filed follow-up #2971941: Move boilerplate code for cache metadata to base class

This one good to go

I'm not sure that #24 or #18 is really what @Wim Leers and I discussed in #15 and #16.

Found this following #3221128: Enable service autoconfiguration for core event subscribers

If we enable autoconfiguration, that means we can automatically add the cache.context tag to services that implement a specific interface. But as I found over there, not all cache contexts actually implement CacheContextInterface.

I think we should start throw deprecation if cache contexts does not implement context interface

+++ b/core/tests/Drupal/KernelTests/Core/Cache/CacheCollectorTest.php
@@ -60,4 +62,15 @@ public function providerTestInvalidCharacters() {
+   * Tests that cache contexts implement the right interfaces.
...
+  public function testContextImplementsInterface() {
+    $cache_contexts = \Drupal::service('service_container')->findTaggedServiceIds('cache.context');
+    foreach (array_keys($cache_contexts) as $context) {
+      $service = \Drupal::service($context);
+      $this->assertTrue($service instanceof CalculatedCacheContextInterface || $service instanceof CacheContextInterface);

This test still makes sense but needs to throw deprecation if collector discovers cache context without interface

+++ b/core/lib/Drupal/Core/Cache/Context/RequestStackCacheContextBase.php
@@ -11,7 +12,7 @@
-abstract class RequestStackCacheContextBase {
+abstract class RequestStackCacheContextBase implements CacheContextInterface {

The change in #18 is BC break so collector should care about it

Looking at usage of CalculatedCacheContextInterface there's only one check for interface in \Drupal\Core\Cache\Context\CacheContextsManager::getLabels()