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

[HttpKernel] Do not superseed private cache-control when no-store is set#60569

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

Conversation

alexander-schranz
Copy link
Contributor

@alexander-schranzalexander-schranz commentedMay 28, 2025•
edited by OskarStark
Loading

QA
Branch?7.3
Bug fix?no
New feature?no
Deprecations?no
IssuesFix #...
LicenseMIT

I don't think its a good idea to superseed the private cache control via the new noStore option

If somebody want to set it toprivate they should explicit do it. via#[Cache(private: true, noStore: true)]. I would avoid this non transparent changes in general.

I had usecases in the past where the response is still public for the symfony cache and varnish public and the no store was only for the third party caches and in browser caches. This specially come into play with usage ofESI where the general page is cached, but no-store set to not allow back forwards caches, because of the ESI content.

/cc@smnandre

smnandre and dunglas reacted with thumbs up emoji
@carsonbotcarsonbot added this to the7.4 milestoneMay 28, 2025
@alexander-schranzalexander-schranz changed the base branch from7.4 to7.3May 28, 2025 07:35
@alexander-schranzalexander-schranzforce-pushed theenhancement/do-not-superseed-private-for-no-cache branch from252be50 to3f1c717CompareMay 28, 2025 07:52
@welcoMatticwelcoMattic modified the milestones:7.4,7.3May 28, 2025
@carsonbotcarsonbot changed the titleDo not superseed private cache-control when no-store is set[HttpKernel] Do not superseed private cache-control when no-store is setMay 28, 2025
@smnandre
Copy link
Member

I'm not sure here.

I get your point about "magic", but.. itdoes supersede the private header as, according to theRFC, a cache MUST not store any content with the no-store header.

So.. as i read it, no-store/public is not something we should allow here.

And there is some similar behaviour in thesetSharedMaxAge method of HttpFoundation/Response

Did i missread or missunderstood something here ?

@alexander-schranz
Copy link
ContributorAuthor

alexander-schranz commentedMay 28, 2025•
edited
Loading

@smnandre I sure understand your point and I know why you did implement it as I'm familiar with the RFC :). But in combination with server side caching and still control browsers handling and combinations of ESI this still a validtemporary state for the cache control headers.

So you have a response which should be cached but it includes a ESI which should not be cached and requires no-store. In that case for service side caches (symfony or varnish) the response in our case is still public because else the symfony http cache and the fos http cache is skipped and dont cache it. ESI can not manipulate the original cache headers so the controller tells it to be no-store if required by such ESI, but as we still want to cache the original response it still flagged aspublic because it knows it renders that ESI. Later post cache listeners or varnish vcl after correctly caching that response replace the public with private in that case, so important part is that in this case done after the caching (outside of the PHP application):

The max-age I know does the same something which we can sure not change because of BC reasons (personally would also have avoided that), but for the no-store we could still avoid such untransparent handling.

@alexander-schranz
Copy link
ContributorAuthor

alexander-schranz commentedMay 28, 2025•
edited
Loading

PS: If Symfony want to go the way of say we want to be strict about the RFC. It maybe would be better the handling is in the http foundation and not in the attribute listener. And so$response->headers->addCacheControlDirective('no-store'); would already set the response to private. Then it would be consistent and not be differently between attribute or manual call.

@@ -165,7 +165,6 @@ public function onKernelResponse(ResponseEvent $event): void
}

if (true === $cache->noStore) {
$response->setPrivate();

This comment was marked as outdated.

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

And so $response->headers->addCacheControlDirective('no-store'); would already set the response to private

I agree with that approach, the listener is not the correct place anyway for implementing such correlation

@smnandre
Copy link
Member

Thanks@alexander-schranz for this detailed explanation, makes perfect sense now!

@fabpotfabpot modified the milestones:7.3,7.4May 30, 2025
@fabpotfabpot changed the base branch from7.3 to7.4May 30, 2025 06:28
@fabpotfabpotforce-pushed theenhancement/do-not-superseed-private-for-no-cache branch from3f1c717 to7e6e33eCompareMay 30, 2025 06:28
@fabpot
Copy link
Member

Thank you@alexander-schranz.

@fabpotfabpot merged commit8c89e4c intosymfony:7.4May 30, 2025
11 checks passed
@alexander-schranzalexander-schranz deleted the enhancement/do-not-superseed-private-for-no-cache branchMay 30, 2025 07:19
@alexander-schranz
Copy link
ContributorAuthor

Should this not be target to 7.3?

nicolas-grekas and OskarStark reacted with thumbs up emoji

@nicolas-grekasnicolas-grekas modified the milestones:7.4,7.3May 30, 2025
nicolas-grekas pushed a commit that referenced this pull requestMay 30, 2025
…o-store is set (alexander-schranz)Discussion----------[HttpKernel] Do not superseed private cache-control when no-store is set| Q             | A| ------------- | ---| Branch?       | 7.3| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Issues        | -| License       | MITI don't think its a good idea to superseed the private cache control via the new noStore option*#59301If somebody want to set it to `private` they should explicit do it. via `#[Cache(private: true, noStore: true)]`. I would avoid this non transparent changes in general.I had usecases in the past where the response is still public for the symfony cache and varnish public and the no store was only for the third party caches and in browser caches. This specially come into play with usage of `ESI` where the general page is cached, but no-store set to not allow back forwards caches, because of the ESI content./cc `@smnandre`Commits-------7e6e33e [HttpKernel] Do not superseed private cache-control when no-store is set
@nicolas-grekas
Copy link
Member

Merged on 7.3 as158dff8

alexander-schranz reacted with thumbs up emoji

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

@fabpotfabpotfabpot approved these changes

@dunglasdunglasdunglas approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@smnandresmnandresmnandre approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.

8 participants
@alexander-schranz@smnandre@fabpot@nicolas-grekas@dunglas@GromNaN@welcoMattic@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp