HomePhabricator

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

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

Description

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

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.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13250

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