[HttpFoundation] Validate/cast cookie expire time #20925
Conversation
ro0NL
commented
Dec 14, 2016
•
| Q | A |
|---|---|
| Branch? | 2.7 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #... |
| License | MIT |
| Doc PR | symfony/symfony-docs#... |
| @@ -63,15 +63,19 @@ public function __construct($name, $value = null, $expire = 0, $path = '/', $dom | |||
| } elseif (!is_numeric($expire)) { | |||
| $expire = strtotime($expire); | |||
|
|
|||
| if (false === $expire || -1 === $expire) { | |||
There was a problem hiding this comment.
👍 -1 was returned by strtotime on PHP 5.1
There was a problem hiding this comment.
Prior to 5.1.0 actually ;) do you want to remove it in 2.7? Im fine with it as is.
| @@ -81,6 +89,13 @@ public function testGetExpiresTime() | |||
| $this->assertEquals($expire, $cookie->getExpiresTime(), '->getExpiresTime() returns the expire date'); | |||
| } | |||
|
|
|||
| public function testGetExpiresTimeIsCastedToInt() | |||
| { | ||
| $cookie = new Cookie('foo', 'bar', 3600.9); | ||
|
|
||
| $this->assertEquals(3600, $cookie->getExpiresTime(), '->getExpiresTime() returns the expire date as an integer'); |
| throw new \InvalidArgumentException('The cookie expiration time is not valid.'); | ||
| } | ||
| } | ||
|
|
||
| if (0 > $expire) { | ||
| throw new \InvalidArgumentException('The cookie expiration time must be a positive number value.'); |
There was a problem hiding this comment.
This is a bc break if anyone is doing this for some reason.
We could deprecate, but the behavior is just fine when negative numbers are used here.
Instead, I'd rather fix getMaxAge and turn 0 !== $this->expire into 0 >= $this->expire.
and merge this in a lower branch
There was a problem hiding this comment.
I think we should indeed settle with >=. Nice one.. didnt thought of it 👍
|
👍 |
| throw new \InvalidArgumentException('The cookie expiration time is not valid.'); | ||
| } | ||
| } | ||
|
|
||
| $this->name = $name; | ||
| $this->value = $value; | ||
| $this->domain = $domain; | ||
| $this->expire = $expire; | ||
| $this->expire = 0 < $expire ? (int) $expire : 0; |
There was a problem hiding this comment.
$this->expire = max(0, (int) $expire) ?
There was a problem hiding this comment.
Not sure the extra function call is really worth it. Tend to keep it as is.
|
Thank you @ro0NL. |
This PR was merged into the 2.7 branch. Discussion ---------- [HttpFoundation] Validate/cast cookie expire time | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #... <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR | symfony/symfony-docs#... <!--highly recommended for new features--> Commits ------- 8215dbd [HttpFoundation] Validate/cast cookie expire time