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

Response::isNotModified returns true when If-Modified-Since is later than Last-Modified #11079

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
5 participants
@skolodyazhnyy
Copy link
Contributor

commented Jun 7, 2014

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #10501, #10857
License MIT
Doc PR

Patch for #10501. I have reworked a bit Response::isNotModified method to make it more readable. Now it's comparing If-Modified-Since and Last-Modified as dates.

@@ -1075,19 +1075,23 @@ public function isNotModified(Request $request)
return false;
}
$lastModified = $request->headers->get('If-Modified-Since');
$notModified = false;
$modified = true;

This comment has been minimized.

Copy link
@matthieuauger

matthieuauger Jun 18, 2014

Contributor

The variable was probably named $notModified to follow the related HTTP status code name (304 NOT MODIFIED), I think you should keep the negation

This comment has been minimized.

Copy link
@skolodyazhnyy

skolodyazhnyy Jun 18, 2014

Author Contributor

Well, It seems to me that it's easier to understand logic if there is no negation

This comment has been minimized.

Copy link
@apfelbox

apfelbox Jun 18, 2014

Contributor

If you apply this logic, you need to rename the method as well.

$notModified is just a special name and I would keep it.

This comment has been minimized.

Copy link
@skolodyazhnyy

skolodyazhnyy Jun 18, 2014

Author Contributor

Can't get how method name is related to variables in it, but OK. I have renamed variable.

@stof

This comment has been minimized.

Copy link
Member

commented Jul 17, 2014

I think your change also fixes #10857. could you add a testcase covering it ?

@skolodyazhnyy

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2014

@stof Done. If I understand it right, the problem with #10857 appears when there is both request headers specified, but response has only etag. If response has non - logic was fine.

$response->headers->set('ETag', 'non-existent-etag');
$response->headers->remove('Last-Modified');
$this->assertFalse($response->isNotModified($request));

This comment has been minimized.

Copy link
@stof

stof Jul 17, 2014

Member

These cases were the response does not have a Last-Modified header but the request has If-Modified-Since should be moved to a separate test method to be more readable

This comment has been minimized.

Copy link
@skolodyazhnyy

skolodyazhnyy Jul 17, 2014

Author Contributor

@stof Can you, please, suggest a test method name? :) Is there any rules for test method naming?

This comment has been minimized.

Copy link
@stof

stof Jul 17, 2014

Member

the test method name shoud describe what the test covers

testIfModififiedSinceWithoutLastModified might be a good name for instance

$response = new Response();
$response->headers->set('ETag', $etag);
$response->headers->remove('Last-Modified');

This comment has been minimized.

Copy link
@stof

stof Jul 17, 2014

Member

this is not needed, as you don't set the header before

$this->assertTrue($response->isNotModified($request));
$response->headers->set('ETag', 'non-existent-etag');
$response->headers->remove('Last-Modified');

This comment has been minimized.

Copy link
@stof

stof Jul 17, 2014

Member

same here

@stof

This comment has been minimized.

Copy link
Member

commented Jul 17, 2014

once the issue I reported is fixed, 👍

@skolodyazhnyy

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2014

@stof Thanks, it's fixed

@fabpot

This comment has been minimized.

Copy link
Member

commented Sep 23, 2014

Thank you @skolodyazhnyy.

fabpot added a commit that referenced this pull request Sep 23, 2014

bug #11079 Response::isNotModified returns true when If-Modified-Sinc…
…e is later than Last-Modified (skolodyazhnyy)

This PR was submitted for the master branch but it was merged into the 2.3 branch instead (closes #11079).

Discussion
----------

Response::isNotModified returns true when If-Modified-Since is later than Last-Modified

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #10501,  #10857
| License       | MIT
| Doc PR        |

Patch for #10501. I have reworked a bit `Response::isNotModified` method to make it more readable. Now it's comparing If-Modified-Since and Last-Modified as dates.

Commits
-------

42ec76e Response::isNotModified returns true when If-Modified-Since is later than Last-Modified

@fabpot fabpot closed this Sep 23, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.