Skip to content

Implements a cookie-date parsing utility #76

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
Merged
merged 1 commit into from
Jun 19, 2017
Merged

Implements a cookie-date parsing utility #76

merged 1 commit into from
Jun 19, 2017

Conversation

aaa2000
Copy link
Contributor

@aaa2000 aaa2000 commented Jun 12, 2017 •
edited
Loading

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets mentioned in php-http/client-common#46
Documentation No
License MIT

What's in this PR?

Implementation of a cookie-date parsing algorithm mentionned in php-http/client-common#46. I preferred to use a simpler method from https://github.com/symfony/symfony/blob/master/src/Symfony/Component/BrowserKit/Cookie.php#L199.

Example Usage

$datetime = CookieUtil::parseDate('Friday, 31 Jul 20 08:49:37 GMT');

Checklist

  • Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix
  • Documentation pull request created (if not simply a bugfix)

aaa2000
aaa2000 commented Jun 12, 2017
View reviewed changes
src/CookieUtil.php Outdated
* @var array
*/
private static $dateFormats = array(
'D, d M y H:i:s T',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I add the format "Tuesday, 31 Mar 99 07:42:12 GMT" at the line 14 mentionned in php-http/client-common#46

aaa2000
aaa2000 commented Jun 12, 2017
View reviewed changes
src/CookieUtil.php Outdated
* @see https://github.com/symfony/symfony/blob/master/src/Symfony/Component/BrowserKit/Cookie.php
*
* @param string $dateValue
* @return \DateTime|false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I rather throw an exception ?

@aaa2000 aaa2000 force-pushed the cookie-date-parsing branch 2 times, most recently from eec2be5 to e83fd58 Compare June 12, 2017 20:53
dbu
dbu approved these changes Jun 13, 2017
View reviewed changes
Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

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

thanks. i agree to not create a dependency on symfony browser kit for just this function.

just one question: is date_create alone not good enough?

CHANGELOG.md Outdated
@@ -3,6 +3,10 @@

## Unreleased

###
Copy link
Contributor

Choose a reason for hiding this comment

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

please use the title "Added" here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix

@aaa2000
Copy link
Contributor Author

aaa2000 commented Jun 13, 2017

@dbu if we use only date_create, the tests will fail for this format Fri, 31-07-20 08:49:37 GMT.

@aaa2000
Copy link
Contributor Author

aaa2000 commented Jun 13, 2017

But this format is not conform to the rfc https://tools.ietf.org/html/rfc6265#section-5.1.1

month = "Jan" | "Feb" | "Mar" | "Apr" | "May" | "Jun" | "Jul" | "Aug" | "Sep" | "Oct" | "Nov" | "Dec"

it has been added to manage unusual cookie dates, see commit symfony/browser-kit@47f146d

dbu
dbu approved these changes Jun 14, 2017
View reviewed changes
Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

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

thanks for the clarification. seems good then, +1 from me.

regarding the build fail, thats a hhvm setup issue. i created #77. if we want to be dead sure it all works out, we can wait for that to merge and then rebase this branch on master afterwards.

@aaa2000 aaa2000 force-pushed the cookie-date-parsing branch from e3ce2ef to b4d1e0e Compare June 14, 2017 22:10
@aaa2000
Copy link
Contributor Author

aaa2000 commented Jun 14, 2017

Rebase done

I added more tests from library mentioned in php-http/client-common#46 https://github.com/salesforce/tough-cookie/blob/master/test/date_test.js except 01 Jan 1600 00:00:00 GMT which should not pass because the date before 1601.

Use only the function date_create all tests pass except Fri, 31-07-20 08:49:37 GMT. Format that does not appear to comply with RFC.

Maybe, CookieUtil::parseDate is not really useful.

@aaa2000
Copy link
Contributor Author

aaa2000 commented Jun 14, 2017

tests failed on PHP 5.4 and HHVM

@aaa2000
Copy link
Contributor Author

aaa2000 commented Jun 14, 2017

I will fix later

http://php.net/manual/en/class.datetime.php#datetime.changelog

5.4.24 	The COOKIE constant was changed to reflect RFC 1036 using a four digit year rather than a two digit year (RFC 850) as prior versions.

@dbu
Copy link
Contributor

dbu commented Jun 15, 2017 •
edited
Loading

as hhvm works on master, did we just discover a difference between hhvm and php here?

the php 5.4 build fails on the same things, formatting the date: expected "Friday, 31-Jul-2020 08:49...", but got "Friday, 31-Jul-2020 08:49...". - unfortunately its cut off before any differences show up.

it seems to me that format(\DateTime::COOKIE) uses different formats in different versions. as we don't want to test the \DateFormat::format method, maybe just use an explicit formatting string so you can compare with a well known outcome?

dbu
dbu reviewed Jun 15, 2017
View reviewed changes
src/CookieUtil.php Outdated
*
* @param string $dateValue
*
* @return \DateTime|false
Copy link
Contributor

Choose a reason for hiding this comment

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

i would prefer throwing an exception when the date can't be parsed

Copy link
Contributor Author

@aaa2000 aaa2000 Jun 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

a php exception like UnexpectedValueException or Http\Message\Exception\UnexpectedValueException ?

@aaa2000
Copy link
Contributor Author

aaa2000 commented Jun 15, 2017

Yes it is format(\DateTime::COOKIE) https://3v4l.org/VpvRp, as you said, I will use use an explicit formatting string. I would do it tonight.

@aaa2000 aaa2000 force-pushed the cookie-date-parsing branch from d5ec782 to caa5344 Compare June 16, 2017 21:09
@aaa2000
Copy link
Contributor Author

aaa2000 commented Jun 16, 2017

throws an exception when the date can't be parsed and commits squashed

fbourigault
fbourigault approved these changes Jun 19, 2017
View reviewed changes
Copy link
Contributor

@fbourigault fbourigault left a comment

Choose a reason for hiding this comment

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

I'm not a cookie date expert, but looks good to me.

@dbu dbu merged commit f12663b into php-http:master Jun 19, 2017
@dbu
Copy link
Contributor

dbu commented Jun 19, 2017

thanks a lot @aaa2000 !

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

@dbu dbu dbu approved these changes

@fbourigault fbourigault fbourigault approved these changes

@Nyholm Nyholm Nyholm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aaa2000 @dbu @fbourigault @Nyholm