Skip to content

[HttpFoundation] Allow DateTimeImmutable in Response setters #24677

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 26, 2017
Merged

[HttpFoundation] Allow DateTimeImmutable in Response setters #24677

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 28 additions & 14 deletions src/Symfony/Component/HttpFoundation/Response.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -672,9 +672,13 @@ public function getDate()
*
* @final since version 3.2
*/
public function setDate(\DateTime $date)
public function setDate(\DateTimeInterface $date)
{
$date->setTimezone(new \DateTimeZone('UTC'));
if ($date instanceof \DateTime) {
$date = \DateTimeImmutable::createFromMutable($date);
}

$date = $date->setTimezone(new \DateTimeZone('UTC'));
$this->headers->set('Date', $date->format('D, d M Y H:i:s').' GMT');

return $this;
Expand Down Expand Up @@ -732,22 +736,27 @@ public function getExpires()
*
* Passing null as value will remove the header.
*
* @param \DateTime|null $date A \DateTime instance or null to remove the header
* @param \DateTimeInterface|null $date A \DateTime instance or null to remove the header
*
* @return $this
*
* @final since version 3.2
*/
public function setExpires(\DateTime $date = null)
public function setExpires(\DateTimeInterface $date = null)
{
if (null === $date) {
$this->headers->remove('Expires');
} else {
$date = clone $date;
$date->setTimezone(new \DateTimeZone('UTC'));
$this->headers->set('Expires', $date->format('D, d M Y H:i:s').' GMT');

return $this;
}

if ($date instanceof \DateTime) {
$date = \DateTimeImmutable::createFromMutable($date);
}

$date = $date->setTimezone(new \DateTimeZone('UTC'));
$this->headers->set('Expires', $date->format('D, d M Y H:i:s').' GMT');

return $this;
}

Expand Down Expand Up @@ -888,22 +897,27 @@ public function getLastModified()
*
* Passing null as value will remove the header.
*
* @param \DateTime|null $date A \DateTime instance or null to remove the header
* @param \DateTimeInterface|null $date A \DateTime instance or null to remove the header
*
* @return $this
*
* @final since version 3.2
*/
public function setLastModified(\DateTime $date = null)
public function setLastModified(\DateTimeInterface $date = null)
{
if (null === $date) {
$this->headers->remove('Last-Modified');
} else {
$date = clone $date;
$date->setTimezone(new \DateTimeZone('UTC'));
$this->headers->set('Last-Modified', $date->format('D, d M Y H:i:s').' GMT');

return $this;
}

if ($date instanceof \DateTime) {
$date = \DateTimeImmutable::createFromMutable($date);
}

$date = $date->setTimezone(new \DateTimeZone('UTC'));
Copy link
Contributor

Choose a reason for hiding this comment

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

Correct me if I'm wrong, but DateTimeInterface doesn't have a setTimezone method from what I can tell.

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't. That's why the block above that line makes sure we're dealing with a DateTimeImmutable here.

Copy link
Member Author

Choose a reason for hiding this comment

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

To explain this a bit more: The sole purpose of DateTimeInterface is to allow type-hinting for objects that are either DateTime or DateTimeImmutable. Those are the only two implementations of that interface and the php runtime doesn't allow userland code to directly implement the interface.

So at the beginning of the method, $date has to be either, null, DateTime or DateTimeImmutable. For null, we have an early exit. A DateTime instance is converted to DateTimeImmutable.

So right here, $date has to be an instance of DateTimeImmutable which contains the method setTimezone().

Copy link
Contributor

Choose a reason for hiding this comment

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

the php runtime doesn't allow userland code to directly implement the interface.

Does this include extensions? Regardless of php allowing it or not, it looks weird like this. Should probably throw an exception or ignore the setter when it's not supported.

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to handle something that doesn't happen. that's just adding visual noise. to me it's good as it is

Copy link
Member Author

Choose a reason for hiding this comment

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

Does this include extensions?

An extension could probably do that, but it would be a very odd thing to do.

I could not even write a test for the case that we have a DateTimeInterface instance that is neither a DateTime nor a DateTimeImmutable instance. I don't think, we need to handle it.

$this->headers->set('Last-Modified', $date->format('D, d M Y H:i:s').' GMT');

return $this;
}

Expand Down
54 changes: 43 additions & 11 deletions src/Symfony/Component/HttpFoundation/Tests/ResponseTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -653,6 +653,22 @@ public function testIsImmutable()
$this->assertTrue($response->isImmutable());
}

public function testSetDate()
{
$response = new Response();
$response->setDate(\DateTime::createFromFormat(\DateTime::ATOM, '2013-01-26T09:21:56+0100', new \DateTimeZone('Europe/Berlin')));

$this->assertEquals('2013-01-26T08:21:56+00:00', $response->getDate()->format(\DateTime::ATOM));
}

public function testSetDateWithImmutable()
{
$response = new Response();
$response->setDate(\DateTimeImmutable::createFromFormat(\DateTime::ATOM, '2013-01-26T09:21:56+0100', new \DateTimeZone('Europe/Berlin')));

$this->assertEquals('2013-01-26T08:21:56+00:00', $response->getDate()->format(\DateTime::ATOM));
}

public function testSetExpires()
{
$response = new Response();
Expand All @@ -666,6 +682,16 @@ public function testSetExpires()
$this->assertEquals($response->getExpires()->getTimestamp(), $now->getTimestamp());
}

public function testSetExpiresWithImmutable()
{
$response = new Response();

$now = $this->createDateTimeImmutableNow();
$response->setExpires($now);

$this->assertEquals($response->getExpires()->getTimestamp(), $now->getTimestamp());
}

public function testSetLastModified()
{
$response = new Response();
Expand All @@ -676,6 +702,16 @@ public function testSetLastModified()
$this->assertNull($response->getLastModified());
}

public function testSetLastModifiedWithImmutable()
{
$response = new Response();
$response->setLastModified($this->createDateTimeImmutableNow());
$this->assertNotNull($response->getLastModified());

$response->setLastModified(null);
$this->assertNull($response->getLastModified());
}

public function testIsInvalid()
{
$response = new Response();
Expand Down Expand Up @@ -912,6 +948,13 @@ protected function createDateTimeNow()
return $date->setTimestamp(time());
}

protected function createDateTimeImmutableNow()
{
$date = new \DateTimeImmutable();

return $date->setTimestamp(time());
}

protected function provideResponse()
{
return new Response();
Expand Down Expand Up @@ -990,14 +1033,3 @@ public function __toString()
class DefaultResponse extends Response
{
}

class ExtendedResponse extends Response
{
public function setLastModified(\DateTime $date = null)
{
}

public function getDate()
{
}
}