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.
Tags
None
Referenced Files
F12853094: D20149.id48122.diff
Fri, Mar 29, 8:03 AM
F12842506: D20149.diff
Thu, Mar 28, 10:04 PM
F12838171: D20149.id48158.diff
Thu, Mar 28, 5:56 PM
Unknown Object (File)
Thu, Feb 29, 5:20 PM
Unknown Object (File)
Feb 14 2024, 12:36 PM
Unknown Object (File)
Feb 8 2024, 1:46 PM
Unknown Object (File)
Feb 8 2024, 9:08 AM
Unknown Object (File)
Jan 26 2024, 5:42 AM
Subscribers
None

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
Branch
uri1
Lint
Lint Passed
SeverityLocationCodeMessage
Advicesrc/parser/PhutilURI.php:361XHP62Class Name Literal
Unit
Tests Passed
Build Status
Buildable 21955
Build 29981: Run Core Tests
Build 29980: arc lint + arc unit

Event Timeline

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

  • Wow, that's a real big exception message.

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

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

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

This revision was automatically updated to reflect the committed changes.