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.
Details
Details
- Reviewers
amckinley - Commits
- rP1fd69f788cee: Replace "getQueryParams()" callsites in Phabricator
Bit of an adventure, see inlines in a minute.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| src/applications/auth/provider/PhabricatorAuthProvider.php | ||
|---|---|---|
| 441–448 | Used Google OAuth. | |
| src/applications/differential/controller/DifferentialRevisionViewController.php | ||
| 1101–1107 | Changed "Whitespace" option in Differential, then clicked "Download Raw Diff". Saw URI reflect whitespace change. | |
| src/applications/differential/view/DifferentialChangesetListView.php | ||
| 362–375 | (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. | |
| src/applications/diffusion/controller/DiffusionBrowseController.php | ||
| 711–712 | 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. | |
| src/applications/oauthserver/PhabricatorOAuthServer.php | ||
| 259–261 | 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. | |
| src/infrastructure/markup/rule/PhabricatorYoutubeRemarkupRule.php | ||
| 31–33 | https://www.youtube.com/watch Not gnomed. https://www.youtube.com/watch?v=6n3pFFPSlW4&v=1 Not gnomed. https://www.youtube.com/watch?v=6n3pFFPSlW4 Gnomed. | |
Comment Actions
- Remove nonfunctional code in Diffusion browse view.
- Apply the Changeset change to the other callsites.
Comment Actions
Builds won't pass until D20136 lands, since it introduces the ...asMap and ...asPairList stuff.
Comment Actions
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.