Skip to content

Commit 1083e8a

Browse files
nicolas-grekasnicolas-grekas
committed
bug #62488 [HttpKernel] Make #[Cache] respect all explicit cache directives set in controller (ayyoub-afwallah)
This PR was merged into the 6.4 branch. Discussion ---------- [HttpKernel] Make `#[Cache]` respect all explicit cache directives set in controller | Q | A | ------------- | --- | Branch? | 7.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix - | License | MIT **Before:** ```php #[Cache(maxage: 20, public: true, noStore: false)] public function index(): Response { $response = new Response(); $response->setMaxAge(300); $response->setPrivate(); $response->headers->addCacheControlDirective('no-store'); return $response; } ``` The previous controller will result : `Cache-Control : max-age=300, public` instead of : `Cache-Control : max-age=300, private, no-store` Controller-defined directives (private, no-store) were lost, which is unexpected and it limits the flexibility of controller-level cache control. The `#[Cache]` is meant for defining default values and shouldn't override the controller. Even the `ResponseHeaderBag::computeCacheControlValue()` respects explicit visibility but the `#[Cache]` doesn't ```php $header = $this->getCacheControlHeader(); if (isset($this->cacheControl['public']) || isset($this->cacheControl['private'])) { return $header; } ``` **After:** ```php #[Cache(public: true, maxage: 3600, noStore: false)] public function index(User $user): Response { $response = new Response($content); if ($user->shouldStoreLonger()) { $response->setMaxAge(7200); } if ($user->shouldNotStore()) { $response->headers->addCacheControlDirective('no-store'); } if ($user->shouldBePrivate()) { $response->setPrivate(); } // Otherwise, `#[Cache]` should take over if no condition was met return $response; } ``` > [!NOTE] > This PR does not change the default behavior of computed cache headers. The `#[Cache]` attribute can still override the default values, only explicitly defined controller directives take precedence. Commits ------- a6053f5 [HttpKernel] Make `#[Cache]` respect all explicit cache directives set in controller
2 parents 15c9cc8 + a6053f5 commit 1083e8a

File tree

2 files changed

+45
-7
lines changed

2 files changed

+45
-7
lines changed

‎src/Symfony/Component/HttpKernel/EventListener/CacheAttributeListener.php‎

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313

1414
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
1515
use Symfony\Component\ExpressionLanguage\ExpressionLanguage;
16+
use Symfony\Component\HttpFoundation\HeaderBag;
1617
use Symfony\Component\HttpFoundation\Request;
1718
use Symfony\Component\HttpFoundation\Response;
1819
use Symfony\Component\HttpKernel\Attribute\Cache;
@@ -126,29 +127,40 @@ public function onKernelResponse(ResponseEvent $event)
126127
// Check if the response has a Vary header that should be considered, ignoring cases where
127128
// it's only 'Accept-Language' and the request has the '_vary_by_language' attribute
128129
$hasVary = ['Accept-Language'] === $response->getVary() ? !$request->attributes->get('_vary_by_language') : $response->hasVary();
130+
//Check if cache-control directive was set manually in cacheControl (not auto computed)
131+
$hasCacheControlDirective = new class($response->headers) extends HeaderBag {
132+
public function __construct(private parent $headerBag)
133+
{
134+
}
135+
136+
public function __invoke(string $key): bool
137+
{
138+
return \array_key_exists($key, $this->headerBag->cacheControl);
139+
}
140+
};
129141

130142
foreach (array_reverse($attributes) as $cache) {
131-
if (null !== $cache->smaxage && !$response->headers->hasCacheControlDirective('s-maxage')) {
143+
if (null !== $cache->smaxage && !$hasCacheControlDirective('s-maxage')) {
132144
$response->setSharedMaxAge($this->toSeconds($cache->smaxage));
133145
}
134146

135147
if ($cache->mustRevalidate) {
136148
$response->headers->addCacheControlDirective('must-revalidate');
137149
}
138150

139-
if (null !== $cache->maxage && !$response->headers->hasCacheControlDirective('max-age')) {
151+
if (null !== $cache->maxage && !$hasCacheControlDirective('max-age')) {
140152
$response->setMaxAge($this->toSeconds($cache->maxage));
141153
}
142154

143-
if (null !== $cache->maxStale && !$response->headers->hasCacheControlDirective('max-stale')) {
155+
if (null !== $cache->maxStale && !$hasCacheControlDirective('max-stale')) {
144156
$response->headers->addCacheControlDirective('max-stale', $this->toSeconds($cache->maxStale));
145157
}
146158

147-
if (null !== $cache->staleWhileRevalidate && !$response->headers->hasCacheControlDirective('stale-while-revalidate')) {
159+
if (null !== $cache->staleWhileRevalidate && !$hasCacheControlDirective('stale-while-revalidate')) {
148160
$response->headers->addCacheControlDirective('stale-while-revalidate', $this->toSeconds($cache->staleWhileRevalidate));
149161
}
150162

151-
if (null !== $cache->staleIfError && !$response->headers->hasCacheControlDirective('stale-if-error')) {
163+
if (null !== $cache->staleIfError && !$hasCacheControlDirective('stale-if-error')) {
152164
$response->headers->addCacheControlDirective('stale-if-error', $this->toSeconds($cache->staleIfError));
153165
}
154166

@@ -161,12 +173,14 @@ public function onKernelResponse(ResponseEvent $event)
161173
}
162174
}
163175

176+
$hasPublicOrPrivateCacheControlDirective = $hasCacheControlDirective('public') || $hasCacheControlDirective('private');
177+
164178
foreach ($attributes as $cache) {
165-
if (true === $cache->public) {
179+
if (true === $cache->public && !$hasPublicOrPrivateCacheControlDirective) {
166180
$response->setPublic();
167181
}
168182

169-
if (false === $cache->public) {
183+
if (false === $cache->public && !$hasPublicOrPrivateCacheControlDirective) {
170184
$response->setPrivate();
171185
}
172186
}

‎src/Symfony/Component/HttpKernel/Tests/EventListener/CacheAttributeListenerTest.php‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -336,6 +336,30 @@ public function testHasRelevantVaryHeaderBehavior(array $responseVary, array $ca
336336
$this->assertSame($expectedVary, $response->getVary());
337337
}
338338

339+
public function testAttributeRespectsExplicitPrivateFromController()
340+
{
341+
$request = $this->createRequest(new Cache(public: true));
342+
$response = new Response();
343+
$response->setPrivate();
344+
345+
$this->listener->onKernelResponse($this->createEventMock($request, $response));
346+
347+
$this->assertTrue($response->headers->hasCacheControlDirective('private'));
348+
$this->assertFalse($response->headers->hasCacheControlDirective('public'));
349+
}
350+
351+
public function testAttributeRespectsExplicitPublicFromController()
352+
{
353+
$request = $this->createRequest(new Cache(public: false));
354+
$response = new Response();
355+
$response->setPublic();
356+
357+
$this->listener->onKernelResponse($this->createEventMock($request, $response));
358+
359+
$this->assertTrue($response->headers->hasCacheControlDirective('public'));
360+
$this->assertFalse($response->headers->hasCacheControlDirective('private'));
361+
}
362+
339363
public static function provideVaryHeaderScenarios(): \Traversable
340364
{
341365
yield 'no vary headers at all' => [

0 commit comments

Comments
 (0)