Skip to content

Conversation

@ayyoub-afwallah
Copy link
Contributor

@ayyoub-afwallah ayyoub-afwallah commented Nov 23, 2025

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

Before:

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

$header = $this->getCacheControlHeader();
if (isset($this->cacheControl['public']) || isset($this->cacheControl['private'])) {
     return $header;
}

After:

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

@carsonbot carsonbot added this to the 7.4 milestone Nov 23, 2025
@ayyoub-afwallah ayyoub-afwallah force-pushed the fix/cache-attribute-overrides-controller branch 2 times, most recently from 509ac93 to 28ddacc Compare November 23, 2025 12:15
@ayyoub-afwallah ayyoub-afwallah changed the title [HttpKernel] Make respect all explicit cache directives set in contr… [HttpKernel] Make #[Cache] respect all explicit cache directives set in contr… Nov 23, 2025
@stof
Copy link
Member

stof commented Nov 23, 2025

I would consider it as a new feature, and so it should target the 8.1 branch.

@ayyoub-afwallah
Copy link
Contributor Author

I would consider it as a new feature, and so it should target the 8.1 branch.

I thought it was worth fixing in 7.4, but if you want to keep it for 8.1, why not.

@nicolas-grekas, what do you think?

@nicolas-grekas nicolas-grekas changed the title [HttpKernel] Make #[Cache] respect all explicit cache directives set in contr… [HttpKernel] Make #[Cache] respect all explicit cache directives set in controller Nov 24, 2025
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Nov 25, 2025
… (ayyoub-afwallah)

This PR was submitted for the 7.4 branch but it was merged into the 6.4 branch instead.

Discussion
----------

[Cache] Clarify `#[Cache]` attribute precedence behavior

Adds tip explaining that #[Cache] attribute doesn't override manually set headers.

Related to [#62488](symfony/symfony#62488)

Commits
-------

9c6197e Clarify `#[Cache]` attribute precedence behavior
@nicolas-grekas nicolas-grekas changed the base branch from 7.4 to 6.4 December 5, 2025 14:42
@nicolas-grekas nicolas-grekas force-pushed the fix/cache-attribute-overrides-controller branch from 28ddacc to b93d460 Compare December 5, 2025 14:42
@nicolas-grekas nicolas-grekas force-pushed the fix/cache-attribute-overrides-controller branch from b93d460 to a6053f5 Compare December 5, 2025 15:05
@nicolas-grekas
Copy link
Member

Thank you @ayyoub-afwallah.

@nicolas-grekas nicolas-grekas merged commit 1083e8a into symfony:6.4 Dec 5, 2025
9 of 11 checks passed
@nicolas-grekas
Copy link
Member

Note that I didn't merge the check for no-store. It makes no sense to me.
Please open a new PR on 7.3 if you think this is a mistake and want to discuss this.

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