Cookies with a 0 epoch expiry aren't treated as expired

[neerolyte]

Q	A
Bug?	yes
New Feature?	no
Version	6.3

Actual Behavior

What is the actual behavior?

Cookies with an expiry of Thu, 01 Jan 1970 00:00:00 GMT are reporting isExpired() as false.

Expected Behavior

What is the behavior you expect?

They should report as expired.

Steps to Reproduce

Run this code:

<?php
require_once('vendor/autoload.php');
use GuzzleHttp\Cookie\SetCookie;
echo "0: ".var_export(SetCookie::fromString('SESSION=destroy; expires=Thu, 01 Jan 1970 00:00:00 GMT;')->isExpired(), true)."\n";
echo "1: ".var_export(SetCookie::fromString('SESSION=destroy; expires=Thu, 01 Jan 1970 00:00:01 GMT;')->isExpired(), true)."\n";

You'll get:

$ php foo.php
0: false <--- should be true
1: true

Possible Solutions

The current check in SetCookie::isExpired() is:

return $this->getExpires() && time() > $this->getExpires();

I believe it should check getExpires() for null rather than just implicitly casting it to bool, e.g:

return $this->getExpires() !== null && time() > $this->getExpires();

[neerolyte]

I've worked around this by avoiding the use GuzzleHttp\Cookie\CookieJar and instead overloading it with a check that adjusts 0 values to 1 (which will still always be enough in the past to be immediately expired):

class CookieJar extends GuzzleHttp\Cookie\CookieJar {
	public function setCookie(GuzzleHttp\Cookie\SetCookie $cookie) {
		if ($cookie->getExpires() === 0) {
			$cookie->setExpires(1);
		}
		return parent::setCookie($cookie);
	}
}

[neerolyte]

Is this project still active?

[sagikazarmark]

If you have a proper solution PRs are always welcome.

[neerolyte]

Ok, I've added a PR to master with the solution I suggested in the description.

The PR is against master, but this is a problem in the 5.3 release too - do you want a PR against that branch as well?

[neerolyte]

So is the PR I've provided sufficient, do I need to do anything else to progress this?

[sagikazarmark]

Yes both the PR and the bug report are great, sorry for the delay. Patch release will be out in a few days.

[sagikazarmark]

@neerolyte yeah, a PR against 5.3 branch would be nice too. In the meantime I will merge the PR and tag a patch release for 6.x

[sagikazarmark]

Fixed in #2014