Issue #2476407 by borisson_, hussainweb, znerol, Fabianx, Wim Leers, dawehner,...

Issue #2476407 by borisson_, hussainweb, znerol, Fabianx, Wim Leers, dawehner, Crell, Berdir: Use CacheableResponseInterface to determine which responses should be cached

core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php

@@ -115,13 +115,30 @@ public function onRespond(FilterResponseEvent $event) {
     $response->headers->set('X-Content-Type-Options', 'nosniff', FALSE);
     $response->headers->set('X-Frame-Options', 'SAMEORIGIN', FALSE);
 
+    // If the current response isn't an implementation of the
+    // CacheableResponseInterface, we assume that a Response is either
+    // explicitly not cacheable or that caching headers are already set in
+    // another place.
+    if (!$response instanceof CacheableResponseInterface) {
+      if (!$this->isCacheControlCustomized($response)) {
+        $this->setResponseNotCacheable($response, $request);
+      }
+
+      // HTTP/1.0 proxies do not support the Vary header, so prevent any caching
+      // by sending an Expires date in the past. HTTP/1.1 clients ignore the
+      // Expires header if a Cache-Control: max-age directive is specified (see
+      // RFC 2616, section 14.9.3).
+      if (!$response->headers->has('Expires')) {
+        $this->setExpiresNoCache($response);
+      }
+      return;
+    }
+
     // Expose the cache contexts and cache tags associated with this page in a
     // X-Drupal-Cache-Contexts and X-Drupal-Cache-Tags header respectively.
-    if ($response instanceof CacheableResponseInterface) {
-      $response_cacheability = $response->getCacheableMetadata();
-      $response->headers->set('X-Drupal-Cache-Tags', implode(' ', $response_cacheability->getCacheTags()));
-      $response->headers->set('X-Drupal-Cache-Contexts', implode(' ', $this->cacheContextsManager->optimizeTokens($response_cacheability->getCacheContexts())));
-    }
+    $response_cacheability = $response->getCacheableMetadata();
+    $response->headers->set('X-Drupal-Cache-Tags', implode(' ', $response_cacheability->getCacheTags()));
+    $response->headers->set('X-Drupal-Cache-Contexts', implode(' ', $this->cacheContextsManager->optimizeTokens($response_cacheability->getCacheContexts())));
 
     $is_cacheable = ($this->requestPolicy->check($request) === RequestPolicyInterface::ALLOW) && ($this->responsePolicy->check($response, $request) !== ResponsePolicyInterface::DENY);
 

core/modules/image/src/Controller/ImageStyleDownloadController.php

@@ -178,7 +178,11 @@ public function deliver(Request $request, $scheme, ImageStyleInterface $image_st
         'Content-Type' => $image->getMimeType(),
         'Content-Length' => $image->getFileSize(),
       );
-      return new BinaryFileResponse($uri, 200, $headers);
+      // \Drupal\Core\EventSubscriber\FinishResponseSubscriber::onRespond()
+      // sets response as not cacheable if the Cache-Control header is not
+      // already modified. We pass in FALSE for non-private schemes for the
+      // $public parameter to make sure we don't change the headers.
+      return new BinaryFileResponse($uri, 200, $headers, $scheme !== 'private');
     }
     else {
       $this->logger->notice('Unable to generate the derived image located at %path.', array('%path' => $derivative_uri));

core/modules/image/src/Tests/ImageStylesPathAndUrlTest.php

@@ -196,10 +196,15 @@ function doImageStyleUrlAndPathTests($scheme, $clean_url = TRUE, $extra_slash =
         $this->assertNoRaw( chr(137) . chr(80) . chr(78) . chr(71) . chr(13) . chr(10) . chr(26) . chr(10), 'No PNG signature found in the response body.');
       }
     }
