[HttpKernel] Bugfix/last modified response strategy

Author: aschempp

HttpCache/ResponseCacheStrategy.php

@@ -37,6 +37,7 @@ class ResponseCacheStrategy implements ResponseCacheStrategyInterface
     private int $embeddedResponses = 0;
     private bool $isNotCacheableResponseEmbedded = false;
     private int $age = 0;
+    private \DateTimeInterface|null|false $lastModified = null;
     private array $flagDirectives = [
         'no-cache' => null,
         'no-store' => null,
@@ -90,6 +91,11 @@ public function add(Response $response)
         $expires = $response->getExpires();
         $expires = null !== $expires ? (int) $expires->format('U') - (int) $response->getDate()->format('U') : null;
         $this->storeRelativeAgeDirective('expires', $expires >= 0 ? $expires : null, 0, $isHeuristicallyCacheable);
+
+        if (false !== $this->lastModified) {
+            $lastModified = $response->getLastModified();
+            $this->lastModified = $lastModified ? max($this->lastModified, $lastModified) : false;
+        }
     }
 
     /**
@@ -102,17 +108,16 @@ public function update(Response $response)
             return;
         }
 
-        // Remove validation related headers of the master response,
-        // because some of the response content comes from at least
-        // one embedded response (which likely has a different caching strategy).
+        // Remove Etag since it cannot be merged from embedded responses.
         $response->setEtag(null);
-        $response->setLastModified(null);
 
         $this->add($response);
 
         $response->headers->set('Age', $this->age);
 
         if ($this->isNotCacheableResponseEmbedded) {
+            $response->setLastModified();
+
             if ($this->flagDirectives['no-store']) {
                 $response->headers->set('Cache-Control', 'no-cache, no-store, must-revalidate');
             } else {
@@ -122,6 +127,8 @@ public function update(Response $response)
             return;
         }
 
+        $response->setLastModified($this->lastModified ?: null);
+
         $flags = array_filter($this->flagDirectives);
 
         if (isset($flags['must-revalidate'])) {
@@ -162,17 +169,14 @@ private function willMakeFinalResponseUncacheable(Response $response): bool
         // RFC2616: A response received with a status code of 200, 203, 300, 301 or 410
         // MAY be stored by a cache […] unless a cache-control directive prohibits caching.
         if ($response->headers->hasCacheControlDirective('no-cache')
-            || $response->headers->getCacheControlDirective('no-store')
+            || $response->headers->hasCacheControlDirective('no-store')
         ) {
             return true;
         }
 
-        // Last-Modified and Etag headers cannot be merged, they render the response uncacheable
+        // Etag headers cannot be merged, they render the response uncacheable
         // by default (except if the response also has max-age etc.).
-        if (\in_array($response->getStatusCode(), [200, 203, 300, 301, 410])
-            && null === $response->getLastModified()
-            && null === $response->getEtag()
-        ) {
+        if (null === $response->getEtag() && \in_array($response->getStatusCode(), [200, 203, 300, 301, 410])) {
             return false;
         }
 

Tests/HttpCache/ResponseCacheStrategyTest.php

@@ -5,10 +5,6 @@
  *
  * (c) Fabien Potencier <fabien@symfony.com>
  *
- * This code is partially based on the Rack-Cache library by Ryan Tomayko,
- * which is released under the MIT license.
- * (based on commit 02d2b48d75bcb63cf1c0c7149c077ad256542801)
- *
  * For the full copyright and license information, please view the LICENSE
  * file that was distributed with this source code.
  */
@@ -138,6 +134,28 @@ public function testMainResponseWithExpirationIsUnchangedWhenThereIsNoEmbeddedRe
         $this->assertTrue($mainResponse->isFresh());
     }
 
+    public function testLastModifiedIsMergedWithEmbeddedResponse()
+    {
+        $cacheStrategy = new ResponseCacheStrategy();
+
+        $embeddedDate = new \DateTime('-1 hour');
+
+        // This master response uses the "validation" model
+        $masterResponse = new Response();
+        $masterResponse->setLastModified(new \DateTime('-2 hour'));
+        $masterResponse->setEtag('foo');
+
+        // Embedded response uses "expiry" model
+        $embeddedResponse = new Response();
+        $embeddedResponse->setLastModified($embeddedDate);
+        $cacheStrategy->add($embeddedResponse);
+
+        $cacheStrategy->update($masterResponse);
+
+        $this->assertTrue($masterResponse->isValidateable());
+        $this->assertSame($embeddedDate->getTimestamp(), $masterResponse->getLastModified()->getTimestamp());
+    }
+
     public function testMainResponseIsNotCacheableWhenEmbeddedResponseIsNotCacheable()
     {
         $cacheStrategy = new ResponseCacheStrategy();