-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simplify Cookie Class #4596
Simplify Cookie Class #4596
Conversation
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.
Your PR title is for changing the Cookie constructor yet you clearly have two PRs here.
- I approve the simplification of the constructor call for the Cookie as that is leaning towards 7.3.
- Unless you can give me reason why
convertExpiresTimestamp
is no longer accessible via class, I cannot accept the removal ofstatic
. - For the
expires
changes (which clearly misplaced here in this PR), I understand that you want to make things simple here but I'm on the side of making things "fluent" on the side of the developer using this API. For example, if I want to make an "expired" Cookie object, which would I choose as more fluent --$cookie->withExpires(0)
or$cookie->withExpired()
. I believe even non-devs would choose the 2nd option as that is clearer. Another one is I want to get the expires string suitable forSet-Cookie
headers. I would definitely want to use$cookie->getExpiresString()
over$cookie->getExpires(false)
. ThegetExpiresTimestamp()
on the other hand is a side effect of havinggetExpiresString()
so as to differentiate the two expires getters. It will be clear that one gets the string expires while the other gets the integer value. - I don't get why the tests on Array Access were removed when there's no changes to that in the class. By removing those tests I'm assuming that the
Cookie
, after this PR change, now allows mutability which is in contrast to its immutable nature.
|
50fa7ec
to
036fdec
Compare
I assume this is yet another PR that will need to be resolved before release. Please prioritize this. |
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.
Unless you'll revert all changes to expires
we can get this merged. Otherwise, I'm afraid that is a blocker for merge.
On a side note, I think I made a mistake with withExpiresAt($expires = 0)
by setting a default value. This should not be the case. It should be only withExpiresAt($expires)
.
ebcb5ce
to
bdfc2a3
Compare
Thank you, @mostafakhudair |
@paulbalandan Many thanks to you for you'r professional review and your patience |
Thanks all around! I think we are now finally cleared for release. |
Since we use php min version
7.3
, why not passing all cookie options to the constructor itself directly.Removed
Cookie::create
whilenew Cookie
do the same thing directly.Renamed
Cookie::getExpiresTimestamp
toCookie::getExpires
to be short and refer to it's property,Renamed
Cookie::withExpiresAt
toCookie::withExpires
to fit it's property and also it may return expired cookie.I think this is last change to new Cookie class should be done before the upcoming release
@paulbalandan what do you think about this changes? is it keeps the interface "fluent"? Thanks in advance