-    elseif ($clean_url) {
-      // Add some extra chars to the token.
-      $this->drupalGet(str_replace(IMAGE_DERIVATIVE_TOKEN . '=', IMAGE_DERIVATIVE_TOKEN . '=Zo', $generate_url));
-      $this->assertResponse(200, 'Existing image was accessible at the URL with an invalid token.');
+    else {
+      $this->assertEqual($this->drupalGetHeader('Expires'), 'Sun, 19 Nov 1978 05:00:00 GMT', 'Expires header was sent.');
+      $this->assertEqual(strpos($this->drupalGetHeader('Cache-Control'), 'no-cache'), FALSE, 'Cache-Control header contains \'no-cache\' to prevent caching.');
+
+      if ($clean_url) {
+        // Add some extra chars to the token.
+        $this->drupalGet(str_replace(IMAGE_DERIVATIVE_TOKEN . '=', IMAGE_DERIVATIVE_TOKEN . '=Zo', $generate_url));
+        $this->assertResponse(200, 'Existing image was accessible at the URL with an invalid token.');
+      }
     }
 
     // Allow insecure image derivatives to be created for the remainder of this

core/modules/page_cache/src/Tests/PageCacheTest.php

@@ -406,4 +406,50 @@ public function testFormImmutability() {
     $this->assertText("Immutable: FALSE", "Form is not immutable,");
   }
 
+  /**
+   * Tests cacheability of a CacheableResponse.
+   *
+   * Tests the difference between having a controller return a plain Symfony
+   * Response object versus returning a Response object that implements the
+   * CacheableResponseInterface.
+   */
+  public function testCacheableResponseResponses() {
+    $config = $this->config('system.performance');
+    $config->set('cache.page.max_age', 300);
+    $config->save();
+
+    // Try to fill the cache.
+    $this->drupalGet('/system-test/respond-reponse');
+    $this->assertFalse(in_array('X-Drupal-Cache', $this->drupalGetHeaders()), 'Drupal page cache header not found');
+    $this->assertEqual($this->drupalGetHeader('Cache-Control'), 'must-revalidate, no-cache, post-check=0, pre-check=0, private', 'Cache-Control header was sent');
+
+    // Still not cached, uncacheable response.
+    $this->drupalGet('/system-test/respond-reponse');
+    $this->assertFalse(in_array('X-Drupal-Cache', $this->drupalGetHeaders()), 'Drupal page cache header not found');
+    $this->assertEqual($this->drupalGetHeader('Cache-Control'), 'must-revalidate, no-cache, post-check=0, pre-check=0, private', 'Cache-Control header was sent');
+
+    // Try to fill the cache.
+    $this->drupalGet('/system-test/respond-cacheable-reponse');
+    $this->assertEqual($this->drupalGetHeader('X-Drupal-Cache'), 'MISS', 'Page was not cached.');
+    $this->assertEqual($this->drupalGetHeader('Cache-Control'), 'max-age=300, public', 'Cache-Control header was sent.');
+
+    // Should be cached now.
+    $this->drupalGet('/system-test/respond-cacheable-reponse');
+    $this->assertEqual($this->drupalGetHeader('X-Drupal-Cache'), 'HIT', 'Page was cached.');
+    $this->assertEqual($this->drupalGetHeader('Cache-Control'), 'max-age=300, public', 'Cache-Control header was sent.');
+
+    // Uninstall page cache. This should flush all caches so the next call to a
+    // previously cached page should be a miss now.
+    $this->container->get('module_installer')
+      ->uninstall(['page_cache']);
+
+    // Try to fill the cache.
+    $this->drupalGet('/system-test/respond-reponse');
+    $this->assertEqual($this->drupalGetHeader('Cache-Control'), 'must-revalidate, no-cache, post-check=0, pre-check=0, private', 'Cache-Control header was sent');
+
+    // Still not cached, uncacheable response.
+    $this->drupalGet('/system-test/respond-reponse');
+    $this->assertEqual($this->drupalGetHeader('Cache-Control'), 'must-revalidate, no-cache, post-check=0, pre-check=0, private', 'Cache-Control header was sent');
+  }
+
 }

