Silverstripe-framework: enableCache() isn't actually enabling caching

[chillu]

Follow up on the recent cache control changes slated for 4.2.0

Overview

I believe there's a regression in there, or at least a misinterpretation of the spirit of the whole caching API. In short, the enableCache() meaning got changed halfway through the discussion, in a way which prevents both browsers and CDNs from actually caching. The headers instruct them to call back to the origin server with their If-Modified-* headers, and the server can choose to return a 304. That's not saving any server resources, since we still need to process the entire request execution. It just saves bandwidth. It's also inconsistent with our 3.x implementation.

I think it's critical that we remove no-cache from enableCache() before we call 4.2.0 stable

Context

@dhensby submitted a PR with a commit titled State default should be state enabled (no-cache is an enabled state), which @tractorcow merged into 4.2.

HttpCacheControlMiddleware::stateDirectives

    protected $stateDirectives = [
        self::STATE_DISABLED => [
            'no-cache' => true,
            'no-store' => true,
            'must-revalidate' => true,
        ],
        self::STATE_PRIVATE => [
            'private' => true,
            'must-revalidate' => true,
        ],
        self::STATE_PUBLIC => [
            'public' => true,
            'must-revalidate' => true,
        ],
        self::STATE_ENABLED => [
            'no-cache' => true,
            'must-revalidate' => true,
        ]
    ];

In the PR, he comments:

An important thing to note here is that no-cache is actually a kind of enabled state; ETags are generated and 304 responses are possible. So I've removed my "default" state which was just a soft enabled state and turned the "enabled" state to be what was the default. This means by default we will allow 304 responses, but it's for a developer to set a max-age. The framework still protects them from caching during sessions and so on.

Our docs say on enableCache():

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.

Further down we use enableCache() in a code example described as:

Global opt-in for page content
Enable caching for all page content (through PageController).

Notes

• There was a discussion about making the default state no-cache. That's still OK, since the default state is disabled.

Pull requests

• [ ] https://github.com/silverstripe/silverstripe-framework/pull/8274

[chillu]

@sminnee Keen to hear your thoughts on this

[dhensby]

So are we saying that enableCache should set a max-age? at the moment the enableCache method basically sets the cache state to enabled and then waits for the developer to set a max-age directive.

the cache control of no-cache is effectively max-age=0. When a developer wants to enable HTTP caching all they need to do is setMaxAge(<time>) and it will be enabled (because the default state is enabled)

[dhensby]

The only point about the docs that is wrong is literally the bit you emboldened:

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.

It doesn't so much remove directives anymore but changes the state to a directive that doesn't have no-store; you still have to use it alongside setMaxAge and then the no-cache setting will disappear...

Well, that's at least how it's meant to

[dhensby]

The examples in the docs are wrong now only because they call enableCache unnecessarily because the default state is not disabled any more.

[dhensby]

I just tested this in 4.2.x-dev and it seems to work as expected. If I setMaxAge in the controller I get chrome loading from disk cache and not hitting the server for validation of current cache

[chillu]

OK I worked this through with Dan and Sam:

It was an issue of docs and implementation getting out of sync, which lead me down a rabbit hole of questioning the implementation rather than the docs :wink: I’m doing some minor API and docs additions to be cherry picked into 4.2.0 (not 4.2.x)

[caffeineinc]

Yup, unless the developer defines a specific max-age, it's not up to framework to decide that for them; you can't cache _until_ you know that, so allowing 304's etc makes sense. Looks like a doc issue 👍 🌮

I guess we should also make it clear that enableCache() has to be used with setMaxAge or you're not going to get HTTP caching.

[dhensby]

glad we got to the bottom of this :)

[chillu]

PR at https://github.com/silverstripe/silverstripe-framework/pull/8274