[HttpFoundation] Postpone setting the date header on a Response #14912
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
There is also a slight BC break if someone access the Date header from the bag directly... |
|
👍 |
|
ooops... HttpKernel tests are failing |
b804502 to
6e85a3a
Compare
|
I've added lazy date header initialisation in |
| if (!$this->headers->has('Date')) { | ||
| $this->setDate(new \DateTime(null, new \DateTimeZone('UTC'))); | ||
| } | ||
|
|
||
| return $this->headers->getDate('Date', new \DateTime()); |
There was a problem hiding this comment.
there is no point in adding a default here since it cannot trigger anymore. also this default could have the wrong timezone anyway.
|
There is a BC break for people who accessed |
|
@Tobion That's the BC break I mentioned above, but I think we can ignore this use case. But of course, that's also why it should only be merged in 2.8 and not on earlier branches. |
|
👍 apart from the changes I mentioned above |
6e85a3a to
2ad3b0d
Compare
|
Thanks Jakub. |
Tobion
added a commit
that referenced
this pull request
Jun 9, 2015
… Response (jakzal) This PR was merged into the 2.8 branch. Discussion ---------- [HttpFoundation] Postpone setting the date header on a Response | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #14906 | License | MIT | Doc PR | - The only risk of doing this is if someone called `getDate()` and the date header was not present, the date might be slightly different than the one sent with the headers. `getDate()` could also set the date header first time it's requested (do a lazy initialisation). Commits ------- 2ad3b0d [HttpFoundation] Postpone setting the date header on a Response
Merged
fabpot
added a commit
that referenced
this pull request
Mar 22, 2017
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
symfony-splitter
pushed a commit
to symfony/http-foundation
that referenced
this pull request
Mar 22, 2017
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 symfony/symfony#14912 (comment).
Commits
-------
3a7fa7ede2 Set Date header in Response constructor already
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The only risk of doing this is if someone called
getDate()and the date header was not present, the date might be slightly different than the one sent with the headers.getDate()could also set the date header first time it's requested (do a lazy initialisation).