core/modules/system/src/FileDownloadController.php

@@ -59,7 +59,11 @@ public function download(Request $request, $scheme = 'private') {
       }
 
       if (count($headers)) {
-        return new BinaryFileResponse($uri, 200, $headers);
+        // \Drupal\Core\EventSubscriber\FinishResponseSubscriber::onRespond()
+        // sets response as not cacheable if the Cache-Control header is not
+        // already modified. We pass in FALSE for non-private schemes for the
+        // $public parameter to make sure we don't change the headers.
+        return new BinaryFileResponse($uri, 200, $headers, $scheme !== 'private');
       }
 
       throw new AccessDeniedHttpException();

core/modules/system/tests/modules/system_test/src/Controller/PageCacheAcceptHeaderController.php

@@ -7,9 +7,9 @@
 
 namespace Drupal\system_test\Controller;
 
+use Drupal\Core\Cache\CacheableJsonResponse;
+use Drupal\Core\Cache\CacheableResponse;
 use Symfony\Component\HttpFoundation\Request;
-use Symfony\Component\HttpFoundation\JsonResponse;
-use Symfony\Component\HttpFoundation\Response;
 
 /**
  * Defines a controller to respond the page cache accept header test.
@@ -26,10 +26,10 @@ class PageCacheAcceptHeaderController {
    */
   public function content(Request $request) {
     if ($request->getRequestFormat() === 'json') {
-      return new JsonResponse(array('content' => 'oh hai this is json'));
+      return new CacheableJsonResponse(['content' => 'oh hai this is json']);
     }
     else {
-      return new Response("<p>oh hai this is html.</p>");
+      return new CacheableResponse("<p>oh hai this is html.</p>");
     }
   }
 }

core/modules/system/tests/modules/system_test/src/Controller/SystemTestController.php

@@ -8,6 +8,7 @@
 namespace Drupal\system_test\Controller;
 
 use Drupal\Core\Access\AccessResult;
+use Drupal\Core\Cache\CacheableResponse;
 use Drupal\Core\Controller\ControllerBase;
 use Drupal\Core\Render\RendererInterface;
 use Drupal\Core\Render\Markup;
@@ -259,13 +260,28 @@ public function authorizeInit($page_title) {
    */
   public function setHeader(Request $request) {
     $query = $request->query->all();
-    $response = new Response();
+    $response = new CacheableResponse();
     $response->headers->set($query['name'], $query['value']);
+    $response->getCacheableMetadata()->addCacheContexts(['url.query_args:name', 'url.query_args:value']);
     $response->setContent($this->t('The following header was set: %name: %value', array('%name' => $query['name'], '%value' => $query['value'])));
 
     return $response;
   }
 
+  /**
+   * A simple page callback that uses a plain Symfony response object.
+   */
+  public function respondWithReponse(Request $request) {
+    return new Response('test');
+  }
+
+  /**
+   * A simple page callback that uses a CacheableResponse object.
+   */
+  public function respondWithCacheableReponse(Request $request) {
+    return new CacheableResponse('test');
+  }
+
   /**
    * A simple page callback which adds a register shutdown function.
    */

core/modules/system/tests/modules/system_test/system_test.routing.yml

@@ -129,3 +129,17 @@ system_test.permission_dependent_route_access:
     _controller: '\Drupal\system_test\Controller\SystemTestController::mainContentFallback'
   requirements:
     _permission: 'pet llamas'
+
+system_test.respond_response:
+  path: '/system-test/respond-reponse'
+  defaults:
+    _controller: '\Drupal\system_test\Controller\SystemTestController::respondWithReponse'
+  requirements:
+    _access: 'TRUE'
+
+system_test.respond_cacheable_response:
+  path: '/system-test/respond-cacheable-reponse'
+  defaults:
+    _controller: '\Drupal\system_test\Controller\SystemTestController::respondWithCacheableReponse'
+  requirements:
+    _access: 'TRUE'