Optional cookie validation

php-http / message

HTTP Message related tools

http://php-http.org

MIT License

1.29k stars 41 forks source link

Optional cookie validation #68

Closed ajgarlag closed 7 years ago

ajgarlag commented 7 years ago

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Related tickets	fixes #67
License	MIT

What's in this PR?

This PR adds optional cookie validation

Example Usage

TBD

Checklist

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

To Do

[x] Discuss how to avoid current cookie constructor validation: additional flag parameter or using ReflectionClass::newInstanceWithoutConstructor

ajgarlag commented 7 years ago

I've implemented it with an additional __construct parameter.

Maybe, we can rename createWithoutValidation to create and deprecate __construct making it private in 2.0

What do you think?

sagikazarmark commented 7 years ago

@php-http/owners can you add some input here?

sagikazarmark commented 7 years ago

I do not think we should disable validation in the constructor though

My only concern is that the current way is a little bit hacky. Also, maybe we could make this a configuration in CookieJar (whether to check cookie validation or not)

sagikazarmark commented 7 years ago

This looks good to me. Is there anything else @Nyholm ?

@ajgarlag can you please rebase your branch?

Nyholm commented 7 years ago

Im happy with the current state of this PR. Thanks!

ajgarlag commented 7 years ago

Rebased. Sorry for the delay.

sagikazarmark commented 7 years ago

Does this require a follow up PR in the common-client repo to solve the original issue? Any last words @php-http/owners before merging?

ajgarlag commented 7 years ago

I'll add a $requireValidCookies flag (true by default) to CookieJar and then, validate each cookie when is added to CookieJar. Then, in the common-client repo, I will always use createWithoutValidation to create the cookie object, although, I think this could be a BC in common-client.

Nyholm commented 7 years ago

I think this could be a BC in common-client

It has to be =)

I do not think the CookieJar should care about if the cookies are valid or not. The Jar is just a storage, it should be pretty stupid.

I agree with you that the CookiePlugin should always use createWithoutValidation. Depending on configuration of the CookiePlugin, we validate the cookies before we put them in the jar. That is BC.

👍 for merging this PR now.

ajgarlag commented 7 years ago

I agree with you that the CookiePlugin should always use createWithoutValidation.

As this the method that should be used always by CookiePlugin, I proposed:

Maybe, we can rename createWithoutValidation to create and deprecate __construct making it private in 2.0

Nyholm commented 7 years ago

I know. But making the constructor private would be a BC break. And we are doing it just because "one class in a other library stopped using the constructor". That is wrong. Also if we created (though unlikely) a AlwaysValidCookiePlugin, that class would use the constructor.

Nyholm commented 7 years ago

Good to merge?

sagikazarmark commented 7 years ago

Sorry for the long wait, thanks for the contribution @ajgarlag