Optional cookie validation #68

ajgarlag · 2016-12-22T13:40:08Z

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

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

To Do

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

ajgarlag · 2016-12-22T22:26:09Z

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 · 2016-12-23T01:09:43Z

src/Cookie.php

@@ -73,11 +81,16 @@ public function __construct(
        $path = null,
        $secure = false,
        $httpOnly = false,
-        \DateTime $expires = null
+        \DateTime $expires = null,
+        $requireValidation = true


I don't really like the idea of having optional validation. Have it or not IMO. So I would just deprecate the exception and bypass the validation manually.

sagikazarmark · 2016-12-23T01:10:51Z

src/Cookie.php

+        $httpOnly = false,
+        \DateTime $expires = null
+    ) {
+        return new self($name, $value, $maxAge, $domain, $path, $secure, $httpOnly, $expires, false);


I would do some hacks here: pass null or the valid empty value to bypass the validation, then set the property directly. Since we are in the same class, we can set private properties.

sagikazarmark · 2016-12-23T01:13:26Z

src/Cookie.php

+     */
+    public function isValid()
+    {
+        if (null === $this->valid) {


Hm, not sure about this. This value should be cleared at least when the value or the max age is "modified", but even then I am not sure the performance gain worth risking inconsistency.

You are right, I'll remove this.

sagikazarmark · 2016-12-31T04:38:24Z

src/Cookie.php

@@ -63,7 +63,7 @@
     * @param bool           $httpOnly
     * @param \DateTime|null $expires  Expires attribute is HTTP 1.0 only and should be avoided.
     *
-     * @throws \InvalidArgumentException If name, value or max age is not valid.
+     * @throws \InvalidArgumentException If name, value or max age is not valid. Attributes validation during instantiation is deprecated since 1.5 and will be removed in 2.0.


I think it should be "Attribute validation"

sagikazarmark · 2016-12-31T04:38:35Z

src/Cookie.php

@@ -89,6 +94,24 @@ public function __construct(
        $this->httpOnly = (bool) $httpOnly;
    }

+    public static function createWithoutValidation(


Can you please add phpdoc?

Should this be deprecated as well? That would be weird, but still...

sagikazarmark · 2016-12-31T04:42:21Z

spec/CookieSpec.php

+    }
+
+    /**
+     * @dataProvider invalidCharacterExamples


Not sure if we want to test the fact that validation actually works correctly here. Although I can't really see any tests for that. Nevertheless I think we only need to test two cases here: validation passes/validation fails. For that I am not sure if we need data provider. In fact using data providers in specification tests is kind of abusing BDD, these kind of tests should be done in separate unit tests.

sagikazarmark · 2016-12-31T04:43:13Z

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

Nyholm

I like the idea of a createWithoutValidation and isValid. I see from your issue why they are needed.

I do not think we should disable validation in the constructor though. Because normally when you are creating a Cookie you want to make sure it is a valid object. Currently dev-master makes sure you never have an invalid object with is a good thing and it should be the default behavior.

sagikazarmark · 2016-12-31T17:46:44Z

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 · 2017-01-14T07:16:53Z

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

@ajgarlag can you please rebase your branch?

Nyholm · 2017-01-14T08:48:19Z

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

Adds a named constructor to avoid cookie attributes validation during instantination, and a method to check if cookie attributes are valid.

…idation

ajgarlag · 2017-01-18T07:55:12Z

Rebased. Sorry for the delay.

sagikazarmark · 2017-01-18T15:04:21Z

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 · 2017-01-19T08:36:29Z

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 · 2017-01-19T08:44:16Z

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 · 2017-01-19T11:52:25Z

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 · 2017-01-19T12:04:43Z

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 · 2017-02-12T08:05:28Z

Good to merge?

sagikazarmark · 2017-02-13T15:11:34Z

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

ajgarlag force-pushed the create-cookies-without-validation branch 2 times, most recently from e656ae8 to d6e6ec8 Compare December 22, 2016 22:22

sagikazarmark reviewed Dec 23, 2016

View reviewed changes

sagikazarmark requested changes Dec 31, 2016

View reviewed changes

Nyholm requested changes Dec 31, 2016

View reviewed changes

sagikazarmark approved these changes Jan 14, 2017

View reviewed changes

Nyholm approved these changes Jan 14, 2017

View reviewed changes

ajgarlag added 6 commits January 18, 2017 08:46

Specification for optional cookie validation

c48157d

Adds Cookie::createWithoutValidation and Cookie::isValid

5b20f04

Adds a named constructor to avoid cookie attributes validation during instantination, and a method to check if cookie attributes are valid.

Creates new cookie instance with dummy name to bypass constructor val…

5fb7027

…idation

Remove deprecation of attribute validation in Cookie::__construct

fbc60f3

Adds docblock to Cookie::createWithoutValidation

f2d32a4

Drop use of dataProvider in Cookie::createWithoutValidation spec

5eeecd1

ajgarlag force-pushed the create-cookies-without-validation branch from fb1ac8e to 5eeecd1 Compare January 18, 2017 07:52

Nyholm mentioned this pull request Feb 12, 2017

Tag new minor version #73

Closed

sagikazarmark merged commit 72d7e28 into php-http:master Feb 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optional cookie validation #68

Optional cookie validation #68

ajgarlag commented Dec 22, 2016 •

edited

Loading

ajgarlag commented Dec 22, 2016

sagikazarmark Dec 23, 2016

sagikazarmark Dec 23, 2016

sagikazarmark Dec 23, 2016

ajgarlag Dec 27, 2016

sagikazarmark Dec 31, 2016

sagikazarmark Dec 31, 2016

sagikazarmark Dec 31, 2016

sagikazarmark Dec 31, 2016

sagikazarmark commented Dec 31, 2016

Nyholm left a comment

sagikazarmark commented Dec 31, 2016

sagikazarmark commented Jan 14, 2017

Nyholm commented Jan 14, 2017

ajgarlag commented Jan 18, 2017

sagikazarmark commented Jan 18, 2017 •

edited

Loading

ajgarlag commented Jan 19, 2017

Nyholm commented Jan 19, 2017

ajgarlag commented Jan 19, 2017

Nyholm commented Jan 19, 2017

Nyholm commented Feb 12, 2017

sagikazarmark commented Feb 13, 2017

Optional cookie validation #68

Optional cookie validation #68

Conversation

ajgarlag commented Dec 22, 2016 • edited Loading

What's in this PR?

Example Usage

Checklist

To Do

ajgarlag commented Dec 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagikazarmark commented Dec 31, 2016

Nyholm left a comment

Choose a reason for hiding this comment

sagikazarmark commented Dec 31, 2016

sagikazarmark commented Jan 14, 2017

Nyholm commented Jan 14, 2017

ajgarlag commented Jan 18, 2017

sagikazarmark commented Jan 18, 2017 • edited Loading

ajgarlag commented Jan 19, 2017

Nyholm commented Jan 19, 2017

ajgarlag commented Jan 19, 2017

Nyholm commented Jan 19, 2017

Nyholm commented Feb 12, 2017

sagikazarmark commented Feb 13, 2017

ajgarlag commented Dec 22, 2016 •

edited

Loading

sagikazarmark commented Jan 18, 2017 •

edited

Loading