[HttpFoundation] Fix isNotModified determination logic #40545
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
|
This breaks some existing tests, please have a look, and add more if it makes sense. |
b9f64b4 to
80f278f
Compare
25696a3 to
352b1f0
Compare
|
Tests are running fine on local machine, travis seems to be on correct tag after closing / re-opening the pr. I honestly do not know why the test fails on travis. Any ideas? |
|
In the |
| @@ -20,7 +20,7 @@ | |||
| "symfony/error-handler": "^4.4", | |||
| "symfony/event-dispatcher": "^4.4", | |||
| "symfony/http-client-contracts": "^1.1|^2", | |||
| "symfony/http-foundation": "^4.4|^5.0", | |||
| "symfony/http-foundation": "^4.4.21|^5.2.6", | |||
There was a problem hiding this comment.
should be bumped to ^4.4.22|^5.2.7 now that 4.4.21 and5.2.6 have been released
befd41e to
3befea3
Compare
|
Thank you. Travis is failing and when I reviewed the output I suspect that the failure is related to this PR. Can you confirm? |
A friend of mine did ask this on slack and this was the outcome:
|
|
EDIT: shouldn't matter since _ are replaced by - and capital letters always to lowercase. |
f35615d to
6b2a7f4
Compare
This comment has been minimized.
This comment has been minimized.
|
Needs a rebase to fix conflicts |
…nce to If-None-Match and If-Modified-Since respectively.
6b2a7f4 to
07d51e2
Compare
|
This should probably resolve sensiolabs/SensioFrameworkExtraBundle#513 in a better way than simply adding weak ETags! |
|
Thank you @ol0lll. |
…0lll) This PR was squashed before being merged into the 4.4 branch. Discussion ---------- [HttpFoundation] Fix isNotModified determination logic | Q | A | ------------- | --- | Branch? | 4.4 <!-- see below --> | Bug fix? | yes | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tickets | Fix #37948 <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead --> | License | MIT | Doc PR | symfony/symfony-docs#... <!-- required for new features --> Use weak comparison for etags; Only do if-modified-since when no if-none-match header is present. See #37948 (comment) <!-- Replace this notice by a short README for your feature/bugfix. This will help people understand your PR and can be used as a start for the documentation. Additionally (see https://symfony.com/releases): - Always add tests and ensure they pass. - Never break backward compatibility (see https://symfony.com/bc). - Bug fixes must be submitted against the lowest maintained 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 branch 5.x. - Changelog entry should follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry --> Commits ------- 77bd21a [HttpFoundation] Fix isNotModified determination logic
| public function testIsNotModifiedWeakEtag() | ||
| { | ||
| $etag = 'randomly_generated_etag'; | ||
| $weakEtag = 'W/randomly_generated_etag'; |
There was a problem hiding this comment.
Thank you for noticing.
Could you submit a PR fixing this with a small explanation?
Use weak comparison for etags; Only do if-modified-since when no if-none-match header is present.
See #37948 (comment)