NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.7k

[HttpFoundation] Fix AcceptHeader overwrites items with different parameters#62287

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

Jump to bottom

Open

yoeunes wants to merge1 commit intosymfony:7.3

base:7.3

Choose a base branch

fromyoeunes:httpfoundation-acceptheader-params

Open

[HttpFoundation] Fix AcceptHeader overwrites items with different parameters#62287

yoeunes wants to merge1 commit intosymfony:7.3fromyoeunes:httpfoundation-acceptheader-params

+489 −5

Conversation

Copy link

Contributor

yoeunes commentedNov 3, 2025

Q	A
Branch?	7.3
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Issues	Fixes#62282
License	MIT

This PR fixes a bug inAcceptHeader where items with the same media type but different parameters (e.g.,text/plain;format=flowed andtext/plain;format=fixed) would overwrite each other.

Theadd() method now uses a canonical key that includes all parameters (exceptq), ensuring distinct items are stored correctly.

Consequently, theget() method has been updated to perform proper content negotiation (as described in RFC 9110), including matching parameters and sorting by specificity (e.g.,text/plain >text/* >*/*). New tests are added to cover these cases.

carsonbot added Status: Needs Review Bug HttpFoundation labels

Nov 3, 2025

carsonbot added this to the7.3 milestone

Nov 3, 2025

nicolas-grekas reviewed

Nov 3, 2025

View reviewed changes

src/Symfony/Component/HttpFoundation/Tests/AcceptHeaderTest.php OutdatedShow resolvedHide resolved

nicolas-grekas reviewed

Nov 3, 2025

View reviewed changes

src/Symfony/Component/HttpFoundation/AcceptHeader.php OutdatedShow resolvedHide resolved

yoeunes force-pushed thehttpfoundation-acceptheader-params branch 2 times, most recently fromb72a7a0 toc99d747Compare

November 3, 2025 10:02

Copy link

Contributor

Spomky commentedNov 3, 2025

Hi@yoeunes,

From what I understand, this looks more like an enhancement of the existing behaviour rather than a bug fix. Is that correct?
If it is indeed a bug fix, it should probably target 6.4 instead of 7.4.

Thanks!

Copy link

ContributorAuthor

yoeunes commentedNov 3, 2025•
edited
Loading

Hi@Spomky, I consider this a bug fix because the current implementation incorrectly overwrites header items (e.g.,text/plain;format=flowed (q=1.0) is lost iftext/plain;format=fixed (q=0.4) is also present) which breaks content negotiation

If it's confirmed as a bug fix, should I rebase this onto 6.4 instead? Let me know your thoughts or if more details are needed

andesk reviewed

Nov 3, 2025

View reviewed changes

Copy link

Contributor

andesk left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to look into the issue I've created yesterday! 👍

It feels like the implementation might be a bit brittle in regards of some assumptions made on the case-(in)sensitivity comparison of two media type strings? Also, certainly having more data to take into account for matching makes things harder. Having test coverage is great, maybe we can improve/simplify the logic still in some areas?

src/Symfony/Component/HttpFoundation/AcceptHeader.php OutdatedShow resolvedHide resolved

yoeunes force-pushed thehttpfoundation-acceptheader-params branch 6 times, most recently from9e19845 tobebe53dCompare

November 3, 2025 19:36

Copy link

ContributorAuthor

yoeunes commentedNov 3, 2025

Hi@andesk, I'm consolidating my responses to your comments here to keep the thread tidy.

Thanks for the detailed review. I've pushed a full refactor that applies your suggestions, makes the code more readable, and adds explanatory comments.

Regarding case-insensitivity, you are correct. I'm now normalizing parameter names usingstrtolower() when building the canonical key. This aligns with RFC 9110 (Section 5.6.6) and correctly handles duplicates (e.g.,Charset vscharset).
https://www.rfc-editor.org/rfc/rfc9110.html#section-5.6.6

The PR should be ready for another look. Thanks!

yoeunes force-pushed thehttpfoundation-acceptheader-params branch frombebe53d toc1dee8fCompare

November 3, 2025 19:57

derrabus reviewed

Nov 5, 2025

View reviewed changes

Copy link

Member

derrabus left a comment

Choose a reason for hiding this comment

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

Thank you for working on this issue.

I think, we should add additional tests forRequest::getAcceptableContentTypes() andRequest::getPreferredFormat() because they're directly impacted by this change.

src/Symfony/Component/HttpFoundation/AcceptHeader.php OutdatedShow resolvedHide resolved

src/Symfony/Component/HttpFoundation/Tests/AcceptHeaderTest.php OutdatedShow resolvedHide resolved

yoeunes force-pushed thehttpfoundation-acceptheader-params branch 2 times, most recently from7f880eb to284531eCompare

November 5, 2025 12:57

Copy link

ContributorAuthor

yoeunes commentedNov 5, 2025

Hi@derrabus, thank you for your review and feedback!

I've just pushed an update to add the requested tests forRequest::getAcceptableContentTypes() andRequest::getPreferredFormat(), covering scenarios with parameters, quality sorting, wildcards, case-insensitivity

Let me know if anything else is needed.