Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
OskarStark commentedMay 27, 2024
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 commentedMay 27, 2024
@OskarStark I kinda agree but BC can be a touchy/tough subject so better to bring up the question early. |
smnandre commentedMay 27, 2024
💐 -- 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 of 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 " (or any better name, but you got the idea), that would avoid any BC ? |
bradjones1 commentedMay 28, 2024
@smnandre Thanks for the input. I understand your point about hashes, however I don't really get this:
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 as 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 like |
nicolas-grekas commentedMay 28, 2024
I was thinking the same about ordering: your proposal would reorder things like 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 commentedMay 28, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
What about - public static function normalizeQueryString(?string $qs): string+ public static function normalizeQueryString(?string $qs, bool $recursive = false): string to prevent BC |
Request::normalizeQueryString()bradjones1 commentedMay 28, 2024
@nicolas-grekas Yeah, that's fair and why I anticipated some discussion on this point. The proposal for a recursive option to |
nicolas-grekas commentedJun 25, 2024
Making this optional would work for me. Inorder to not break BC, the argument should be added using |
fabpot commentedMar 29, 2025
Closing as there is no more activity and the current patch cannot be merged as is. |
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.