Page MenuHomePhabricator

Replace "getQueryParams()" callsites in Phabricator
ClosedPublic

Authored by epriestley on Mon, Feb 11, 7:30 PM.

Details

Summary

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.

Test Plan

Bit of an adventure, see inlines in a minute.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Mon, Feb 11, 7:30 PM
Harbormaster returned this revision to the author for changes because remote builds failed.Mon, Feb 11, 7:32 PM
Harbormaster failed remote builds in B21923: Diff 48084!
epriestley added inline comments.Mon, Feb 11, 7:57 PM
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.

epriestley updated this revision to Diff 48085.Mon, Feb 11, 8:00 PM
  • Remove nonfunctional code in Diffusion browse view.
  • Apply the Changeset change to the other callsites.
Harbormaster returned this revision to the author for changes because remote builds failed.Mon, Feb 11, 8:01 PM
Harbormaster failed remote builds in B21924: Diff 48085!
epriestley requested review of this revision.Mon, Feb 11, 8:01 PM

Builds won't pass until D20136 lands, since it introduces the ...asMap and ...asPairList stuff.

amckinley accepted this revision.Mon, Feb 11, 11:02 PM
This revision is now accepted and ready to land.Mon, Feb 11, 11:02 PM

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.

This revision was automatically updated to reflect the committed changes.