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