Move cache contexts classes from \Drupal\Core\Cache to \Drupal\Core\Cache\Context

Good idea.

DrupalCon LA 2015

First Patch for this issue with the Changes to Cache Contexts.

At DrupalCon LA 2015 Sprints

2 files that used a cache context but didn't have the new namespace in their use statements.

Looks like I forgot one.

The remaining fatal errors seem to work locally, hopefully testbot agrees.

I can't seem to reproduce the current errors locally when running tests.

Looks like some mocked objects were missed in the conversion. Hopefully this fixes the failures.

Rerolled

+1

+++ b/core/modules/views/tests/modules/views_test_data/src/Cache/ViewsTestCacheContext.php
@@ -7,7 +7,7 @@
diff --git a/core/tests/Drupal/Tests/Core/Cache/CacheContextsManagerTest.php b/core/tests/Drupal/Tests/Core/Cache/CacheContextsManagerTest.php

diff --git a/core/tests/Drupal/Tests/Core/Cache/CacheContextsManagerTest.php b/core/tests/Drupal/Tests/Core/Cache/CacheContextsManagerTest.php
index 859aee6..20f9f94 100644

index 859aee6..20f9f94 100644
--- a/core/tests/Drupal/Tests/Core/Cache/CacheContextsManagerTest.php

--- a/core/tests/Drupal/Tests/Core/Cache/CacheContextsManagerTest.php
+++ b/core/tests/Drupal/Tests/Core/Cache/CacheContextsManagerTest.php

If we move the CacheContextsManager, should we not move this one as well?

If we move the CacheContextsManager, should we not move this one as well?

Yeah we better do.

Why use plural into the "Context" namespace ? Just trolling but it feel very weird, Validator/Validate or Contraint namespaces are not plural if I remember correctly.

Good point @pounard!

Thanks, I'm +42 for this change, last time I was working on the Redis cache backend for D8, contextes where everywhere seriously polluting the overall cache design when I was navigating in the code, this change makes everything clearer.

I'm wondering a bit why we don't make this \Drupal\Core\CacheContext - it's a completely separate API to the backends. But then we could also eventually do \Drupal\Core\Cache\Backend eventually. Do not mind very much, having everything at the same folder level is indeed confusing though.

#26: Because \Drupal\Core\Cache\Cache deals also with cache contexts I think the current proposal is better.

It is part of the caching subsystem and yes indeed having a subfolder for backends would be good.

So just to recap the changes needed:

• Move Drupal\Core\Cache\Contexts to Drupal\Core\Cache\Context
• Move Drupal\Tests\Core\Cache\CacheContextsManagerTest to Drupal\Tests\Core\Cache\Context\CacheContextsManagerTest

#28: Yes, that is correct.

Here are the changes listed in #28

This issue is a normal task that introduces a BC break, and per https://www.drupal.org/core/beta-changes, we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary. Also a bit clearer summary would be helpful since the issue it links to is very long. :)

Rerolled because of #2493021: Remove unused & useless services from HtmlRenderer.

For the beta criteria I think this is an acceptable change because the class structure at the moment makes it quite difficult to know what's necessary to implement for what bits of the cache API - i.e. it's hard to see what's for backends/tags and what's for contexts, and this makes both clearer. Should have zero actual contrib impact since the API is very new and most modules won't need to interact with the classes/interfaces at all.

Assigning to xjm though in case I'm being overly generous.

However we definitely need a change record and change record updates.

phpunit --debug -c core/
...
Starting test 'Drupal\Tests\system\Unit\Menu\MenuLinkTreeTest::testBuildCacheability with data set #0 ('Empty tree.', array(), array(array(array(), array(), -1)))'.
PHP Fatal error:  Call to undefined method Mock_CacheContextsManager_9df8c868::validateTokens() in /Library/WebServer/Documents/drupal8x/core/lib/Drupal/Core/Cache/Cache.php on line 40

Fatal error: Call to undefined method Mock_CacheContextsManager_9df8c868::validateTokens() in /Library/WebServer/Documents/drupal8x/core/lib/Drupal/Core/Cache/Cache.php on line 40

@cilefen
Yeah, you need to update that class reference as well

Hrm. Unfortunately, while I can see why the reorganization of the classes would be a DX improvement, I don't really think this passes the beta criteria: https://www.drupal.org/core/beta-changes#flowchart

1. It's not a feature request.
2. It's not unfrozen or critical.
3. It's not major nor a prioritized change (no bugfix, usability, accessibility, security, or performance impact).
4. It doesn't really reduce fragility.

So even if the disruption is small, it seems to me we shouldn't make this change anymore at this point.

Not postponing just yet; going to check with @Wim Leers and @catch first.

On the other hand we don't treat existing random classes as API, and which cache context is basically a random class.

So I talked about this more with @catch for the background information on this and why it's important. catch pointed out that this issue is a followup to a recent critical (#2335661: Outbound path & route processors must specify cacheability metadata) which is potentially a prioritized change. It's a bit borderline, because the namespace itself is not new, but the interface is new as of #2428563: Introduce parameter-dependent cache contexts and having loads of different cache context is even more recent.

So there is a case for it to go in. I've updated the beta evaluation clarifying why. (For the beta evaluations of the future: "DX" is not a prioritized beta change on its own; that's a priority for 8.1.x and 9.x depending on the BC. Cleaning up stuff that's introduced by fixing criticals, however is important and in scope, because the sooner the better once the critical lands, but without blocking it.) :D

One thing I'm not sure of is whether or not we should provide BC. See: https://www.drupal.org/core/beta-changes#bc

We could reduce the disruption of the change by making all the changers in this patch, but also retaining a \Drupal\Core\Cache\CacheContextInterface that is an empty extension of \Drupal\Core\Cache\Context\CacheContextInterface. I wasn't sure whether that would be better or worse than just moving the interface. We'd still move the child classes. Since those will typically be used via the core services provided for them, that'd mean the only disruption would be for subclasses of existing context which are even less likely to exist.

I see a case for just moving the interface since it's only three months old anyway. Setting NR for that. If others think that's best then just set it back to RTBC with that consensus.

Thanks everyone for your patience and your help explaining the change here in the issue and in IRC.

Well yeah, having those kind of BC layers is not a bad idea. This would also allows us to put @deprecated on the class and then let people figure things out automatically.
But I agree with you, the interface is quite new, and the amount of contrib modules implementing that won't be that high. I vote for sanity.

But I agree with you, the interface is quite new, and the amount of contrib modules implementing that won't be that high.

Based on the /modules folder of d8status.md-systems.ch, which doesn't contain everything, but a lot (around 75 projects at the moment), that amount is currently 0 :)

There probably are some use cases around access and modules like organic groups, but those few modules haven't picked this concept/interface up yet.

From that perspective, moving it seems fine.

Alright, in it goes then. Committed and pushed to 8.0.x. Thanks!

I also published the CR.

Awesome! Thank you for letting this in!!! This makes things sane ...