bug symfony#23129 Fix two edge cases in ResponseCacheStrategy (mpdude)

This PR was squashed before being merged into the 2.7 branch (closes symfony#23129).

Discussion
----------

Fix two edge cases in ResponseCacheStrategy

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

While reviewing how `ResponseCacheStrategy` calculates the caching-related headers for responses that embed subrequests, I came across two cases that I think are currently implemented incorrectly.

a) When the main response is public and cacheable with an expiration time, but it embeds (via ESI) a controller that does not set any caching-related headers, this embedded response is more constrained. So, the resulting (combined) response must not be cacheable, especially it may not keep the s-maxage.

b) When the main response is public and cacheable with an expiration time, but it embeds (via ESI) a controller that explicitly creates a "private" response, the resulting (combined) response must be private as well.

Commits
-------

c6e8c07 Fix two edge cases in ResponseCacheStrategy

Author: fabpot

src/Symfony/Component/HttpKernel/HttpCache/ResponseCacheStrategy.php

@@ -39,7 +39,7 @@ class ResponseCacheStrategy implements ResponseCacheStrategyInterface
      */
     public function add(Response $response)
     {
-        if ($response->isValidateable()) {
+        if ($response->isValidateable() || !$response->isCacheable()) {
             $this->cacheable = false;
         } else {
             $maxAge = $response->getMaxAge();

src/Symfony/Component/HttpKernel/Tests/HttpCache/ResponseCacheStrategyTest.php

@@ -137,4 +137,45 @@ public function testMasterResponseWithExpirationIsUnchangedWhenThereIsNoEmbedded
 
         $this->assertTrue($masterResponse->isFresh());
     }
+
+    public function testMasterResponseIsNotCacheableWhenEmbeddedResponseIsNotCacheable()
+    {
+        $cacheStrategy = new ResponseCacheStrategy();
+
+        $masterResponse = new Response();
+        $masterResponse->setSharedMaxAge(3600); // Public, cacheable
+
+        /* This response has no validation or expiration information.
+           That makes it uncacheable, it is always stale.
+           (It does *not* make this private, though.) */
+        $embeddedResponse = new Response();
+        $this->assertFalse($embeddedResponse->isFresh()); // not fresh, as no lifetime is provided
+
+        $cacheStrategy->add($embeddedResponse);
+        $cacheStrategy->update($masterResponse);
+
+        $this->assertTrue($masterResponse->headers->hasCacheControlDirective('no-cache'));
+        $this->assertTrue($masterResponse->headers->hasCacheControlDirective('must-revalidate'));
+        $this->assertFalse($masterResponse->isFresh());
+    }
+
+    public function testEmbeddingPrivateResponseMakesMainResponsePrivate()
+    {
+        $cacheStrategy = new ResponseCacheStrategy();
+
+        $masterResponse = new Response();
+        $masterResponse->setSharedMaxAge(3600); // public, cacheable
+
+        // The embedded response might for example contain per-user data that remains valid for 60 seconds
+        $embeddedResponse = new Response();
+        $embeddedResponse->setPrivate();
+        $embeddedResponse->setMaxAge(60); // this would implicitly set "private" as well, but let's be explicit
+
+        $cacheStrategy->add($embeddedResponse);
+        $cacheStrategy->update($masterResponse);
+
+        $this->assertTrue($masterResponse->headers->hasCacheControlDirective('private'));
+        // Not sure if we should pass "max-age: 60" in this case, as long as the response is private and
+        // that's the more conservative of both the master and embedded response...?
+    }
 }