Expanded fault-tolerance for unusual cookie dates #7149
Conversation
…time fault-tolerance for cookies that have very unusual date formatting
| @@ -205,6 +207,11 @@ private static function parseDate($dateValue) | |||
| } | |||
| } | |||
|
|
|||
| // attempt a fallback for unusual formatting | |||
| if (false !== $dateTimestamp = strtotime($dateValue)) { | |||
|
Please add some unit tests for the new formats. |
|
Sorry for neglecting the unit tests, they've been updated (2 matching new common date formats, 1 uncommon date format, and changing the existing bad-date check to be more realistically bad.) I also changed from strtotime to date_create to match the existing DateTime::createFromFormat check (although in my cookiejar analysis leading to this pull requests, all the cookies I'd encountered had timezones in them.) I'm using date_create vs. constructing a DateTime so I can immediately rely on the return value. |
|
@vicb The two Travis failures are against the master branch unrelated to my changes. Should I retarget this pull against 2.3, or what would you advise to get this pull accepted? |
| @@ -205,6 +207,11 @@ private static function parseDate($dateValue) | |||
| } | |||
| } | |||
|
|
|||
| // attempt a fallback for unusual formatting | |||
| if (false !== $date = date_create($dateValue, new \DateTimeZone('GMT'))) { | |||
There was a problem hiding this comment.
tabs ? Sf use 4 spaces for indentation
There was a problem hiding this comment.
Believe me - I got a bit nauseous when I saw that slip through. I thought that Scrutinizer's auto-change got merged back into this pull. I'll update.
|
The Travis failure come for a bug in PHPUnit (there is a Sf issue for that). There is no 2.3 branch yet (devs happen in master). @fabpot will decide wether this should be considered a a fix (and merge to former releases) or an enhancement which will be merged to 2.3. (Could you please update the PR header which still refers to strtotime, thanks) |
| @@ -205,6 +207,11 @@ private static function parseDate($dateValue) | |||
| } | |||
| } | |||
|
|
|||
| // attempt a fallback for unusual formatting | |||
| if (false !== $date = date_create($dateValue, new \DateTimeZone('GMT'))) { | |||
There was a problem hiding this comment.
Should use UTC rather than GMT, per http://www.php.net/manual/en/timezones.others.php
There was a problem hiding this comment.
Wouldn't that be out of the scope of this pull, given the two other existing places in Cookie.php that already invoke GMT rather than the preferred UTC?
|
This should probably go into 2.0. Also, do you have a reference where those 7 formats are explained/described? |
|
@fabpot I couldn't find a reference because the cookies that we're addressing are ones that are behaving outside the spec (at least what I understand from http://curl.haxx.se/rfc/cookie_spec.html), as pull #1793 began to address and this continues. The cases that I've added are ones that I have encountered over the weeks of using BrowserKit and Goutte. |
This PR was submitted for the master branch but it was merged into the 2.1 branch instead (closes #7149). Commits ------- 0c25d41 Expanded fault-tolerance for unusual cookie dates Discussion ---------- Expanded fault-tolerance for unusual cookie dates Building on pull #1793, this resolves situations where the Cookie's date field uses a numeric month. Also, expanding on the 7 most typical formats we fall-back to date_create() before throwing an exception. --------------------------------------------------------------------------- by vicb at 2013-02-21T17:30:28Z Please add some unit tests for the new formats. --------------------------------------------------------------------------- by ecaron at 2013-02-21T18:06:46Z Sorry for neglecting the unit tests, they've been updated (2 matching new common date formats, 1 uncommon date format, and changing the existing bad-date check to be more realistically bad.) I also changed from strtotime to date_create to match the existing DateTime::createFromFormat check (although in my cookiejar analysis leading to this pull requests, all the cookies I'd encountered had timezones in them.) I'm using date_create vs. constructing a DateTime so I can immediately rely on the return value. --------------------------------------------------------------------------- by ecaron at 2013-02-21T18:21:03Z @vicb The two Travis failures are against the master branch unrelated to my changes. Should I retarget this pull against 2.3, or what would you advise to get this pull accepted? --------------------------------------------------------------------------- by vicb at 2013-02-21T19:40:59Z The Travis failure come for a bug in PHPUnit (there is a Sf issue for that). There is no 2.3 branch yet (devs happen in master). @fabpot will decide wether this should be considered a a fix (and merge to former releases) or an enhancement which will be merged to 2.3. _(Could you please update the PR header which still refers to strtotime, thanks)_ --------------------------------------------------------------------------- by fabpot at 2013-02-21T21:37:15Z This should probably go into 2.0. Also, do you have a reference where those 7 formats are explained/described? --------------------------------------------------------------------------- by ecaron at 2013-02-21T23:10:38Z @fabpot I couldn't find a reference because the cookies that we're addressing are ones that are behaving outside the spec (at least what I understand from http://curl.haxx.se/rfc/cookie_spec.html), as pull #1793 began to address and this continues. The cases that I've added are ones that I have encountered over the weeks of using BrowserKit and Goutte.
|
merged in 2.1, thanks. |
Building on pull #1793, this resolves situations where the Cookie's date field uses a numeric month. Also, expanding on the 7 most typical formats we fall-back to date_create() before throwing an exception.