Page MenuHomePhabricator

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

Authored by epriestley on Jan 29 2019, 3:38 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 27, 5:21 PM
Unknown Object (File)
Wed, Mar 27, 5:20 PM
Unknown Object (File)
Wed, Mar 27, 5:20 PM
Unknown Object (File)
Wed, Mar 27, 5:20 PM
Unknown Object (File)
Sat, Mar 23, 12:33 PM
Unknown Object (File)
Sat, Mar 23, 12:33 PM
Unknown Object (File)
Sat, Mar 23, 12:33 PM
Unknown Object (File)
Sat, Mar 23, 12:33 PM
Subscribers
None

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
Branch
qstring1
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 21741
Build 29665: Run Core Tests
Build 29664: arc lint + arc unit

Event Timeline

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.

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