Page MenuHomePhabricator

Replace "getQueryParams()" callsites in Phabricator
ClosedPublic

Authored by epriestley on Feb 11 2019, 7:30 PM.
Tags
None
Referenced Files
F12819124: D20141.id.diff
Thu, Mar 28, 5:21 AM
Unknown Object (File)
Tue, Mar 26, 1:43 PM
Unknown Object (File)
Tue, Mar 26, 1:42 PM
Unknown Object (File)
Tue, Mar 26, 1:41 PM
Unknown Object (File)
Tue, Mar 26, 1:41 PM
Unknown Object (File)
Tue, Mar 26, 1:41 PM
Unknown Object (File)
Feb 10 2024, 12:09 PM
Unknown Object (File)
Jan 31 2024, 8:18 PM
Subscribers
None

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
Branch
pair2
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 21924
Build 29936: Run Core Tests
Build 29935: arc lint + arc unit

Event Timeline

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!
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.

  • 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!

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

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.