[HttpClient] allow arbitrary JSON values in requests #34216
Conversation
831cc35 to
4b75a5a
Compare
|
I think allowing stdclass would make sense. I'm really unsure about scalars. That doesn't look like valid API design to me so I don't feel the need support them much. There is "body" if ppl really want them. |
That still ignores all other object types: class Book
{
public $title = '';
public $author = '';
}
That's not for Symfony do decide, especially not for a client which has to work with existing servers. There are plenty of services that distingish between an empty object and null, for example. If the json option doesn't allow all valid values, library authors have no choice but to ignore the json option and implement the encoding themselves. What is the reason for the type restriction? I don't see any downside in removing it. Passing any value that json_encode can't work with will still throw an InvalidArgumentException. |
To me, we don't want to allow arbitrary objects, or that's quite dangerous as one can very easily provide something that is not meant to be sent on the wire. In you example,
Can you provide an example please? E.g a link to a documentation?
That'd be fair to me - such libs would take responsibility for the type broadening - ensuring some guard (or not) on their own.
On top of the small safety barrier, this is also consistent with
That's the issue: json_encode works with way too many objects. At least that's the current reasoning. |
As a library author I can't do that, because JsonSerialize may be implemented.
I don't follow this reasoning. json_encode supports all those values that can be represented in JSON. And checking the "top-level" value doesn't change anything about that anyway. It seems silly to me to disallow class instances at the top level but allowing them as an array value (i.e. I was just surprised that the json option isn't just an equivalent for |
|
/cc @symfony/mergers any opinion here? |
|
+1 for this. Ref. php-http/psr7-integration-tests#40 |
| * | ||
| * @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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| '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 - when set, implementations MUST set the "body" option to the JSON-encoded |
There was a problem hiding this comment.
Changed it to "if set". We either have to remove the default option completely to support null, or null is inconsequential, making "set" the correct term.
|
I'm not able to push on your repo to update the patch. |
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.
001a502 to
e98731a
Compare
|
Rebased on 4.4. |
|
Thank you @pschultz. |
…pschultz) This PR was merged into the 4.4 branch. Discussion ---------- [HttpClient] allow arbitrary JSON values in requests | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tickets | n/a | License | MIT | Doc PR | n/a 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](https://www.php.net/manual/en/function.json-encode.php): > value > The value being encoded. Can be any type except a resource. It seems silly to expect users to add classes implementing JsonSerializable to represent, say, scalar values or an empty object. I'm not sure if you consider removed exceptional cases a BC break or not. I'm assuming "no" for now. Commits ------- e98731a [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:
It seems silly to expect users to add classes implementing JsonSerializable to represent, say, scalar values or an empty object.
I'm not sure if you consider removed exceptional cases a BC break or not. I'm assuming "no" for now.