Page MenuHomePhabricator

Replace manual query string construction with "phutil_build_http_querystring()"

Authored by epriestley on Jan 29 2019, 3:38 AM.



Now that we have a nice function for this, use it to simplify some code.

Test Plan

Ran through the Duo enroll workflow to make sure signing still works.

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.Jan 29 2019, 3:38 AM
epriestley requested review of this revision.Jan 29 2019, 3:40 AM

There's also one of these in AphrontDefaultApplicationConfiguration. However, I'm not going to touch it because it's parsing input (which may be in the form x=1&x=2&x=3, even though we flatten them today) and it's entangled with D20046, and it uses urlencode() instead of rawurlencode() which is a bit messy.

The differences between the two functions, per this script:


foreach (range(0, 255) as $byte) {
  $c = chr($byte);

  $u = urlencode($c);
  $v = rawurlencode($c);
  $w = http_build_query(array($c => $c), '', '&');

  if (($u !== $v) || ("{$u}={$u}" !== $w)) {


0x20	+	%20	+=+
0x7e	%7E	~	%7E=%7E

...which actually makes me think that we should be passing PHP_QUERY_RFC3986 to http_build_query(), since I think we want the rawurlencode() behavior where space becomes %20.

I'll revisit this and maybe add some test coverage tomorrowish.

epriestley updated this revision to Diff 47886.Jan 29 2019, 1:35 PM
  • Rebase this on D20046.
  • Explain the ApplicationConfiguration case.
  • Switch it to rawurlencode() since that method generally has better behavior (see D20049 for rationale momentarily).
  • The input "x[]=apple&x[]=banana" is actually legitimate (parses into array('apple', 'banana') and sees at least some use in older code. This is probably a bad pattern (and it's a weird PHP-ism) but I don't think it's worth breaking for now.
amckinley accepted this revision.Jan 31 2019, 2:20 AM
This revision is now accepted and ready to land.Jan 31 2019, 2:20 AM
This revision was automatically updated to reflect the committed changes.