Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HttpClient] allow arbitrary JSON values in requests #34216

Merged
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

[HttpClient] allow arbitrary JSON values in requests

Allow arbitrary values in the "json" request option. Previously values were
limitated to arrays and objects of type JsonSerializable. This doesn't account
for scalar values and classes with public properties (which don't need to
implement JsonSerializable), all of which are perfectly acceptable arguments to
json_encode.
  • Loading branch information
pschultz committed Nov 1, 2019
commit e98731aabf01b39814e2b640a2fef7a2acc19cfd
@@ -15,6 +15,7 @@ CHANGELOG
* added `TraceableHttpClient`, `HttpClientDataCollector` and `HttpClientPass` to integrate with the web profiler
* allow enabling buffering conditionally with a Closure
* allow option "buffer" to be a stream resource
* allow arbitrary values for the "json" option

4.3.0
-----
@@ -332,18 +332,14 @@ private static function normalizePeerFingerprint($fingerprint): array
}
/**
* @param array|\JsonSerializable $value
* @param mixed $value
*
* @throws InvalidArgumentException When the value cannot be json-encoded
*/
private static function jsonEncode($value, int $flags = null, int $maxDepth = 512): string
{
$flags = $flags ?? (JSON_HEX_TAG | JSON_HEX_APOS | JSON_HEX_AMP | JSON_HEX_QUOT | JSON_PRESERVE_ZERO_FRACTION);
if (!\is_array($value) && !$value instanceof \JsonSerializable) {

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Nov 6, 2019

Member

Should null be supported too? if yes then the isset L71 should be replaced by array_key_exists()
This would prevent setting a default "json" option and being able to override it with setting a "body" one later one.
But setting a default json/body makes no sense to me so doesn't have to be supported that much.

This comment has been minimized.

Copy link
@pschultz

pschultz Nov 7, 2019

Author Contributor

Hmm, good catch, I haven't thought about that.

HttpClientInterface::OPTIONS_DEFAULTS is the only place where options are documented, isn't it? I don't like removing the json option there.

Also, it's somewhat dangerous to change the meaning of null from "no request body" to "request body containing null". Sure, we could point that out in the changelog, but you probably know as well as I do how good people are at reading changelogs, especially if the client package gets updated as a transitive dependency.

I'm leaning towards not supporting null.

throw new InvalidArgumentException(sprintf('Option "json" must be array or JsonSerializable, %s given.', \is_object($value) ? \get_class($value) : \gettype($value)));
}
try {
$value = json_encode($value, $flags | (\PHP_VERSION_ID >= 70300 ? \JSON_THROW_ON_ERROR : 0), $maxDepth);
} catch (\JsonException $e) {
@@ -86,7 +86,7 @@ public function setBody($body)
}
/**
* @param array|\JsonSerializable $json
* @param mixed $json
*
* @return $this
*/
@@ -33,11 +33,11 @@ interface HttpClientInterface
'query' => [], // string[] - associative array of query string values to merge with the request's URL
'headers' => [], // iterable|string[]|string[][] - headers names provided as keys or as part of values
'body' => '', // array|string|resource|\Traversable|\Closure - the callback SHOULD yield a string
// smaller than the amount requested as argument; the empty string signals EOF; when
// smaller than the amount requested as argument; the empty string signals EOF; if
// an array is passed, it is meant as a form payload of field names and values
'json' => null, // array|\JsonSerializable - when set, implementations MUST set the "body" option to
// the JSON-encoded value and set the "content-type" headers to a JSON-compatible
// value if they are not defined - typically "application/json"
'json' => null, // mixed - if set, implementations MUST set the "body" option to the JSON-encoded
// value and set the "content-type" header to a JSON-compatible value if it is not
// explicitly defined in the headers option - typically "application/json"
'user_data' => null, // mixed - any extra data to attach to the request (scalar, callable, object...) that
// MUST be available via $response->getInfo('user_data') - not used internally
'max_redirects' => 20, // int - the maximum number of redirects to follow; a value lower than or equal to 0
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.