[HttpClient] Handle requests with null body by jderusse · Pull Request #45566 · symfony/symfony

jderusse · 2022-02-26T18:52:03Z

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 "".

jderusse · 2022-02-26T19:06:12Z

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 · 2022-02-26T21:14:34Z

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 · 2022-02-26T21:45:49Z

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 · 2022-02-26T22:06:15Z

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 · 2022-02-26T22:25:58Z

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 · 2022-02-26T23:01:11Z

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

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

nicolas-grekas · 2022-02-27T08:27:13Z

Thank you @jderusse.

nicolas-grekas · 2022-02-27T09:43:29Z

I changed the approach a bit in 65609d8 FYI

jderusse requested a review from nicolas-grekas February 26, 2022 18:52

carsonbot added Status: Needs Review Bug labels Feb 26, 2022

carsonbot added this to the 4.4 milestone Feb 26, 2022

jderusse added the HttpClient label Feb 26, 2022

carsonbot changed the title ~~Handle requests with null body~~ [HttpClient] Handle requests with null body Feb 26, 2022

jderusse mentioned this pull request Feb 26, 2022

Do not send request with null body async-aws/aws#1191

Merged

jderusse force-pushed the fix-null-body branch from ae432f4 to 6561f9e Compare February 26, 2022 22:59

nicolas-grekas reviewed Feb 27, 2022

View reviewed changes

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

[HttpClient] Handle requests with null body

39aec09

nicolas-grekas force-pushed the fix-null-body branch from 557ce67 to 39aec09 Compare February 27, 2022 08:26

nicolas-grekas merged commit b8bf937 into symfony:4.4 Feb 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[HttpClient] Handle requests with null body#45566

[HttpClient] Handle requests with null body#45566
nicolas-grekas merged 1 commit intosymfony:4.4from
jderusse:fix-null-body

jderusse commented Feb 26, 2022 •

edited

Loading

Uh oh!

jderusse commented Feb 26, 2022

Uh oh!

nicolas-grekas commented Feb 26, 2022

Uh oh!

nicolas-grekas commented Feb 26, 2022

Uh oh!

jderusse commented Feb 26, 2022

Uh oh!

nicolas-grekas commented Feb 26, 2022

Uh oh!

jderusse commented Feb 26, 2022

Uh oh!

nicolas-grekas left a comment

Uh oh!

nicolas-grekas commented Feb 27, 2022

Uh oh!

nicolas-grekas commented Feb 27, 2022 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	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);

Uh oh!

Conversation

jderusse commented Feb 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jderusse commented Feb 26, 2022

Uh oh!

nicolas-grekas commented Feb 26, 2022

Uh oh!

nicolas-grekas commented Feb 26, 2022

Uh oh!

jderusse commented Feb 26, 2022

Uh oh!

nicolas-grekas commented Feb 26, 2022

Uh oh!

jderusse commented Feb 26, 2022

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Feb 27, 2022

Uh oh!

nicolas-grekas commented Feb 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jderusse commented Feb 26, 2022 •

edited

Loading

nicolas-grekas commented Feb 27, 2022 •

edited

Loading