Skip to content

Conversation

@ayyoub-afwallah
Copy link
Contributor

Q A
Branch? 7.3
Bug fix? yes
New feature? no
Deprecations? no
Issues -
License MIT

Related to #62488

Using no-store still overrides the controller

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

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

@carsonbot carsonbot added this to the 7.3 milestone Dec 9, 2025
@carsonbot carsonbot changed the title [HttpKernel] Fix #[Cache] overrides controller no-store directive [Cache][HttpKernel] Fix # overrides controller no-store directive Dec 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: remove no-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-afwallah ayyoub-afwallah changed the title [Cache][HttpKernel] Fix # overrides controller no-store directive [Cache][HttpKernel] Fix #[Cache] overrides controller no-store directive Dec 13, 2025
@ayyoub-afwallah
Copy link
Contributor Author

Here is my reasoning:

#[Cache(noStore: false)] means: remove no-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 adds no-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
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.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants