Page MenuHomePhabricator

Replace "getQueryParams()" callsites in Phabricator

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



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

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

Event Timeline

epriestley created this revision.Feb 11 2019, 7:30 PM
Harbormaster returned this revision to the author for changes because remote builds failed.Feb 11 2019, 7:32 PM
Harbormaster failed remote builds in B21923: Diff 48084!
epriestley added inline comments.Feb 11 2019, 7:57 PM

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, hit /oauthserver/auth/?client_id=...&response_type=code&redirect_uri=, 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.


Not gnomed.

Not gnomed.


epriestley updated this revision to Diff 48085.Feb 11 2019, 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.Feb 11 2019, 8:01 PM
Harbormaster failed remote builds in B21924: Diff 48085!
epriestley requested review of this revision.Feb 11 2019, 8:01 PM

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

amckinley accepted this revision.Feb 11 2019, 11:02 PM
This revision is now accepted and ready to land.Feb 11 2019, 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.