See D20136. This method is sort of inherently bad because it is destructive for some inputs (x=1&x=2) and had "PHP-flavored" behavior for other inputs (x=1&x=2). Move to explicit ...AsMap and ...AsPairList methods.
- rP1fd69f788cee: Replace "getQueryParams()" callsites in Phabricator
Bit of an adventure, see inlines in a minute.
Used Google OAuth.
Changed "Whitespace" option in Differential, then clicked "Download Raw Diff". Saw URI reflect whitespace change.
(This needs a minor adjustment since there are two more callsites, will followup.)
Changed a revision to "Whitespace: View All". Clicked "View Options > Show Raw File (Right)", saw "whitespace" parameter preserved in URI.
Visited some file in Diffusion, added ?zebra=quack to the URI, clicked "show older changes".
This did not preserve zebra=quack.
It doesn't preserve it in master either, though, since these URIs don't generate respecting query parameters in the first place. I'll just remove this.
Created an OAuth server for http://example.com/?v=1&v=1, hit /oauthserver/auth/?client_id=...&response_type=code&redirect_uri=http://example.com/?v=1, got a validation exception.
This error is very slightly opaque (it doesn't tell you what's wrong explicitly, just that the provided redirect URI does not match the specified redirect URI) but extremely hard to hit.
I think this is generally a step forward. Since I'm now planning to look at setQueryParam() / setQueryParams() too, there may be an opportunity to clean this up a bit more later on, but I think it's okay to land as-is.
I also checked for getQueryParams() in instances/ and services/ and found no callsites.