Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MultiDomainBundle] Replaced host override cookie with session #688

Merged

Conversation

yoshz
Copy link

@yoshz yoshz commented Sep 4, 2015

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets none

I had a problem with the host override cookie was unset by Varnish because it is prefixed with underscore.
As it is a session cookie it is easy replaced by using the request session instead of an extra cookie.

@jockri
Copy link
Contributor

jockri commented Sep 4, 2015

All admin routes (/admin/xx) should never be cached by Varnish. So the host override cookie should work as expected. Or do you have an other use case?

@yoshz
Copy link
Author

yoshz commented Sep 4, 2015

Varnish doesn't cache the response, so that isn't the problem.
Our Varnish VCL strips all _* cookies from the request. The convention is to prefix a cookie with an underscore only when you use the cookie clientside with javascript, like Google Analytics does. Otherwise varnish will not cache the response when a cookie is set.

This pull request is just another way of overriding the host without removing the underscore which solves my usecase and probably that of others.

@jockri
Copy link
Contributor

jockri commented Sep 4, 2015

@wimvds What was the reason why you moved from using sessions to cookies?

@yoshz yoshz force-pushed the feature/multidomain-session-hostoverride branch from 09f23d3 to ba98ff6 Compare September 9, 2015 08:13
@roderik
Copy link
Contributor

roderik commented Sep 15, 2015

@wimvds ping

@wimvds
Copy link
Contributor

wimvds commented Sep 16, 2015

A session was created on every request (because $session->has starts a session when no session is started yet). Putting it inside a $session->isStarted() test did not seem to work (and I didn't have time to find out why). That's why I refactored it to use a cookie instead.

@mlebkowski
Copy link
Contributor

@jockri
Copy link
Contributor

jockri commented Oct 29, 2015

@yoshz can you rebase please.

Using the session is ok for us. It looks like there is some sort of convention that cookies that start with underscores can be removed in Varnish. So, or we remove the underscore, or we use the session.

@yoshz yoshz force-pushed the feature/multidomain-session-hostoverride branch 2 times, most recently from cb42826 to 13bfacb Compare October 29, 2015 13:15
@yoshz
Copy link
Author

yoshz commented Oct 29, 2015

Done

@roderik
Copy link
Contributor

roderik commented Oct 29, 2015

@yoshz the tests failed

@yoshz
Copy link
Author

yoshz commented Oct 29, 2015

Uhh.. I guess someone added tests in meantime :).
I will look into it.

@yoshz yoshz force-pushed the feature/multidomain-session-hostoverride branch 2 times, most recently from 5d0e168 to 54a6847 Compare October 29, 2015 16:10
@yoshz yoshz force-pushed the feature/multidomain-session-hostoverride branch from 54a6847 to db12498 Compare October 29, 2015 16:46
@yoshz
Copy link
Author

yoshz commented Oct 31, 2015

All checks have passed! Please merge otherwise I keep rebasing :)

@roderik roderik added this to the 3.4.2 milestone Oct 31, 2015
roderik pushed a commit that referenced this pull request Oct 31, 2015
…toverride

[MultiDomainBundle] Replaced host override cookie with session
@roderik roderik merged commit 991abbd into Kunstmaan:master Oct 31, 2015
@roderik
Copy link
Contributor

roderik commented Oct 31, 2015

done :)

@yoshz yoshz deleted the feature/multidomain-session-hostoverride branch December 11, 2015 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants