Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Open
ayyoub-afwallah wants to merge1 commit intosymfony:7.3
base:7.3
Choose a base branch
Loading
fromayyoub-afwallah:fix/cache-attribute-overrides-noStore

Conversation

@ayyoub-afwallah
Copy link
Contributor

QA
Branch?7.3
Bug fix?yes
New feature?no
Deprecations?no
Issues-
LicenseMIT

Related to#62488

Using no-store still overrides the controller

#[Cache(noStore:false)]publicfunctionindex():Response{$response =newResponse();$response->headers->addCacheControlDirective('no-store');return$response;}

Expected :Cache-Control : no-store, private
Result :Cache-Control : no-cache, private

@carsonbotcarsonbot added this to the7.3 milestoneDec 9, 2025
@carsonbotcarsonbot changed the title[HttpKernel] Fix#[Cache] overrides controller no-store directive[Cache][HttpKernel] Fix# overrides controller no-store directiveDec 9, 2025
@nicolas-grekas
Copy link
Member

As I wrote on the linked PR:

Note that I didn't merge the check for no-store. It makes no sense to me.

So thanks for opening if this needs to be discussed.

Here is my reasoning:

#[Cache(noStore: false)] means: removeno-store from responses. That's what the implementation does indeed.
That "remove" semantic means there is something to be removed. Unless I missed something, no-store is not part of the defaults. Which mean if it's present, it was done explicitly. This PR is conflicting with this definition.

@ayyoub-afwallahayyoub-afwallah changed the title[Cache][HttpKernel] Fix# overrides controller no-store directive[Cache][HttpKernel] Fix#[Cache] overrides controller no-store directiveDec 13, 2025
@ayyoub-afwallah
Copy link
ContributorAuthor

Here is my reasoning:

#[Cache(noStore: false)] means: removeno-store from responses. That's what the implementation does indeed. That "remove" semantic means there is something to be removed. Unless I missed something, no-store is not part of the defaults. Which mean if it's present, it was done explicitly. This PR is conflicting with this definition.

Thanks@nicolas-grekas !
I understand the concern, but the #[Cache] attribute's own documentation says:

* Describes the default HTTP cache headers on controllers.
* Headers defined in the Cache attribute are ignored if they are already set
* by the controller.

Attributes define defaults, controllers have the final say, at least that's what I understood.

When a controller explicitly addsno-store at runtime, attributes shouldn't remove it because it's an intentional decision based on dynamic conditions that should be respected, just like the other directives.

@nicolas-grekas
Copy link
Member

@smnandre can you help us on this one since you added support for no-store on the cache attribute?

smnandre reacted with eyes emoji

@smnandre
Copy link
Member

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
Copy link
Member

@ayyoub-afwallah I'm curious too to understand the use case for

#[Cache(noStore: false)]public function index(): Response{    $response = new Response();    $response->headers->addCacheControlDirective('no-store');        return $response;}

Here it would make no difference if you removed the attribute entirely.

@ayyoub-afwallah
Copy link
ContributorAuthor

@smnandre thanks for the feedback!

I think the issue here is simpler than it looks, and it comes down to consistency and intent.

noStore: false almost has no real-world use case scenario:

#[Cache(maxage:3600)]#[Cache(maxage:3600, noStore:false)]// exactly the same state

So the only timenoStore: false has any effect today is when it strips ano-store directive that the controller explicitly added.

#[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:

Headers defined in the Cache attribute are ignored if they are already set

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
Copy link
Member

To be honest, I understand thetechnical point, and yet I still don't see the problem.

As you said,

noStore: false almost has no real-world use case scenario
(...)
The only time noStore: false has any effect today is when it strips a no-store directive that the controller explicitly added.

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');}

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

7.3

Development

Successfully merging this pull request may close these issues.

4 participants

@ayyoub-afwallah@nicolas-grekas@smnandre@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp