Response::isNotModified returns true when If-Modified-Since is later than Last-Modified #11079
Conversation
| @@ -1075,19 +1075,23 @@ public function isNotModified(Request $request) | |||
| return false; | |||
| } | |||
|
|
|||
| $lastModified = $request->headers->get('If-Modified-Since'); | |||
| $notModified = false; | |||
| $modified = true; | |||
There was a problem hiding this comment.
The variable was probably named $notModified to follow the related HTTP status code name (304 NOT MODIFIED), I think you should keep the negation
There was a problem hiding this comment.
Well, It seems to me that it's easier to understand logic if there is no negation
There was a problem hid 8000 ing this comment.
If you apply this logic, you need to rename the method as well.
$notModified is just a special name and I would keep it.
There was a problem hiding this comment.
Can't get how method name is related to variables in it, but OK. I have renamed variable.
…than Last-Modified
|
I think your change also fixes #10857. could you add a testcase covering it ? |
|
|
||
| $response->headers->set('ETag', 'non-existent-etag'); | ||
| $response->headers->remove('Last-Modified'); | ||
| $this->assertFalse($response->isNotModified($request)); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@stof Can you, please, suggest a test method name? :) Is there any rules for test method naming?
There was a problem hiding this comment.
the test method name shoud describe what the test covers
testIfModififiedSinceWithoutLastModified might be a good name for instance
|
once the issue I reported is fixed, 👍 |
…ence of Last-Modified header
|
@stof Thanks, it's fixed |
|
Thank you @skolodyazhnyy. |
…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
Patch for #10501. I have reworked a bit
Response::isNotModifiedmethod to make it more readable. Now it's comparing If-Modified-Since and Last-Modified as dates.