Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[HttpFoundation] Fix isNotModified determination logic#40545

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@ol0lll
Copy link
Contributor

QA
Branch?4.4
Bug fix?yes
New feature?no
Deprecations?no
TicketsFix#37948
LicenseMIT
Doc PRsymfony/symfony-docs#...

Use weak comparison for etags; Only do if-modified-since when no if-none-match header is present.
See#37948 (comment)

@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (seehttps://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://symfony.com/releases)
  • Features and deprecations must be submitted against the 5.x branch.

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!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@ol0lllol0lll changed the base branch from5.x to4.4March 22, 2021 15:48
@nicolas-grekasnicolas-grekas added this to the4.4 milestoneMar 22, 2021
@nicolas-grekas
Copy link
Member

This breaks some existing tests, please have a look, and add more if it makes sense.

OskarStark reacted with thumbs up emoji

@ol0lllol0lllforce-pushed thefix-isNotModified-determination-logic branch fromb9f64b4 to80f278fCompareMarch 23, 2021 16:50
@ol0lllol0lllforce-pushed thefix-isNotModified-determination-logic branch 2 times, most recently from25696a3 to352b1f0CompareMarch 25, 2021 16:42
@ol0lllol0lll closed thisMar 25, 2021
@ol0lllol0lll reopened thisMar 25, 2021
@ol0lll
Copy link
ContributorAuthor

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?

@xabbuh
Copy link
Member

In thecomposer.json file of the HttpKernel component you need to update the HttpFoundation constraints like this:

"symfony/http-foundation": "^4.4.21|^5.2.6"

"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",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

should be bumped to^4.4.22|^5.2.7 now that 4.4.21 and5.2.6 have been released

@ol0lllol0lllforce-pushed thefix-isNotModified-determination-logic branch frombefd41e to3befea3CompareMarch 30, 2021 15:04
@Nyholm
Copy link
Member

Thank you.

Travis is failing and when I reviewed the output I suspect that the failure is related to this PR. Can you confirm?

@ol0lll
Copy link
ContributorAuthor

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:

alexpott 30 Mar at 16:49
@xabbuh @nicolasgrekas I’m trying to help a friend with a Symfony PR -#40545 - the problem is that HttpKernel tests are failing because HttpFoundation is changing. The PR passes locally when running the tests with both components patches. I’ve looked at the .travis.yml file and I’ve tried to work out how it determines to use components that have changed as part of MR as dependencies but I can’t see how. I feel like I missing something obvious

xabbuh 13 days ago
in this case, the deps=high job is allowed to fail

xabbuh 13 days ago
deps=low passes that’s important

xabbuh 13 days ago
the other job will pass once the branch is merged up

alexpott 13 days ago
Ah thanks… and I was about to ask the question that @nicolasgrekas just answered on the PR - thanks to both you!

xabbuh 13 days ago
ah indeed, I missed that we had new releases after the latest PR update

@TheNewSound
Copy link

TheNewSound commentedApr 12, 2021•
edited
Loading

uses wrong header tag,if_none_match should beIf-None-Match

EDIT: shouldn't matter since _ are replaced by - and capital letters always to lowercase.

ol0lll reacted with thumbs up emoji

@ol0lllol0lllforce-pushed thefix-isNotModified-determination-logic branch fromf35615d to6b2a7f4CompareApril 12, 2021 17:58
@TheNewSound

This comment has been minimized.

@maxhelias
Copy link
Contributor

Needs a rebase to fix conflicts

@ol0lllol0lllforce-pushed thefix-isNotModified-determination-logic branch from6b2a7f4 to07d51e2CompareMay 17, 2021 12:14
@PF4Public
Copy link
Contributor

This should probably resolve sensiolabs/SensioFrameworkExtraBundle#513 in a better way than simply adding weak ETags!

@fabpot
Copy link
Member

Thank you@ol0lll.

fabpot added a commit that referenced this pull requestAug 26, 2021
…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 peopleunderstand your PR and can be used as a start for the documentation.Additionally (seehttps://symfony.com/releases): - Always add tests and ensure they pass. - Never break backward compatibility (seehttps://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 followhttps://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry-->Commits-------77bd21a [HttpFoundation] Fix isNotModified determination logic
@fabpotfabpot closed thisAug 26, 2021
This was referencedAug 30, 2021
@chrfritschchrfritsch deleted the fix-isNotModified-determination-logic branchDecember 6, 2021 11:56
publicfunctiontestIsNotModifiedWeakEtag()
{
$etag ='randomly_generated_etag';
$weakEtag ='W/randomly_generated_etag';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thank you for noticing.
Could you submit a PR fixing this with a small explanation?

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@NyholmNyholmNyholm left review comments

@xabbuhxabbuhxabbuh left review comments

@fabpotfabpotfabpot approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

[HttpFoundation] ETag for If-None-Match comparison

9 participants

@ol0lll@carsonbot@nicolas-grekas@xabbuh@Nyholm@TheNewSound@maxhelias@PF4Public@fabpot
[8]ページ先頭
©2009-2025 Movatter.jp