Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HttpFoundation][Cookie] Cookie DateTimeInterface fix #17370

Closed
wants to merge 6 commits into from
Closed

[HttpFoundation][Cookie] Cookie DateTimeInterface fix #17370

wants to merge 6 commits into from

Conversation

wildewouter
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

I came across an issue with expiration times on cookies. They were not working with DateTimeImmutable but only the DateTime implementation itself. I refactored this to work with the DateTimeInterface.

@@ -51,7 +51,7 @@ public function __construct($name, $value = null, $expire = 0, $path = '/', $dom
}

// convert expiration time to a Unix timestamp
if ($expire instanceof \DateTime) {
if ($expire instanceof \DateTimeInterface) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this does not work on PHP 5.3

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instanceof \DateTime || instanceof \DateTimeInterface

@wildewouter wildewouter changed the title Cookie DateTimeInterface fix [HttpFoundation][Cookie] Cookie DateTimeInterface fix Jan 14, 2016
@wildewouter
Copy link
Contributor Author

Closed in favor of #17371

@xabbuh
Copy link
Member

xabbuh commented Jan 14, 2016

Well, we could still additionally check for DateTimeImmutable instances on older Symfony versions.

@@ -85,6 +85,14 @@ public function testConstructorWithDateTime()
$this->assertEquals($expire->format('U'), $cookie->getExpiresTime(), '->getExpiresTime() returns the expire date');
}

public function testConstructorWithDateTimeImmutable()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@requires PHP 5.5 annotation

@xabbuh
Copy link
Member

xabbuh commented Jan 14, 2016

👍

Status: Reviewed

@fabpot
Copy link
Member

fabpot commented Jan 14, 2016

Thank you @wildewouter.

@fabpot fabpot closed this Jan 14, 2016
fabpot added a commit that referenced this pull request Jan 14, 2016
…dewouter)

This PR was squashed before being merged into the 2.3 branch (closes #17370).

Discussion
----------

[HttpFoundation][Cookie] Cookie DateTimeInterface fix

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

I came across an issue with expiration times on cookies. They were not working with DateTimeImmutable but only the DateTime implementation itself. I refactored this to work with the DateTimeInterface.

Commits
-------

f1f9754 [HttpFoundation][Cookie] Cookie DateTimeInterface fix
@fabpot fabpot mentioned this pull request Feb 3, 2016
@wildewouter wildewouter deleted the 2.3 branch February 3, 2016 13:55
This was referenced Feb 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants