-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[HttpFoundation] Recursively order query string keys in Request::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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
From my POV this looks like a feature but not a BC break. At least I can't imagine how this can break sth. 🤷🏼♂️😅 |
@OskarStark I kinda agree but BC can be a touchy/tough subject so better to bring up the question early. |
💐 -- very personal and opiniated message here -- 💐 To me it can be seen as a major BC break... as values would be changed here. A query string is a map of But the suggested change would modify the values, 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); // FALSE
var_dump($foo1 == $foo2); // true
$foo2 = ['key' => ['b' => 'B', 'a' => 'A']]; // true
$foo3 = ['key' => ['b' => 'B', 'a' => 'A']]; // true
var_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 ? |
@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 |
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. |
What about - public static function normalizeQueryString(?string $qs): string
+ public static function normalizeQueryString(?string $qs, bool $recursive = false): string to prevent BC |
Request::normalizeQueryString()
@nicolas-grekas Yeah, that's fair and why I anticipated some discussion on this point. The proposal for a recursive option to |
Making this optional would work for me. Inorder to not break BC, the argument should be added using |
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 on a 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.