Merge pull request #8274 from open-sausages/pulls/4.2/cache-docs-and-deprecation-handling

Corrected caching docs and deprecation behaviour (fixes #8272)

docs/en/02_Developer_Guides/08_Performance/02_HTTP_Cache_Headers.md

@@ -8,7 +8,7 @@ summary: Set the correct HTTP cache headers for your responses.
 By default, SilverStripe sends headers which signal to HTTP caches
 that the response should be not considered cacheable.
 HTTP caches can either be intermediary caches (e.g. CDNs and proxies), or clients (e.g. browsers).
-The cache headers sent are `Cache-Control: no-store, no-cache, must-revalidate`;
+The cache headers sent are `Cache-Control: no-cache, must-revalidate`;
 
 HTTP caching can be a great way to speed up your website, but needs to be properly applied.
 Getting it wrong can accidentally expose draft pages or other protected content.
@@ -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 unless a `max-age` is set; other directives will remain in place.
+Use alongside `setMaxAge()` to activate caching.
 
 Does not set `public` directive. Usually, `setMaxAge()` is sufficient. Use `publicCache()` if this is explicitly required
 ([details](https://developers.google.com/web/fundamentals/performance/optimizing-content-efficiency/http-caching#public_vs_private))
@@ -184,24 +184,13 @@ class PageController extends ContentController
 }
 ```
 
-## Defaults
-
-By default, PHP adds caching headers that make the page appear purely dynamic. This isn't usually appropriate for most 
-sites, even ones that are updated reasonably frequently. SilverStripe overrides the default settings with the following 
-headers:
-
-  * The `Last-Modified` date is set to be most recent modification date of any database record queried in the generation 
-  of the page.
-  * The `Expiry` date is set by taking the age of the page and adding that to the current time.
-  * `Cache-Control` is set to `max-age=86400, must-revalidate`
-  * Since a visitor cookie is set, the site won't be cached by proxies.
-  * Ajax requests are never cached.
-
 ## Max Age
 
 The cache age determines the lifetime of your cache, in seconds.
 It only takes effect if you instruct the cache control
-that your response is cacheable in the first place (via `enableCache()` or via modifying the `HTTP.cache_control` defaults).
+that your response is cacheable in the first place
+(via `enableCache()`, `publicCache()` or `privateCache()`), 
+or via modifying the `HTTP.cache_control` defaults).
 
 ```php
 use SilverStripe\Control\Middleware\HTTPCacheControlMiddleware;
@@ -209,7 +198,8 @@ HTTPCacheControlMiddleware::singleton()
     ->setMaxAge(60)
 ```
 
-Note that `setMaxAge(0)` is NOT sufficient to disable caching in all cases.
+Note that `setMaxAge(0)` is NOT sufficient to disable caching in all cases,
+use `disableCache()` instead.
 
 ### Last Modified
 

src/Control/HTTP.php

@@ -71,6 +71,7 @@ class HTTP
     /**
      * List of names to add to the Cache-Control header.
      *
+     * @deprecated 4.2..5.0 Handled by HTTPCacheControlMiddleware instead
      * @see HTTPCacheControlMiddleware::__construct()
      * @config
      * @var array Keys are cache control names, values are boolean flags
@@ -80,7 +81,7 @@ class HTTP
     /**
      * Vary string; A comma separated list of var header names
      *
-     * @deprecated 4.2..5.0 Handled by HTTPCacheMiddleware instead
+     * @deprecated 4.2..5.0 Handled by HTTPCacheControlMiddleware instead
      * @config
      * @var string|null
      */
@@ -473,6 +474,7 @@ class HTTP
      * Ensure that all deprecated HTTP cache settings are respected
      *
      * @deprecated 4.2..5.0 Use HTTPCacheControlMiddleware instead
+     * @throws \LogicException
      * @param HTTPRequest $request
      * @param HTTPResponse $response
      */
@@ -509,6 +511,37 @@ class HTTP
             $cacheControlMiddleware->addVary($configVary);
         }
 
+        // Pass cache_control to middleware
+        $configCacheControl = $config->get('cache_control');
+        if ($configCacheControl) {
+            Deprecation::notice('5.0', 'Use HTTPCacheControlMiddleware API instead');
+
+            $supportedDirectives = ['max-age', 'no-cache', 'no-store', 'must-revalidate'];
+            if ($foundUnsupported = array_diff(array_keys($configCacheControl), $supportedDirectives)) {
+                throw new \LogicException(
+                    'Found unsupported legacy directives in HTTP.cache_control: ' .
+                    implode(', ', $foundUnsupported) .
+                    '. Please use HTTPCacheControlMiddleware API instead'
+                );
+            }
+
+            if (isset($configCacheControl['max-age'])) {
+                $cacheControlMiddleware->setMaxAge($configCacheControl['max-age']);
+            }
+
+            if (isset($configCacheControl['no-cache'])) {
+                $cacheControlMiddleware->setNoCache((bool)$configCacheControl['no-cache']);
+            }
+
+            if (isset($configCacheControl['no-store'])) {
+                $cacheControlMiddleware->setNoStore((bool)$configCacheControl['no-store']);
+            }
+
+            if (isset($configCacheControl['must-revalidate'])) {
+                $cacheControlMiddleware->setMustRevalidate((bool)$configCacheControl['must-revalidate']);
+            }
+        }
+
         // Set modification date
         if (self::$modification_date) {
             Deprecation::notice('5.0', 'Use HTTPCacheControlMiddleware::registerModificationDate() instead');

src/Control/Middleware/HTTPCacheControlMiddleware.php

@@ -507,7 +507,8 @@ class HTTPCacheControlMiddleware implements HTTPMiddleware, Resettable
     /**
      * Specifies the maximum amount of time (seconds) a resource will be considered fresh.
      * This directive is relative to the time of the request.
-     * Affects all non-disabled states. Use setStateDirective() instead to set for a single state.
+     * Affects all non-disabled states. Use enableCache(), publicCache() or
+     * setStateDirective() instead to set the max age for a single state.
      *
      * @param int $age
      * @return $this
@@ -560,6 +561,7 @@ class HTTPCacheControlMiddleware implements HTTPMiddleware, Resettable
 
     /**
      * Simple way to set cache control header to a cacheable state.
+     * Needs either `setMaxAge()` or the `$maxAge` method argument in order to activate caching.
      *
      * The resulting cache-control headers will be chosen from the 'enabled' set of directives.
      *
@@ -568,14 +570,20 @@ class HTTPCacheControlMiddleware implements HTTPMiddleware, Resettable
      *
      * @see https://docs.silverstripe.org/en/developer_guides/performance/http_cache_headers/
      * @param bool $force Force the cache to public even if its unforced private or public
+     * @param int $maxAge Shortcut for `setMaxAge()`, which is required to actually enable the cache.
      * @return $this
      */
-    public function enableCache($force = false)
+    public function enableCache($force = false, $maxAge = null)
     {
         // Only execute this if its forcing level is high enough
         if ($this->applyChangeLevel(self::LEVEL_ENABLED, $force)) {
             $this->setState(self::STATE_ENABLED);
         }
+
+        if (!is_null($maxAge)) {
+            $this->setMaxAge($maxAge);
+        }
+
         return $this;
     }
 
@@ -627,20 +635,27 @@ class HTTPCacheControlMiddleware implements HTTPMiddleware, Resettable
 
     /**
      * Advanced way to set cache control header to a cacheable state.
-     * Indicates that the response may be cached by any cache. (eg: CDNs, Proxies, Web browsers)
+     * Indicates that the response may be cached by any cache. (eg: CDNs, Proxies, Web browsers).
+     * Needs either `setMaxAge()` or the `$maxAge` method argument in order to activate caching.
      *
      * The resulting cache-control headers will be chosen from the 'private' set of directives.
      *
      * @see https://docs.silverstripe.org/en/developer_guides/performance/http_cache_headers/
      * @param bool $force Force the cache to public even if it's private, unless it's been forced private
+     * @param int $maxAge Shortcut for `setMaxAge()`, which is required to actually enable the cache.
      * @return $this
      */
-    public function publicCache($force = false)
+    public function publicCache($force = false, $maxAge = null)
     {
         // Only execute this if its forcing level is high enough
         if ($this->applyChangeLevel(self::LEVEL_PUBLIC, $force)) {
             $this->setState(self::STATE_PUBLIC);
         }
+
+        if (!is_null($maxAge)) {
+            $this->setMaxAge($maxAge);
+        }
+
         return $this;
     }
 

src/ORM/DataObject.php

@@ -6,8 +6,6 @@ use BadMethodCallException;
 use Exception;
 use InvalidArgumentException;
 use LogicException;
-use SilverStripe\Control\HTTP;
-use SilverStripe\Control\Middleware\HTTPCacheControlMiddleware;
 use SilverStripe\Core\ClassInfo;
 use SilverStripe\Core\Config\Config;
 use SilverStripe\Core\Injector\Injector;

tests/php/Control/HTTPTest.php

@@ -9,6 +9,7 @@ use SilverStripe\Control\HTTPRequest;
 use SilverStripe\Control\HTTPResponse;
 use SilverStripe\Control\Middleware\HTTPCacheControlMiddleware;
 use SilverStripe\Control\Session;
+use SilverStripe\Core\Config\Config;
 use SilverStripe\Dev\FunctionalTest;
 
 /**
@@ -112,6 +113,79 @@ class HTTPTest extends FunctionalTest
         $this->assertEmpty($v);
     }
 
+    public function testDeprecatedVaryHandling()
+    {
+        /** @var Config */
+        Config::modify()->set(
+            HTTP::class,
+            'vary',
+            'X-Foo'
+        );
+        $response = new HTTPResponse('', 200);
+        $this->addCacheHeaders($response);
+        $header = $response->getHeader('Vary');
+        $this->assertContains('X-Foo', $header);
+    }
+
+    public function testDeprecatedCacheControlHandling()
+    {
+        HTTPCacheControlMiddleware::singleton()->publicCache();
+
+        /** @var Config */
+        Config::modify()->set(
+            HTTP::class,
+            'cache_control',
+            [
+                'no-store' => true,
+                'no-cache' => true,
+            ]
+        );
+        $response = new HTTPResponse('', 200);
+        $this->addCacheHeaders($response);
+        $header = $response->getHeader('Cache-Control');
+        $this->assertContains('no-store', $header);
+        $this->assertContains('no-cache', $header);
+    }
+
+    public function testDeprecatedCacheControlHandlingOnMaxAge()
+    {
+        HTTPCacheControlMiddleware::singleton()->publicCache();
+
+        /** @var Config */
+        Config::modify()->set(
+            HTTP::class,
+            'cache_control',
+            [
+                // Needs to be separate from no-cache and no-store,
+                // since that would unset max-age
+                'max-age' => 99,
+            ]
+        );
+        $response = new HTTPResponse('', 200);
+        $this->addCacheHeaders($response);
+        $header = $response->getHeader('Cache-Control');
+        $this->assertContains('max-age=99', $header);
+    }
+
+    /**
+     * @expectedException \LogicException
+     * @expectedExceptionMessageRegExp /Found unsupported legacy directives in HTTP\.cache_control: unknown/
+     */
+    public function testDeprecatedCacheControlHandlingThrowsWithUnknownDirectives()
+    {
+        /** @var Config */
+        Config::modify()->set(
+            HTTP::class,
+            'cache_control',
+            [
+                'no-store' => true,
+                'unknown' => true,
+            ]
+        );
+        $response = new HTTPResponse('', 200);
+        $this->addCacheHeaders($response);
+    }
+
     /**
      * Tests {@link HTTP::getLinksIn()}
      */

tests/php/Control/Middleware/HTTPCacheControlMiddlewareTest.php

@@ -68,6 +68,65 @@ class HTTPCacheControlMiddlewareTest extends SapphireTest
         }
     }
 
+    public function testEnableCacheWithMaxAge()
+    {
+        $maxAge = 300;
+
+        $cc = HTTPCacheControlMiddleware::singleton();
+        $cc->enableCache(false, $maxAge);
+
+        $response = new HTTPResponse();
+        $cc->applyToResponse($response);
+
+        $this->assertContains('max-age=300', $response->getHeader('cache-control'));
+        $this->assertNotContains('no-cache', $response->getHeader('cache-control'));
+        $this->assertNotContains('no-store', $response->getHeader('cache-control'));
+    }
+
+    public function testEnableCacheWithMaxAgeAppliesWhenLevelDoesNot()
+    {
+        $maxAge = 300;
+
+        $cc = HTTPCacheControlMiddleware::singleton();
+        $cc->privateCache(true);
+        $cc->enableCache(false, $maxAge);
+
+        $response = new HTTPResponse();
+        $cc->applyToResponse($response);
+
+        $this->assertContains('max-age=300', $response->getHeader('cache-control'));
+    }
+
+    public function testPublicCacheWithMaxAge()
+    {
+        $maxAge = 300;
+
+        $cc = HTTPCacheControlMiddleware::singleton();
+        $cc->publicCache(false, $maxAge);
+
+        $response = new HTTPResponse();
+        $cc->applyToResponse($response);
+
+        $this->assertContains('max-age=300', $response->getHeader('cache-control'));
+        // STATE_PUBLIC doesn't contain no-cache or no-store headers to begin with,
+        // so can't test their removal effectively
+        $this->assertNotContains('no-cache', $response->getHeader('cache-control'));
+    }
+
+    public function testPublicCacheWithMaxAgeAppliesWhenLevelDoesNot()
+    {
+        $maxAge = 300;
+
+        $cc = HTTPCacheControlMiddleware::singleton();
+        $cc->privateCache(true);
+        $cc->publicCache(false, $maxAge);
+
+        $response = new HTTPResponse();
+        $cc->applyToResponse($response);
+
+        $this->assertContains('max-age=300', $response->getHeader('cache-control'));
+    }
+
     /**
      * @dataProvider provideCacheStates
      */