Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.8k
[Cache][HttpKernel] Fix#[Cache] overrides controller no-store directive#62708
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:7.3
Are you sure you want to change the base?
[Cache][HttpKernel] Fix#[Cache] overrides controller no-store directive#62708
Conversation
#[Cache] overrides controller no-store directive# overrides controller no-store directivenicolas-grekas commentedDec 9, 2025
As I wrote on the linked PR:
So thanks for opening if this needs to be discussed. Here is my reasoning:
|
# overrides controller no-store directive#[Cache] overrides controller no-store directiveayyoub-afwallah commentedDec 13, 2025
Thanks@nicolas-grekas ! symfony/src/Symfony/Component/HttpKernel/Attribute/Cache.php Lines 15 to 17 in0dd7d93
Attributes define defaults, controllers have the final say, at least that's what I understood. When a controller explicitly adds |
nicolas-grekas commentedDec 13, 2025
@smnandre can you help us on this one since you added support for no-store on the cache attribute? |
smnandre commentedDec 13, 2025
I'm trying to get a good comprehension of the issue here, reading the changes made since. #62488 without something similar for no-store could not bring coherent results now indeed. But I don't see how checking the header bag in foreach.. loops could be fully working, as multiple response setter act on two different headers, so one may just be decided by the other. That beein said, I agree I did not implement it as "default header".. as I in fact never anticipate the usage of setting no-stiore "per default" and remove it explicitely later. But that was the same for public/private, no ? |
smnandre commentedDec 13, 2025
@ayyoub-afwallah I'm curious too to understand the use case for Here it would make no difference if you removed the attribute entirely. |
ayyoub-afwallah commentedDec 15, 2025
@smnandre thanks for the feedback! I think the issue here is simpler than it looks, and it comes down to consistency and intent.
#[Cache(maxage:3600)]#[Cache(maxage:3600, noStore:false)]// exactly the same state So the only time #[Cache(maxage:3600, noStore:false)]// Implicitly: "Allow caching"publicfunctionshow(Article$article):Response{// ...// Explicit runtime decision: "Don't cache this specific response"if ($article->isDraft() ||$article->isPrivate()) {$response->headers->addCacheControlDirective('no-store');// <- Developer's clear intent }return$response;// no-store is silently stripped by the attribute} This contradicts both the documented behavior and the established pattern for all other directives, i don’t see a clear reason why no-store should be the exception. The best way for attribute and controller caching to coexist is to follow the existing documentation:
The attribute defines the happy-path (or fallback) configuration, while the controller makes a contextual runtime decision based on information only available at execution time. Overriding that explicit decision is surprising and breaks the mental model already established by all other cache directives. |
smnandre commentedDec 15, 2025
To be honest, I understand thetechnical point, and yet I still don't see the problem. As you said,
Do you have anyreal-life scenario where someone would do this ? #[Cache(maxage:3600, noStore:false)]publicfunctionshow(Article$article):Response{// ...$response->headers->addCacheControlDirective('no-store');} instead of this #[Cache(maxage:3600)]publicfunctionshow(Article$article):Response{// ...$response->headers->addCacheControlDirective('no-store');} |
Related to#62488
Using no-store still overrides the controller
Expected :
Cache-Control : no-store, privateResult :
Cache-Control : no-cache, private