Optional cookie validation

[ajgarlag]

Q	A
Bug?	yes
New Feature?	no
Version	69e4723

Actual Behavior

I'm writing a proxy script where I have to parse the set-cookie headers sent back by the upstream servers. Sometimes the upstream server (out of my control) send a cookie with invalid characters in name and/or value and when I try to parse it the library throws an \InvalidArgumentException

Expected Behavior

I'd like to make this validation optional

Steps to Reproduce

use Http\Discovery\HttpClientDiscovery;
use Http\Message\CookieJar;
use Http\Client\Common\PluginClient;
use Http\Client\Common\Plugin\CookiePlugin;
use Http\Client\Curl\Client;
use Http\Message\MessageFactory\DiactorosMessageFactory;
use Http\Message\StreamFactory\DiactorosStreamFactory;

$messageFactory = new DiactorosMessageFactory();

$pluginClient = new PluginClient(
    new Client($messageFactory, new DiactorosStreamFactory()),
    [new CookiePlugin(new CookieJar())]
);

$response = $pluginClient->sendRequest(
    $messageFactory->createRequest('GET', 'http://www.micromedexsolutions.com/home/dispatch')
);

[ajgarlag]

Possible solution (Borrowed from guzzle implementation)

A new 'isValid' method could be added to the Cookie class, and then, control in the CookieJar if invalid cookies are allowed.

What do you think?

[sagikazarmark]

What does "valid" mean in our case? Given that we have 3 different kind of validations.

Also, would this be a BC break (not doing validation)? Sounds like one to me.

What I would do is a static constructor createWithoutValidation bypassing the validation, but that's problematic from the plugin POV which creates the cookie.

[ajgarlag]

For me, "valid" means that the cookie passes all 3 validations.

If the createWithoutValidation constructor is added, it wouldn't be a BC. Then, in the plugin a flag can control if cookie validation is required (true by default). When false it would use the new constructor.

[sagikazarmark]

Sounds like an acceptable solution to me, but I have to say, the Guzzle way is probably better, so in a possible v2 package I would rather remove the validation from the constructor.

Also, regardless of having a separate constructor I think adding an isValid method makes sense, since other code might want to be sure that the cookie is valid.

Are you willing to provide a PR?

[ajgarlag]

Yes, but how to avoid current validation in the original constructor?

I see these options:

1. To add a flag parameter $requireValidation = true to the class constructor, and setting it to false in createWithoutValidation.
2. To create a new instance in createWithoutValidation with the help of ReflectionClass::newInstanceWithoutConstructor

If we choose the first option, the static method can be avoided too.