Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Set Date header in Response constructor already#22036

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
mpdude wants to merge2 commits intosymfony:2.8fromwebfactory:response-always-date
Closed

Set Date header in Response constructor already#22036

mpdude wants to merge2 commits intosymfony:2.8fromwebfactory:response-always-date

Conversation

@mpdude
Copy link
Contributor

QA
Branch?2.8
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets
LicenseMIT
Doc PR

Setting theDate header in theResponse constructor has been removed in#14912 and changed to a more lazy approach ingetDate().

That way, methods likegetAge(),getTtl() orisFresh() cause side effects as they eventually callgetDate() 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

$response =newResponse();$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 theisCacheable() method, because it callsisFresh() only under certain conditions, like particular status codes, noETag 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 theDate header into theResponseHeaderBag. If theResponseHeaderBag guards that we always have theDate, we would not need special logic insendHeaders() and could also take care of#14912 (comment).

@xabbuh
Copy link
Member

Not sure I understand the actual this is trying to fix. Can you explain this a bit more?

@mpdude
Copy link
ContributorAuthor

mpdude commentedMar 17, 2017
edited
Loading

@xabbuh I'll try 😄

EveryResponse needs aDate: header.#14906 asked to postpone the initialization to as late as possible which was done in#14912. It now happens ingetDate() orsendHeaders() only.

As the date is needed for age calculations, it is implicitly initialized when you callgetAge(),getTtl() orisFresh().

My first point is that this is ugly API-wise because these getters/issers now change theResponse state in a subtle way. Also it feels weird that a Response does not "get old" unless you once call one of the above methods. After that,getTtl() will decrement every second andisFresh() will eventually expire.

Second,HttpCache relies on the age calculation functions. But because the lazy initialization happens ingetDate() orsendHeaders() andHttpCache may happen to callneither one, chances are the date remains uninitialized – this was reported in#19390.

[More precisely, when the Reponse has an ETag set,Response::isCacheable returns true because ofisValidateable(), without touching the date throughisFresh().]

#19390 fixed this by adding an extra safeguard inHttpCache::store(), but I think that's just a workaround for the underlying cause.

Wecould remove the additional initializations ingetDate() orsendHeaders() but then we'd need to make sure otherwise that the header remains presenteven if you remove it (I have an idea how to do so). [Updated: See#22124]

Does that make it clearer?

@fabpot
Copy link
Member

Thank you@mpdude.

fabpot added a commit that referenced this pull requestMar 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
@fabpotfabpot closed thisMar 22, 2017
@mpdudempdude deleted the response-always-date branchMarch 22, 2017 21:44
This was referencedApr 5, 2017
nicolas-grekas added a commit that referenced this pull requestApr 7, 2017
…pdonadio)This PR was squashed before being merged into the 3.2 branch (closes#22304).Discussion----------Moved $this->setDate() before the deprecation handling.| Q             | A| ------------- | ---| Branch?       | 3.2 <!-- see comment below -->| Bug fix?      | yes| New feature?  | no <!-- don't forget updating src/**/CHANGELOG.md files -->| BC breaks?    | no?| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->| Tests pass?   | yes| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License       | MIT| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features--><!--- Bug fixes must be submitted against the lowest branch where they apply  (lowest branches are regularly merged to upper ones so they get the fixes too).- Features and deprecations must be submitted against the master branch.- Please fill in this template according to the PR you're about to submit.- Replace this comment by a description of what your PR is solving.-->#22036 merged the patch into the wrong place; the `$this->setDate()` happens after the deprecation handling, so it does not always get called.See alsohttps://www.drupal.org/node/2712647?page=1#comment-12025349Commits-------b6df4e7 Moved ->setDate() before the deprecation handling.
symfony-splitter pushed a commit to symfony/http-foundation that referenced this pull requestApr 7, 2017
…pdonadio)This PR was squashed before being merged into the 3.2 branch (closes #22304).Discussion----------Moved $this->setDate() before the deprecation handling.| Q             | A| ------------- | ---| Branch?       | 3.2 <!-- see comment below -->| Bug fix?      | yes| New feature?  | no <!-- don't forget updating src/**/CHANGELOG.md files -->| BC breaks?    | no?| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->| Tests pass?   | yes| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License       | MIT| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features--><!--- Bug fixes must be submitted against the lowest branch where they apply  (lowest branches are regularly merged to upper ones so they get the fixes too).- Features and deprecations must be submitted against the master branch.- Please fill in this template according to the PR you're about to submit.- Replace this comment by a description of what your PR is solving.-->symfony/symfony#22036 merged the patch into the wrong place; the `$this->setDate()` happens after the deprecation handling, so it does not always get called.See alsohttps://www.drupal.org/node/2712647?page=1#comment-12025349Commits-------b6df4e7 Moved ->setDate() before the deprecation handling.
fabpot added a commit that referenced this pull requestJun 16, 2017
…seHeaderBag (mpdude)This PR was squashed before being merged into the 3.4 branch (closes#22124).Discussion----------Shift responsibility for keeping Date header to ResponseHeaderBag| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets || License       | MIT| Doc PR        |This is an improvement over#22036. It shifts responsibility for preserving a `Date` header to the `ResponseHeaderBag`.We already have similar logic there for the `Cache-Control` header.Commits-------5d83836 Shift responsibility for keeping Date header to ResponseHeaderBag
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mpdude@xabbuh@fabpot@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp