Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpFoundation] Create cookie from string + synchronize response cookies#20569
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| constSAMESITE_LAX ='lax'; | ||
| constSAMESITE_STRICT ='strict'; | ||
| /** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Creates
HeahDude commentedNov 19, 2016
It seems there is no deprecation in your PR, wrong description or WIP? |
ro0NL commentedNov 19, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Not sure :) i tend to think it solves more then it would break. Right now all cookies are dealt with in a consistent manner, which is good. However we could lose some raw cookie attributes though, and people benefit from this bug before in terms of preserving those attributes. I think
yes :) deprecation label should be removed |
ro0NL commentedNov 19, 2016
The synchronization is also missing in Or what about the other way around? Add a |
ro0NL commentedNov 19, 2016
Ready :) |
| $headers =str_replace("\r\n",PHP_EOL, (string)$bag); | ||
| $this->assertRegExp('#^Set-Cookie:\s+foo=bar; path=/path/foo; domain=foo.bar; httponly$#m',$headers); | ||
| $this->assertRegExp('#^Set-Cookie:\s+foo=bar; path=/path/foo; domain=foo.bar; httponly$#m',$headers); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Shouldn't it bepath=/path/bar;?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nice catch, also regex escaping was missing. Fixed.
ro0NL commentedNov 20, 2016
Green :) |
| if ($this->path) { | ||
| $str .='; path='.$this->path; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why? IIRC,path is optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Based onhttps://github.com/ro0NL/symfony/blob/721fc351b2dcf81ba1888682a42155288f5474d2/src/Symfony/Component/HttpFoundation/Cookie.php#L124 i chose to force/ by default here as well. Maybe it's not needed indeed.
I guess the only case we dont have a path is when a developer overridesCookie::$path (set it manually).
What about
if ($this->getPath()) {//..}
to be consistent with the domain attribute handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
See#20910
| publicfunction__toString() | ||
| { | ||
| if (!$this->headers) { | ||
| if (!$headers =$this->all()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If you removeheaders usage, the property should be removed as well... but it is protected, so that's a BC break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ResponsHeaderBag overridesall() by including the additional cookie headers.HeaderBag::all still returns$this->headers... so it's not removed.
However internally it now usesall() (the API method) instead of$headers (the property) so those cookie headers are taken into account when using for exampleHeaderBag::has.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the property is still used.
In the normal HeaderBag,->all() is a simple getter for it. But it is now overwritten in ResponseHeaderBag to add the set-cookie header
| if ('' === (string)$this->getValue()) { | ||
| $str .='deleted; expires='.gmdate('D, d-M-Y H:i:s T',time() -31536001); | ||
| }else { | ||
| $str .=urlencode($this->getValue()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
isn't this a bug fix ? If yes, it should go in older branches
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Guess so. I will separate it this evening 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
See#20910
| publicfunction__toString() | ||
| { | ||
| if (!$this->headers) { | ||
| if (!$headers =$this->all()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the property is still used.
In the normal HeaderBag,->all() is a simple getter for it. But it is now overwritten in ResponseHeaderBag to add the set-cookie header
| { | ||
| returnarray_combine($this->headerNames,$this->headers); | ||
| $headers =$this->allPreserveCase(); | ||
| if (isset($this->headerNames['set-cookie'])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
what if a case is not provided for theset-cookie header ? you will not remove it at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
…ookies (ro0NL)This PR was squashed before being merged into the 3.1 branch (closes#20910).Discussion----------[HttpFoundation] Fix cookie to string conversion for raw cookies| Q | A| ------------- | ---| Branch? | 3.1| Bug fix? | yes| New feature? | not really| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#20569 (comment)| License | MIT| Doc PR | symfony/symfony-docs#... <!--highly recommended for new features-->Separated from#20569This mimics PHP's `setrawcookie` behavior.Commits-------5e899cd [HttpFoundation] Fix cookie to string conversion for raw cookies
This PR was merged into the 3.3-dev branch.Discussion----------[HttpFoundation] Compute cookie max-age attribute| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | comma-separated list of tickets fixed by the PR, if any| License | MIT| Doc PR | reference to the documentation PR, if anySeehttps://wiki.php.net/rfc/cookie_max-age, PHP sends it since 5.5.~~The date format actually differs now from PHP (`Saturday` instead of `Sat`). But it really doesnt matter much imo, using the constant seems more reliable to me and max-age overrules anyway ;)~~Relates to#20569Commits-------8c28317 [HttpFoundation] Compute cookie max-age attribute
ro0NL commentedDec 17, 2016
Rebased with the latest cookie changes. Still all good :) |
fabpot commentedDec 17, 2016
Thank you@ro0NL. |
…ze response cookies (ro0NL)This PR was merged into the 3.3-dev branch.Discussion----------[HttpFoundation] Create cookie from string + synchronize response cookies| Q | A| ------------- | ---| Branch? | "master"| Bug fix? | yes| New feature? | yes| BC breaks? | no-ish| Deprecations? | no| Tests pass? | yes| Fixed tickets | comma-separated list of tickets fixed by the PR, if any| License | MIT| Doc PR | reference to the documentation PR, if anyFixes the conversion/synchroniczation of cookies between the object API and string API, ie;```php$headers->setCookie(new Cookie('foo', 'bar'));$headers->has('set-cookie'); // true$headers->set('set-cookie', 'foo2=bar2', false);count($headers->getCookies()); // 2```With an additional feature `Cookie::fromString($cookie)`The BC break happens in terms you cannot set a custom `Set-Cookie` header anymore, all cookies are set via `set(raw)cookie` by PHP.We could go one step further by tracking misc. attributes (`Cookie::getAttributes()`) and populate accordingly from `Cookie::fromString`. If the cookie has attributes we should bypass `setcookie` and write a header instead, but im not sure it's worth it.Commits-------7314456 [HttpFoundation] Create cookie from string + synchronize response cookiesThis PR was merged into the 3.3-dev branch.Discussion----------[HttpFoundation] update changelog| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#20569| License | MIT| Doc PR |Commits-------e91a654 [HttpFoundation] update changelog
This PR was merged into the 3.3-dev branch.Discussion----------[HttpKernel] Continuation of#20569| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#20567 (comment)| License | MIT| Doc PR | symfony/symfony-docs#... <!--highly recommended for new features-->Forgotten in#20569 and makes#20567 truly a feature only.BeforeAfterCommits-------e1616b3 [HttpKernel] Continuation of#20569
This PR was squashed before being merged into the 2.7 branch (closes#20972).Discussion----------[HttpFoundation] Improved set cookie header tests| Q | A| ------------- | ---| Branch? | 2.7 (already in master)| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | symfony/symfony-docs#... <!--highly recommended for new features-->Separated from#20569Commits-------05bce71 [HttpFoundation] Improved set cookie header tests
This PR was merged into the 3.3-dev branch.Discussion----------[WebProfilerBundle] Improved cookie traffic| Q | A| ------------- | ---| Branch? | "master"| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | comma-separated list of tickets fixed by the PR, if any| License | MIT| Doc PR | reference to the documentation PR, if anyRelates to#20569 in terms of getting _all_ the cookies.Commits-------171c6d1 [WebProfilerBundle] Improved cookie traffic
Uh oh!
There was an error while loading.Please reload this page.
Fixes the conversion/synchroniczation of cookies between the object API and string API, ie;
With an additional feature
Cookie::fromString($cookie)The BC break happens in terms you cannot set a custom
Set-Cookieheader anymore, all cookies are set viaset(raw)cookieby PHP.We could go one step further by tracking misc. attributes (
Cookie::getAttributes()) and populate accordingly fromCookie::fromString. If the cookie has attributes we should bypasssetcookieand write a header instead, but im not sure it's worth it.