HomePhabricator

Fix destructive representation of "?x=1&x=2" in PhutilURI so it can raise…

Authored by epriestley on Feb 11 2019, 4:20 PM.

Description

Fix destructive representation of "?x=1&x=2" in PhutilURI so it can raise errors immediately

Summary:
Ref T13250. Currently, PhutilURI uses a map to represent parameters internally. This is destructive for query strings in the form x=1&x=2, and has PHP-flavored behavior for query strings in the form x[]=1&x[]=2.

In the case of T13250, it would be better for PhutilURI to raise an exception earlier (when a nonscalar parameter is set), rather than later (in __toString()). Partly, handling exceptions from __toString() in PHP is a mess, since PHP fatals. Partly, raising errors sooner is generally better.

Since we use query strings in the x[]=... form, and may possibly rely on PhutilURI being able to parse and represent them, we probably can't make PhutilURI reject URIs in this form. The destructive behavior on x=1&x=2 also "feels bad", even if it does not concretely cause any specific problems.

To fix this stuff:

  • Change the internal representation from a map to a list of pairs.
  • In new PhutilURI(X), parse X to a list of pairs.
  • Retain the same semantics for setQueryParam() (replace keys).
  • Add a new appendQueryParam() with new semantics (add keys, even if they are duplicates).
  • Then, add strict error checking on parse/set so errors are raised immediately.

The behavioral changes are:

  • set/append/QueryParam(X, Y) now raises an exception if X or Y are nonscalar, and setQueryParams(M) now raises an exception if any key or value in M is nonscalar. This is good, and the primary goal of the change.
  • ?x=1&x=2 is no longer parsed destructively. This is good.
  • getQueryParams() no longer contains an 'x' => array(1, 2) for ?x[]=1&x[]=2. Instead, it will return 'x[]' => '2'. That is, it has lost the PHP-specific notion of what x[]=1&x[]=2 means. I claim we never use this behavior and never expect PhutilURI to have it, and I think this is generally a good change, although it's possible it breaks something.
    • You can still get the full list with getQueryParamsAsPairList().
    • You can use PhutilQueryStringParser to explicitly say "I want PHP behavior". I don't think we ever want it, and it's good if opting into PHP behavior is explicit.

Test Plan:

  • Added unit tests, ran unit tests.
  • Without D20134, loaded one of the affected interfaces. Got a more useful exception sooner (when building the URI) rather than a less useful exception later (when converting the URI into a string).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250

Differential Revision: https://secure.phabricator.com/D20136