Fix #67 Fixed Cookie creation in cookie plugin. #68
Conversation
|
I am sort a new to php-spec 😄 Therefor i did not know how to assert values in the cookie object, since the cookiejar can not be mocked (final). Any suggestions? |
|
Thanks for the report and the PR. I will review this shortly. |
| 'cookie=value; expires=%s; Max-Age=60; path=/; domain=test.com; secure; HttpOnly', | ||
| (new \DateTime())->add( | ||
| new \DateInterval('PT60S') | ||
| )->format(DATE_COOKIE) |
There was a problem hiding this comment.
Can you please use \DateTime::COOKIE instead?
|
that indeed looks more robust to me. @sagikazarmark can you maybe help with phpspec / my fear of a test that will randomly fail? |
| @@ -109,11 +110,11 @@ private function createCookie(RequestInterface $request, $setCookie) | |||
|
|
|||
| switch (strtolower($key)) { | |||
| case 'expires': | |||
| $expires = \DateTime::createFromFormat(DATE_COOKIE, $value); | |||
| $expires = \DateTime::createFromFormat(\DateTime::COOKIE, $value); | |||
There was a problem hiding this comment.
looking at the test failure on hhvm, i wonder if hhvm can't handle dates that far in the future?
which in turn leads me to think we should check the return value as \DateTime::createFromFormat is one of those stupid functions that returns false instead of throwing an exception on failure. if we check for false === $expires we can throw an exception and put the $value in the message to say it was not converted to a date.
There was a problem hiding this comment.
And what exception type would you like this to be? It probably needs to implement the name namespace exception interface. Can it be a TransferException? Or since there is a response a HttpException?
There was a problem hiding this comment.
I would say it is not an HTTP related exception, so we should just fail with one of the SPL exceptions. UnexpectedValue maybe?
There was a problem hiding this comment.
if i am not mistaken, HttpException is to express a HTTP error code. so it would be TransferException and we say that the server sent unexpected data.
looking at DateTime::COOKIE having "l, d-M-Y H:i:s T" and Y meaning 4 digit year, i have no idea what hhvm is doing here. but if it works for all versions with a two digit year lets go with that - its just mock data after all.
and lets add a test that triggers the exception, with a date value of "this is not a date" or something.
There was a problem hiding this comment.
|
Hmm hhvm does not seem to handle 4 digit year in cookie expires date. |
|
oh well... hm, whatever then lets just put 99 :-D
not our problem to solve really, and in 2099 we probably won't live
anymore (and i would be surprised if PHP was still around then...). and
in 2100, 99 will probably mean 2199 so we are all good.
|
|
One last minor thing (Style CI) and ready to merge from my point of view. |
|
👍 maybe also squash the commits. |
| @@ -86,6 +87,7 @@ public function handleRequest(RequestInterface $request, callable $next, callabl | |||
| * @param $setCookie | |||
| * | |||
| * @return Cookie|null | |||
There was a problem hiding this comment.
the styleci complaint is here: we require a blank line between the @return and an @throws
Expand the cookie header content test with all parts. Pass cookie parts as intended to cookie object. Throw transfer exception when the cookie expires header can not be parsed.
|
Fixed and squashed 🎱 |
|
Merged this manually because there were some conflicts. |
|
Thanks @Bouwdie |
Update todo Remove options resolver dependency Revert: Remove options resolver dependency Add AddHostPlugin Add Authentication plugins Add content length plugin Add final warning to plugins Add cookie plugin Add decoder plugin Add error plugin Add header plugins Add history plugin Fix namespace import order Add redirect plugin Add retry plugin Make plugin classes final, related #18 Fix throw keyword Manually apply php-http/plugins#68 Manually apply and close php-http/plugins#65
Update todo Remove options resolver dependency Revert: Remove options resolver dependency Add AddHostPlugin Add Authentication plugins Add content length plugin Add final warning to plugins Add cookie plugin Add decoder plugin Add error plugin Add header plugins Add history plugin Fix namespace import order Add redirect plugin Add retry plugin Make plugin classes final, related #18 Fix throw keyword Manually apply php-http/plugins#68 Manually apply and close php-http/plugins#65
Update todo Remove options resolver dependency Revert: Remove options resolver dependency Add AddHostPlugin Add Authentication plugins Add content length plugin Add final warning to plugins Add cookie plugin Add decoder plugin Add error plugin Add header plugins Add history plugin Fix namespace import order Add redirect plugin Add retry plugin Make plugin classes final, related #18 Fix throw keyword Manually apply php-http/plugins#68 Manually apply and close php-http/plugins#65
Update todo Remove options resolver dependency Revert: Remove options resolver dependency Add AddHostPlugin Add Authentication plugins Add content length plugin Add final warning to plugins Add cookie plugin Add decoder plugin Add error plugin Add header plugins Add history plugin Fix namespace import order Add redirect plugin Add retry plugin Make plugin classes final, related #18 Fix throw keyword Manually apply php-http/plugins#68 Manually apply and close php-http/plugins#65
fix #67
Fix applied by calculating the expireIn (max-age) seconds instead of expire datetime.
Expanded save spec by using a more complete cookie.
Use DATE_COOKIE constant instead of DateTime constant.
Expand the cookie header content with all parts.