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] Recursively order query string keys inRequest::normalizeQueryString()#57190

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

@bradjones1
Copy link
Contributor

QA
Branch?7.1
Bug fix?no
New feature?yes, changelog updated
Deprecations?no
LicenseMIT

I'm not quite sure how to handle this proposal (particularly as it comes to BC implications) but opening an MR to at least start the discussion and get some guidance.

Over in Drupal I am working ona bug where our cache is poisoned due to query parameters that are altered during our internal request handling. That bug is specific to Drupal's request and caching API, however the solution has me looking upstream with a potential improvement.

We need to add the request query parameters and the potentially altered set to the cache ID. To maximize cache hits (and also hedge against a bit of DDoS vectors, I think?) it's necessary to predictably order the query parameters, recursively. Symfony only sorts the top-level keys, and even has test coverage to ensure the ordering is limited to the top level.

I'm not sure there's any real benefit to this and the git log for the applicable test coverage dates to version 3.4 and I think is lumped in with "cleanup," so it's hard to know the motivation.

Technically speaking this would be a BC break because you get a differently-ordered query string out of the function, but it would work exactly the same. I know BC is important but it also seems like this might be a case where functional equivalence is fine?

It could also be that this just isn't important in Symfony's code and we can continue to address this directly in our code (as we will now, unless this merges fast) but I wanted to also raise this here.

OskarStark reacted with eyes emoji
@OskarStark
Copy link
Contributor

From my POV this looks like a feature but not a BC break. At least I can't imagine how this can break sth. 🤷🏼‍♂️😅

@bradjones1
Copy link
ContributorAuthor

@OskarStark I kinda agree but BC can be a touchy/tough subject so better to bring up the question early.

OskarStark reacted with thumbs up emoji

@smnandre
Copy link
Member

💐 -- very personal and opiniated message here -- 💐

To me it can be seen as amajor BC break... as values would bechanged here.

A query string is a map ofkey => value,... so the order ofkeys has no meaning. And reordering them has no impact on the data.

But the suggested change would modify thevalues, and i think this is something that would be really hard to justify.

Wether someone consider nested values as an hash, a map, a string, should not be guessed by the framework.

And as those values are not consider Identical in PHP, i do think this change is hard to justify.

$foo1 = ['key' => ['a' =>'A','b' =>'B']];$foo2 = ['key' =>  ['b' =>'B','a' =>'A']];var_dump($foo1 ===$foo2);// FALSEvar_dump($foo1 ==$foo2);// true$foo2 = ['key' =>  ['b' =>'B','a' =>'A']];// true$foo3 = ['key' => ['b' =>'B','a' =>'A']];// truevar_dump($foo2 ===$foo3);var_dump($foo2 ==$foo3);

But i understand clearly the expected usage here, and can imagine how it would ease DX in several cases...

So... what about some "getNormalizedQuery" method

(or any better name, but you got the idea), that would avoid any BC ?

@OskarStarkOskarStark mentioned this pull requestMay 28, 2024
@bradjones1
Copy link
ContributorAuthor

@smnandre Thanks for the input. I understand your point about hashes, however I don't really get this:

But the suggested change would modify the values, and i think this is something that would be really hard to justify.

We're not changing any values, unless you consider the specific order of K/V pairs in an array to be a "value." I suppose there could be some discussion around whether the ordering of multiple values such asa[]=1&a[]=2 should carry meaning from the client side in their order... but that's very brittle.

You provide an example of comparing two arrays for strict equivalence, but I don't believe that example really carries much water when we're talking about query string parameters. Because their order may be determined by the client, server implementations shouldn't depend on such comparisons but rather look at the values overall or doing something more likein_array($request->query->all('a')).

@nicolas-grekas
Copy link
Member

a[]=1&a[]=2 should carry meaning from the client side in their order...

I was thinking the same about ordering: your proposal would reorder things likea[foo]=1&a[bar]=2, and I'm very confident that there are UIs out there that do use Symfony and that do rely on the ordering (eg when providing a UI to order rows in a table). I don't think that's brittle - it just works and has no reason to be unpredictable.

This means I'm on the 👎 side of this proposal, it might be acceptable in Drupal, but in the more generic case of Symfony, I don't think so.

@OskarStark
Copy link
Contributor

OskarStark commentedMay 28, 2024
edited
Loading

What about

- public static function normalizeQueryString(?string $qs): string+ public static function normalizeQueryString(?string $qs, bool $recursive = false): string

to prevent BC

@OskarStarkOskarStark changed the title[HttpFoundation] Recursively order query string keys in Request::normalizeQueryString()[HttpFoundation] Recursively order query string keys inRequest::normalizeQueryString()May 28, 2024
@bradjones1
Copy link
ContributorAuthor

@nicolas-grekas Yeah, that's fair and why I anticipated some discussion on this point. The proposal for a recursive option to::normalizeQueryString() is what I had first thought of, and perhaps that's still in play.

@xabbuhxabbuh modified the milestones:7.1,7.2May 31, 2024
@nicolas-grekas
Copy link
Member

Making this optional would work for me. Inorder to not break BC, the argument should be added using@param only and be accessed using func_get_arg

smnandre reacted with thumbs up emojiOskarStark reacted with hooray emoji

@fabpotfabpot modified the milestones:7.2,7.3Nov 20, 2024
@fabpot
Copy link
Member

Closing as there is no more activity and the current patch cannot be merged as is.
Feel free to reopen if that still makes sense.

@fabpotfabpot closed thisMar 29, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

7.3

Development

Successfully merging this pull request may close these issues.

7 participants

@bradjones1@OskarStark@smnandre@nicolas-grekas@fabpot@xabbuh@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp