Page MenuHomePhabricator

Replace manual query string construction with "phutil_build_http_querystring()"
ClosedPublic

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

Details

Summary

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

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

<?php

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)) {
    printf(
      "0x%02x\t%s\t%s\t%s\n",
      $byte,
      $u,
      $v,
      $w);
  }
}

...are:

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.Tue, Jan 29, 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.Thu, Jan 31, 2:20 AM
This revision is now accepted and ready to land.Thu, Jan 31, 2:20 AM
This revision was automatically updated to reflect the committed changes.