Add cookie #22
Add cookie #22
Conversation
| * | ||
| * @return self | ||
| */ | ||
| public function withExpiration($expiration) |
There was a problem hiding this comment.
what is the ordering logic of these methods? for max-age its get/has/with, here its with/get/has. i propose we sort all of them the same way. has/get/with feels most natural to me but i am happy with any, as long as its consistent.
There was a problem hiding this comment.
That's just a mistake. I usually follow get/has/set order.
There was a problem hiding this comment.
ah sorry, i whould have read the the phpdoc. but then i don't get what the idea is. if the param is an int, we use it as max-age, if its a datetime we use it as expires? are we sure that this is a good idea?
i would prefer to have withMaxAge remove the expires property, and vice versa.
There was a problem hiding this comment.
expires is the old way of doing things, HTTP 1.1 has max-age: http://mrcoles.com/blog/cookies-max-age-vs-expires/ (IE9+ supports max-age). i guess we can keep expires support, but i would not support the mixed approach. if anything, it would be the job of some hypothetical server side code that sets expires from the max-age.
though when we speak about client side code, we could receive a cookie with both values set and might care about that (and be it just to report that the server is sending ambigous data) - so probably setting one value should not null out the other.
but i would still remove this method, it seems weird. and we could phpdoc that expires is the old way of doing things and no longer recomended
|
Thanks for the feedback. |
|
About the expires thing: currently you can pass an expiration parameter in the constructor. Should we have max age instead. Also, this expiration is set for both max age and expires property ATM. Should we keep both max age and expires and only remove the magic expiration method? |
|
i would allow to set both from the constructor, to make it possible to
model a response precisely.
but remove the magic method.
and phpdoc that expire is http 1.0 and should be avoided.
|
|
What about order of arguments: should expires be the last? |
| public function __construct( | ||
| $name, | ||
| $value = null, | ||
| $maxAge = 0, |
There was a problem hiding this comment.
is this correct or should default value be null?
|
👍 agree to have expires last so it can be ignored when not recording an exact Server response |
| * @param string|null $path | ||
| * @param bool $secure | ||
| * @param bool $httpOnly | ||
| * @param \DateTime|null $expires |
There was a problem hiding this comment.
lets add the deprication note here too
There was a problem hiding this comment.
Should we add a @deprecated phpdoc to methods as well?
@deprecated since HTTP 1.0
There was a problem hiding this comment.
|
We could add cookie related helpers here. One thing I am thinking about: We could have the cookie parser as is OR provide a compat package around this method: http://php.net/manual/en/function.http-parse-cookie.php In general, we could have a pecl_http compatibility layer, but I have never used it before and don't plan in the future, so I wonder if it makes sense. |
Applied fixes from StyleCI Make CookieJar a single class Update Cookie docblocks Make properties private Remove virtual expiration attribute Improve character match tests Improve cookie tests Make Max-Age defatul null Add deprecation notice to expires constructor argument Add docblock constructor Allow null to be passed to expires Fine tune regex Add some more tests for path matching Add test for invalid max age
sagikazarmark
force-pushed
the
cookie
branch
from
3900c06 to
e121a73
Compare
|
Let's make cookie parsers in another PR. I think this is good to merge. Checked RFC compliance, but I am not sure about it. I've read the RFC several times, it is not always clear. From what I can understand, everything is OK. |
|
the pecl_http compatibility layer sounds like a good idea but lets move forward. it can always be added later. |
This PR adds Cookie implementation as discussed in php-http/httplug#98
Fix #15
A few open issues left in the original repository:
https://github.com/php-http/cookie/issues