Details

Reviewers

amckinley

Maniphest Tasks

T13250: Some typeaheads use nonscalar HTTP parameters, which fatal under new "phutil_build_http_query()" rules

Commits

rPHUf8b3f480f91a: Introduce new PhutilURI API methods to move away from the ambiguity of the…

Summary

Ref T13250. See D20136. Problems:

setQueryParams(map) is currently ambiguous. Does it remove all existing params first? (Yes.) Or just remove the specified params? (No.)
setQueryParam($key, $value) has a lot of compatibility logic to retain order, and it isn't obvious that all copies of the named parameter are replaced.
setQueryParam($key, null) is some secret magic to remove a parameter.

Solutions?

New method removeAllQueryParams() is unambiguous.
New method removeQueryParam(key) is unambiguous and replaces setQueryParam(key, null).
New method replaceQueryParam(key, value) removes all copies of the named key and then appends the new key-value pair.

These all seem pretty solid and unambiguous. Then, this is a little dicier:

~~New method appendQueryParamsAsMap(map) is an unambiguous append.~~ - Never mind, we don't need it!

I like that this is unambiguous. I don't like that the operation we most often want may be "remove all keys in this map, then append them", and this doesn't do that. It's possible it will lead to bugs where we get ?id=x&id=y if misused.

Methods I'm NOT introducing:

replaceAllQueryParamsAsMap(map): This is useful, but I think it's unclear whether this means "clear EVERYTHING, then append these" or "clear JUST THESE KEYS, then append them". I'd introduce this if I could come up with a very clear name for it that's more concise than removeAllQueryParamsThenAppendThisMapOfQueryParams(map).
replaceAllQueryParams(pair_list): Probably not useful since calling code rarely has pair lists, and if it's looping to build one it could just call replaceQueryParam(key, value) anyway.
Any methods for interacting with x=1&x=2&x=3 stuff. We never really interact with this stuff, and you can remove + append if you really need to.

I'm going to hold this until I've converted enough stuff to be confident that usage actually matches the API.

Test Plan

Only new methods, see followup changes.

Diff Detail

Repository

rPHU libphutil

Branch

uri1

Lint

Lint Passed

Severity	Location	Code	Message
Advice	src/parser/PhutilURI.php:361	XHP62	Class Name Literal

Unit

Tests Passed

Build Status

Buildable 21955
Build 29981: Run Core Tests
Build 29980: arc lint + arc unit

Event Timeline

epriestley created this revision.Feb 12 2019, 3:33 PM

Harbormaster completed remote builds in B21953: Diff 48110.Feb 12 2019, 3:33 PM

epriestley requested review of this revision.Feb 12 2019, 3:33 PM

The majority of setQueryParams(map) callsites in rP are id(new PhutilURI($path))->setQueryParams($map).

I think I'm going to support new PhutilURI($path, $map_of_parameters) instead, similar to how HTTPFutures work. This generally sidesteps the issue of appendQueryParamsAsMap() being somewhat unwieldy.

This runs into a mild issue here. What should this do?

new PhutilURI('/x/?y=1', array('y' => 2));

I think the options are:

/x/?y=1&y=2
/x/?y=2
/x/?y=1
Throw an exception.

Option (3) seems wildly surprising. I don't think anyone ever actually wants option (1), even though it's the cleanest representation of the request literally as written.

Option (2) is probably what you want if you're doing this on purpose. Option (4) is definitely what you want if you're doing this by accident.

For almost every (or, perhaps, every) callsite this is moot anyway, because they're all using static path strings. Thus, I'm inclined to throw for now, mostly because it's safer. We could consider a "replace" API later if this causes issues -- it's easier to go from "exception" to "new behavior" than from "useful behavior" to "exception".

Wow, that's a real big exception message.

(Still iterating on this.)

Harbormaster completed remote builds in B21955: Diff 48112.Feb 12 2019, 5:04 PM

Here's another question -- what should this do?

new PhutilURI('/x/', array('y' => null));

Options are:

/x/?y
/x/?y=
/x/

We currently do (3). I think this is desirable and reasonable, even though there's an argument that it isn't as faithful as possible to exactly what the user provided. In practice, it's common and convenient, and basically no reasonable software uses ?y or ?y=, and you can pass array('y' => '') if you really truly want ?y=. There's no way to get ?y. This is vaguely "bad" in the sense that it's slightly destructive (parsing and then serializing /x/?y won't result in the output matching the input), but any program relying on the distinction between ?y= vs ?y surely has bigger issues?

When initializing a URI, skip null parameters.

Harbormaster completed remote builds in B21956: Diff 48113.Feb 12 2019, 6:45 PM

epriestley mentioned this in D20151: Replace "URI->setQueryParams()" after initialization with a constructor argument.Feb 12 2019, 7:02 PM

new PhutilURI('/x/?y=1', array('y' => 2));

I like throwing an exception. I would also probably support the more extremist view that using this constructor with parameters in the path and in the new, 2nd argument should also be an exception, regardless of whether or not those parameters conflict with each other.

new PhutilURI('/x/', array('y' => null));

I think the current behavior is fine (notwithstanding my fundamentalist opinions above).

(Is this ready for review in general?)

This probably isn't going to change too much more, but I'm at least going to get some test coverage on it.

Just remove appendQueryParamsAsMap(map) since we don't seem to actually need it anywhere.

Harbormaster completed remote builds in B21962: Diff 48118.Feb 12 2019, 9:22 PM

epriestley mentioned this in D20153: Clean up final `setQueryParams()` callsites.Feb 12 2019, 9:23 PM

epriestley edited the summary of this revision. (Show Details)Feb 12 2019, 9:25 PM

epriestley mentioned this in D20154: Replace all "setQueryParam()" calls with "remove/replaceQueryParam()".Feb 12 2019, 9:38 PM

Make new PhutilURI(<uri object>, map) work.
Update unit tests.
Add more constructor unit tests.
Throw on replaceQueryParam(key, null).

Harbormaster completed remote builds in B21965: Diff 48121.Feb 12 2019, 10:24 PM

More phutil_string_cast().
Make "ambiguous construction: the novel" more concise.

Harbormaster completed remote builds in B21966: Diff 48122.Feb 12 2019, 10:28 PM

This is probably good to go now. 🐕

epriestley mentioned this in D20155: Update "setQueryParam[s]()" calls in libphutil.Feb 12 2019, 10:35 PM

amckinley accepted this revision.Feb 14 2019, 7:24 PM

amckinley added inline comments.

src/parser/PhutilURI.php
369	Shouldn't we throw here instead of just swallowing a null value as though we did something with it?

This revision is now accepted and ready to land.Feb 14 2019, 7:24 PM

epriestley added inline comments.Feb 14 2019, 7:41 PM

src/parser/PhutilURI.php
369	I'm intentionally allowing `null` to mean "no value" in the constructor, since there were a number of callsites like: $params = array( 'class' => $symbol->getClass(), 'line' => $symbol->getLine(), 'context' => $symbol->getContext(), 'language' => $symbol->getLanguage(), ); ...where several parameters may be `null`. If we can't pass `null`, this turns into 20 lines of null tests, or we just end up wrapping it in some `filter_nulls_out()` method which we don't currently have (`array_filter()` filters all "falsey" values). I think mistakes are unlikely here / the intent is fairly clear / the behavior is significantly common and useful.