Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return null as Expire header if it was set to null #33353

Merged
merged 1 commit into from Aug 29, 2019

Conversation

@danrot
Copy link
Contributor

danrot commented Aug 27, 2019

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR symfony/symfony-docs#...

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.

$response = new Response(null, 200, ['Expires' => null]);
$response->getExpires(); // Returns a date currently, but should return null

See also the comment in the PR introducing this regression.

@@ -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()

This comment has been minimized.

Copy link
@berezuev

berezuev Aug 27, 2019

Contributor

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')

This comment has been minimized.

Copy link
@danrot

danrot Aug 27, 2019

Author Contributor

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.

Copy link
Member

nicolas-grekas left a comment

This highlights an interesting behavior. In case anyone is wondering, this reverts to the historical behavior for values explicitly set to null.

src/Symfony/Component/HttpFoundation/HeaderBag.php Outdated Show resolved Hide resolved
@xabbuh
xabbuh approved these changes Aug 29, 2019
@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Aug 29, 2019
@nicolas-grekas nicolas-grekas force-pushed the danrot:expires-header branch from 507e07f to 5e3c7ea Aug 29, 2019
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented 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
@nicolas-grekas nicolas-grekas merged commit 5e3c7ea into symfony:3.4 Aug 29, 2019
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details
@danrot danrot deleted the danrot:expires-header branch Aug 29, 2019
This was referenced Oct 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.