Skip to content

[HttpKernel] Dont store response cookies with HttpCache #24190

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[HttpKernel] Dont store response cookies with HttpCache #24190

wants to merge 1 commit into from

Conversation

ro0NL
Copy link
Contributor

@ro0NL ro0NL commented Sep 13, 2017 •
edited
Loading

Q A
Branch? 3.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #...
License MIT
Doc PR symfony/symfony-docs#...

Before #20569 response cookies, created using the object API, were not preserved in ResponseHeaderBag::all(), thus not stored with HttpCache. Yet, cookies created with the string API did.

That difference is now eliminated, but because of it we now always include all cookies 😅 Causing side effects here (mentioned on slack today). Sorry, i did not anticipated that.

I think the preferred behavior is to not store any cookies, as i tend to believe object api is used a lot more.

So this is a hotfix, whereas 3.4 might further discuss features about cookie policy.

Reporter notified, lets wait for confirmation a bit. But i think this should do.

@ro0NL
Copy link
Contributor Author

ro0NL commented Sep 13, 2017 •
edited
Loading

TBH im just as scared of the opposite behavior really :) im not too familiar with HttpCache, so dont merge blindly.

Status: Needs work

@nicolas-grekas
Copy link
Member

How does Varnish behave here? What does the HTTP spec say about cookies?

@ro0NL
Copy link
Contributor Author

ro0NL commented Sep 15, 2017

From https://varnish-cache.org/docs/5.1/users-guide/increasing-your-hitrate.html#cookies

Varnish will, in the default configuration, not cache an object coming from the backend with a 'Set-Cookie' header present. Also, if the client sends a Cookie header, Varnish will bypass the cache and go directly to the backend.

Also think https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php#L562 implies to preserve actually.

Goal of this PR is to add a test in 3.3 for this :)

@nicolas-grekas nicolas-grekas added this to the 3.3 milestone Sep 24, 2017
@ro0NL
Copy link
Contributor Author

ro0NL commented Sep 25, 2017 •
edited
Loading

Closing for now. Side effect is real; yet current behavior is the one expected IMHO. Tend to believe there be'd more issues if it was really bad :)

@ro0NL ro0NL closed this Sep 25, 2017
@ro0NL ro0NL deleted the httpcache/no-cookies branch September 25, 2017 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug HttpKernel Status: Needs Work
Projects
None yet
Milestone
3.3
Development

Successfully merging this pull request may close these issues.
3 participants
@ro0NL @nicolas-grekas @carsonbot