Skip to content

[HttpClient] allow arbitrary JSON values in requests #34216

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 7, 2019
Merged

[HttpClient] allow arbitrary JSON values in requests #34216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Symfony/Component/HttpClient/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
-----
Expand Down
6 changes: 1 addition & 5 deletions src/Symfony/Component/HttpClient/HttpClientTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/HttpClient/HttpOptions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public function setBody($body)
}

/**
* @param array|\JsonSerializable $json
* @param mixed $json
*
* @return $this
*/
Expand Down
8 changes: 4 additions & 4 deletions src/Symfony/Contracts/HttpClient/HttpClientInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down