Page MenuHomePhabricator

Introduce new PhutilURI API methods to move away from the ambiguity of the "set" operation
ClosedPublic

Authored by epriestley on Feb 12 2019, 3:33 PM.

Details

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

  1. /x/?y=1&y=2
  2. /x/?y=2
  3. /x/?y=1
  4. 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".

epriestley updated this revision to Diff 48112.Feb 12 2019, 5:03 PM
  • Wow, that's a real big exception message.
epriestley planned changes to this revision.Feb 12 2019, 5:03 PM

(Still iterating on this.)

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

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

Options are:

  1. /x/?y
  2. /x/?y=
  3. /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?

epriestley updated this revision to Diff 48113.Feb 12 2019, 6:45 PM
  • When initializing a URI, skip null parameters.

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?)

epriestley planned changes to this revision.Feb 12 2019, 8:55 PM

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

epriestley updated this revision to Diff 48118.Feb 12 2019, 9:21 PM
  • Just remove appendQueryParamsAsMap(map) since we don't seem to actually need it anywhere.
epriestley edited the summary of this revision. (Show Details)Feb 12 2019, 9:25 PM
  • Make new PhutilURI(<uri object>, map) work.
  • Update unit tests.
  • Add more constructor unit tests.
  • Throw on replaceQueryParam(key, null).
  • More phutil_string_cast().
  • Make "ambiguous construction: the novel" more concise.

This is probably good to go now. πŸ•

amckinley accepted this revision.Feb 14 2019, 7:24 PM
amckinley added inline comments.
src/parser/PhutilURI.php
379

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
379

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.

This revision was automatically updated to reflect the committed changes.