Return null as Expire header if it was set to null #33353
Merged
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
berezuev
reviewed
Aug 27, 2019
| @@ -369,6 +369,12 @@ public function testExpire() | |||
| $this->assertNull($response->headers->get('Expires'), '->expire() removes the Expires header when the response is fresh'); | |||
| } | |||
|
|
|||
| public function testNullExpireHeader() | |||
There was a problem hiding this comment.
Think, you'd better place this test into testExpire() method (see line 339 in this file) and use more consistent format, like this:
$response = new Response();
$response->headers->set('Expires', null);
$response->expire();
$this->assertNull($response->headers->get('Expires'), 'Place description of expected behavior here')There was a problem hiding this comment.
I thought about it, but this bug is not about calling expire on the response, actually this call is not necessary at all. That's why I've decided against that, in order to keep the test minimal.
nicolas-grekas
approved these changes
Aug 27, 2019
xabbuh
reviewed
Aug 29, 2019
xabbuh
approved these changes
Aug 29, 2019
|
Thank you @danrot. |
nicolas-grekas
added a commit
that referenced
this pull request
Aug 29, 2019
This PR was squashed before being merged into the 3.4 branch (closes #33353). Discussion ---------- Return null as Expire header if it was set to null | Q | A | ------------- | --- | Branch? | 3.4 <!-- see below --> | Bug fix? | yes | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | BC breaks? | no <!-- see https://symfony.com/bc --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tests pass? | yes <!-- please add some, will be required by reviewers --> | Fixed tickets | <!-- #-prefixed issue number(s), if any --> | License | MIT | Doc PR | symfony/symfony-docs#... <!-- required for new features --> This PR fixes a regression introduces in #33332. If you set the `Expires` header to null when creating a `Response`, the `getExpires` function returned a date instead of null. ```php $response = new Response(null, 200, ['Expires' => null]); $response->getExpires(); // Returns a date currently, but should return null ``` See also [the comment](#33332 (comment)) in the PR introducing this regression. Commits ------- 5e3c7ea Return null as Expire header if it was set to null
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.
This PR fixes a regression introduces in #33332. If you set the
Expiresheader to null when creating aResponse, thegetExpiresfunction returned a date instead of null.See also the comment in the PR introducing this regression.