Skip to content

Commit

Permalink
bug #22036 Set Date header in Response constructor already (mpdude)
Browse files Browse the repository at this point in the history
This PR was squashed before being merged into the 2.8 branch (closes #22036).

Discussion
----------

Set Date header in Response constructor already

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

Setting the `Date` header in the `Response` constructor has been removed in #14912 and changed to a more lazy approach in `getDate()`.

That way, methods like `getAge()`, `getTtl()` or `isFresh()` cause side effects as they eventually call `getDate()` and the Request "starts to age" once you call them.

I don't know if this would be a nice test, but current behaviour is

```php
        $response = new Response();
        $response->setSharedMaxAge(10);
        sleep(20);
        $this->assertTrue($response->isFresh());
        sleep(5);
        $this->assertTrue($response->isFresh());
        sleep(5);
        $this->assertFalse($response->isFresh());
```

A particular weird case is the `isCacheable()` method, because it calls `isFresh()` only under certain conditions, like particular status codes, no `ETag` present etc. This symptom is also described under "Cause of the problem" in #19390, however the problem is worked around there in other ways.

So, this PR suggests to effectively revert #14912.

Additionally, I'd like to suggest to move this special handling of the `Date` header into the `ResponseHeaderBag`. If the `ResponseHeaderBag` guards that we always have the `Date`, we would not need special logic in `sendHeaders()` and could also take care of #14912 (comment).

Commits
-------

3a7fa7e Set Date header in Response constructor already
  • Loading branch information
fabpot committed Mar 22, 2017
2 parents 89bb895 + 3a7fa7e commit e3d90db
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 1 deletion.
11 changes: 11 additions & 0 deletions src/Symfony/Component/HttpFoundation/Response.php
Expand Up @@ -201,6 +201,11 @@ public function __construct($content = '', $status = 200, $headers = array())
$this->setContent($content);
$this->setStatusCode($status);
$this->setProtocolVersion('1.0');

/* RFC2616 - 14.18 says all Responses need to have a Date */
if (!$this->headers->has('Date')) {
$this->setDate(new \DateTime(null, new \DateTimeZone('UTC')));
}
}

/**
Expand Down Expand Up @@ -329,6 +334,7 @@ public function sendHeaders()
return $this;
}

/* RFC2616 - 14.18 says all Responses need to have a Date */
if (!$this->headers->has('Date')) {
$this->setDate(\DateTime::createFromFormat('U', time()));
}
Expand Down Expand Up @@ -612,6 +618,11 @@ public function mustRevalidate()
*/
public function getDate()
{
/*
RFC2616 - 14.18 says all Responses need to have a Date.
Make sure we provide one even if it the header
has been removed in the meantime.
*/
if (!$this->headers->has('Date')) {
$this->setDate(\DateTime::createFromFormat('U', time()));
}
Expand Down
4 changes: 3 additions & 1 deletion src/Symfony/Component/HttpFoundation/Tests/ResponseTest.php
Expand Up @@ -276,8 +276,10 @@ public function testGetDate()
$this->assertEquals($now->getTimestamp(), $date->getTimestamp(), '->getDate() returns the date when the header has been modified');

$response = new Response('', 200);
$now = $this->createDateTimeNow();
$response->headers->remove('Date');
$this->assertInstanceOf('\DateTime', $response->getDate());
$date = $response->getDate();
$this->assertEquals($now->getTimestamp(), $date->getTimestamp(), '->getDate() returns the current Date when the header has previously been removed');
}

public function testGetMaxAge()
Expand Down

0 comments on commit e3d90db

Please sign in to comment.