Skip to content

[HttpClient] Handle requests with null body #45566

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

Jump to bottom
Merged
merged 1 commit into from
Feb 27, 2022
Merged

[HttpClient] Handle requests with null body #45566

merged 1 commit into from
Feb 27, 2022

Conversation

jderusse
Copy link
Member

@jderusse jderusse commented Feb 26, 2022 •
edited
Loading

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets /
License MIT
Doc PR /

since #45527 passing null to the body parameters leads to an exception (which breaks async-aws)

Argument #2 ($body) must be of type Closure, null given, called in /home/runner/work/aws/aws/vendor/symfony/http-client/CurlHttpClient.php on line 221

In curl client: null is not a string and self::readRequestBody expects a closure.

symfony/src/Symfony/Component/HttpClient/CurlHttpClient.php

Lines 214 to 221 in 08fa74a

if (!\is_string($body = $options['body'])) {
if (\is_resource($body)) {
$curlopts[\CURLOPT_INFILE] = $body;
} else {
$eof = false;
$buffer = '';
$curlopts[\CURLOPT_READFUNCTION] = static function ($ch, $fd, $length) use ($body, &$buffer, &$eof) {
return self::readRequestBody($length, $body, $buffer, $eof);

In NativeClient, getBodyAsString will fail to return null because of the string return type.

Before #45527 null was converted to "" thanks to the defaultOptions, but this is not the case anymore.

In many places, we check if the body is !== "" but rarely check if the body is null, this PR restores the original behaviors for the body parameters and converts nulls to "".

@carsonbot carsonbot added this to the 4.4 milestone Feb 26, 2022
@carsonbot carsonbot changed the title Handle requests with null body [HttpClient] Handle requests with null body Feb 26, 2022
@jderusse jderusse mentioned this pull request Feb 26, 2022
Merged
@jderusse
Copy link
Member Author

note: The signature of the HttpClientInterface::request method says

'body' => '' // array|string|resource|\Traversable|\Closure - description ...

null is not a valid type and the application should not have sent body => null.

But from another hand, the same docblock says

'auth_basic' => null, // array|string - description ...

One could think that null is not a valid type, but could be interpreted as "I don't want to set the option"

So I believe #45527 is a BC break for body (and maybe other options)

@nicolas-grekas
Copy link
Member

So I believe #45527 is a BC break for body (and maybe other options)

You are right. I'm annoyed for other options. I'm wondering if we shouldn't revert #45527 and/or review which options need null to be overriding. Can you have a look? I will also as soon as I'm not AFK.

@nicolas-grekas
Copy link
Member

Here is what I'd like to try:
On the first call to prepareRequest, store the defaults in a new property named eg $emptyDefaults.
When merging options, if the corresponding key in $emptyDefaults is null, allow null to override the defaults. Otherwise no (and keep the previous behavior). That should fix body and all affected options.
Could you please give it a try?

@jderusse
Copy link
Member Author

Here is what I'd like to try: On the first call to prepareRequest, store the defaults in a new property named eg $emptyDefaults. When merging options, if the corresponding key in $emptyDefaults is null, allow null to override the defaults. Otherwise no (and keep the previous behavior). That should fix body and all affected options. Could you please give it a try?

I'm not sure to understand: The first call to prepareRequest might contain proxy=FOO or body="" in the first case (proxy=FOO) we want null to reset proxy. In the second case (body="") we want null to use the default "".

I believe the right solution is to have an internal technical default (not paramterizable) that fallbacks nulls to a specific value:

  • body => ""
  • proxy => null
  • max_duration => INF ...
    OR making our implementations resilient to null options (adding if (isset($option['...'])) everywhere).

@nicolas-grekas
Copy link
Member

The first call to prepareRequest might contain proxy=FOO

Actually not, because the first call always happens in the constructor. This call defines what you are looking for, this technical description of which values allow null.

@jderusse
Copy link
Member Author

The first call to prepareRequest might contain proxy=FOO

Actually not, because the first call always happens in the constructor. This call defines what you are looking for, this technical description of which values allow null.

go it.. PR updated.
Actually, the method is not always called only when the defaultOption is provided (But the result is the same). And the MockClient does not have default options

nicolas-grekas
nicolas-grekas reviewed Feb 27, 2022
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Looks like the init should be moved to mergeDefaultOptions, see failures.
👍 Otherwise. Maybe just rename $key to $k?

@nicolas-grekas nicolas-grekas force-pushed the fix-null-body branch 2 times, most recently from 231621b to 557ce67 Compare February 27, 2022 08:21
@nicolas-grekas
Copy link
Member

Thank you @jderusse.

@nicolas-grekas nicolas-grekas merged commit b8bf937 into symfony:4.4 Feb 27, 2022
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Feb 27, 2022 •
edited
Loading

I changed the approach a bit in 65609d8 FYI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

Assignees
No one assigned
Labels
Bug HttpClient Status: Needs Review
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
3 participants
@jderusse @nicolas-grekas @carsonbot