Corrected caching docs and deprecation behaviour (fixes #8272) #8274
Conversation
| @@ -184,32 +184,22 @@ class PageController extends ContentController | |||
| } | |||
| ``` | |||
|
|
|||
| ## Defaults | |||
There was a problem hiding this comment.
I can't believe we still had this in there, it's completely out of date :(
|
|
||
| $supportedDirectives = ['max-age', 'no-cache', 'no-store', 'must-revalidate']; | ||
| if ($foundUnsupported = array_diff(array_keys($configCacheControl), $supportedDirectives)) { | ||
| throw new \LogicException( |
There was a problem hiding this comment.
Could you add this to the PHPDoc please?
tests/php/Control/HTTPTest.php
Outdated
| public function testDeprecatedVaryHandling() | ||
| { | ||
| /** @var Config */ | ||
| Config::inst()->update( |
There was a problem hiding this comment.
- Config::inst()->update(
+ Config::modify()->set(There was a problem hiding this comment.
Oh is that what the cool kids are using these day? :D Updated.
chillu
force-pushed
the
pulls/4.2/cache-docs-and-deprecation-handling
branch
from
e28beef
to
d92e891
Compare
| @@ -68,6 +68,65 @@ public function testSetMaxAge($state, $immutable) | |||
| } | |||
| } | |||
|
|
|||
| public function testEnableCacheWithMaxAge() | |||
There was a problem hiding this comment.
I think these tests aren't needed and we should just update the provider and the existing tests...
There was a problem hiding this comment.
For this particular test (testEnableCacheWithMaxAge), I agree with you - it's already covered by testSetMaxAge.
But otherwise, I find this particular use of PHPUnit providers quite unreadable actually - it took me a while to figure out what the meaning of $immutable is in this context. I think providers are fine when they contain one dimension, since that's something you can keep in your head. But otherwise, DRY principles apply differently to unit tests: In my opinion, clarity (and sometimes verbosity) is more important than brevity. I can understand what testEnableCacheWithMaxAgeAppliesWhenLevelDoesNot tests. I couldn't understand this if it was mixed in as a third array element in provideCacheStates, even when using named data provider sets
There was a problem hiding this comment.
Update: The difference with testEnableCacheWithMaxAge (compared to testSetMaxAge) is the max-age setting via argument rather than setter. Not all methods in your data provider have this argument, because it doesn't make sense for all of them - so you'd either have to use a different data provider, or add more confusing dimensions to the existing one. And since the assertions change between enableCache and publicCache usage, I don't think a new data provider capturing both cases will be more readable.
| @@ -59,8 +59,8 @@ Does not set `private` directive, use `privateCache()` if this is explicitly req | |||
| Simple way to set cache control header to a cacheable state. | |||
| Use this method over `publicCache()` if you are unsure about caching details. | |||
|
|
|||
| Removes `no-store` and `no-cache` directives; other directives will remain in place. | |||
| Use alongside `setMaxAge()` to indicate caching. | |||
| Removes the `no-store` directive; other directives will remain in place. | |||
There was a problem hiding this comment.
This still isn't quire true; it doesn't remove the no-store directive unless a max-age is supplied.
chillu
force-pushed
the
pulls/4.2/cache-docs-and-deprecation-handling
branch
from
d92e891
to
5670280
Compare
dhensby
force-pushed
the
pulls/4.2/cache-docs-and-deprecation-handling
branch
from
5670280
to
bdab939
Compare
dhensby
force-pushed
the
pulls/4.2/cache-docs-and-deprecation-handling
branch
from
bdab939
to
d12c2fe
Compare
See #8272 for context
The HTTP.cache_control behaviour was removed in https://github.com/silverstripe/silverstripe-framework/pull/8166/files#diff-523aa2e152e10c465a85790ac13e30e4L402,
but not properly deprecated - it just silently stopped working.
This was a different approach to
HTTP.varydeprecation, which I think why it's an omission (@dhensby couldn't remember).