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

[9.x] Add ability to supply HTTP client methods with JsonSerializable instances #41055

Merged
merged 2 commits into from
Feb 16, 2022
Merged

[9.x] Add ability to supply HTTP client methods with JsonSerializable instances #41055

merged 2 commits into from
Feb 16, 2022

Conversation

JaZo
Copy link
Contributor

@JaZo JaZo commented Feb 16, 2022

Problem

Guzzle supports "any PHP type that can be operated on by PHP's json_encode() function" as JSON body format, but the current implementation prevents using anything other than a string or an array (or Arrayable). This is because the logic that parses the request data for assertions assumes it's always either of those types.

Description

This PR introduces the ability – or fixes the bug, depends how you see it – to supply JsonSerializable instances into the Laravel HTTP Client for more convenient use.

Usage

Before

$user = new User(['name' => 'Taylor']); // Some class implementing JsonSerializable.

Http::withBody(json_encode($user), 'application/json')->post('/users');

After

$user = new User(['name' => 'Taylor']); // Some class implementing JsonSerializable.

Http::post('/users', $user);

This example uses a class that implements JsonSerializable, but it can be anything that json_encode can handle.

@JaZo JaZo changed the title Add ability to supply HTTP client methods with JsonSerializable instances [9.x] Add ability to supply HTTP client methods with JsonSerializable instances Feb 16, 2022
@taylorotwell
Copy link
Member

Can you explain how your code change actually works and why it fixes the problem?

@JaZo
Copy link
Contributor Author

JaZo commented Feb 16, 2022

The internal parseRequestData method is ought to return an array, but when you use anything other than a string or an array as body, it returns that instead, resulting in errors in several withData() calls on the request instance later on.

If we follow the code flow given my earlier 'after' example (Http::post('/users', new User(['name' => 'Taylor']));):

protected function parseRequestData($method, $url, array $options)
{
    $laravelData = $options[$this->bodyFormat] ?? $options['query'] ?? []; // $laravelData is the User instance

    $urlString = Str::of($url);

    if (empty($laravelData) && $method === 'GET' && $urlString->contains('?')) { // $laravelData isn't empty, so remains the User instance
        $laravelData = (string) $urlString->after('?');
    }
    if (is_string($laravelData)) { // $laravelData isn't a string, so remains the User instance
        parse_str($laravelData, $parsedData);
        $laravelData = is_array($parsedData) ? $parsedData : [];
    }

    // at this point, $laravelData still isn't an array and before my fix it was returned as if it was, resulting in the error

    if (!is_array($laravelData)) { // $laravelData isn't an array, but we should return one and we can't reliably convert it to an array, so we return an empty array instead
        $laravelData = [];
    }

    return $laravelData;
}

One could say we can call jsonSerialize on $laravelData if it implements JsonSerializable, but it doesn't necessarily return an array as it can be anything json_encode understands. Returning an empty array is ok because if it is empty, the request tries to parse the actual PSR request body itself: https://github.com/laravel/framework/blob/9.x/src/Illuminate/Http/Client/Request.php#L196. At that point Guzzle already encoded the data to JSON and we can easily decode it.

@taylorotwell
Copy link
Member

This test already passes for me so I'm not able to recreate the issue. Am I doing something wrong?

image

image

@JaZo
Copy link
Contributor Author

JaZo commented Feb 16, 2022

I think you used the default User model. Eloquent models implement the Arrayable interface so these work fine. Might not be the clearest example, sorry. I've created a minimal reproducible project where you can see the issue. It includes a class that implements JsonSerializable, but not Arrayable. See route and test.

afbeelding

@taylorotwell taylorotwell merged commit f4f8405 into laravel:9.x Feb 16, 2022
@taylorotwell
Copy link
Member

Thanks

@huangdijia
Copy link
Contributor

https://github.com/JaZo/framework/blob/2119cd3a2ce2441d4146bbb0eca87ae0d5959913/src/Illuminate/Http/Client/PendingRequest.php#L732

    protected function parseHttpOptions(array $options)
    {
        if (isset($options[$this->bodyFormat])) {
            if ($this->bodyFormat === 'multipart') {
                $options[$this->bodyFormat] = $this->parseMultipartBodyFormat($options[$this->bodyFormat]);
            } elseif ($this->bodyFormat === 'body') {
                $options[$this->bodyFormat] = $this->pendingBody;
            }

            if (is_array($options[$this->bodyFormat])) {
                $options[$this->bodyFormat] = array_merge(
                    $options[$this->bodyFormat], $this->pendingFiles
                );
            }
        } else {
            $options[$this->bodyFormat] = $this->pendingBody;
        }

        if ($laravelData instanceof JsonSerializable) {
            $laravelData = $laravelData->jsonSerialize();
        }

        return collect($options)->toArray();
    }

@taylorotwell Should add at this?

@JaZo
Copy link
Contributor Author

JaZo commented Feb 17, 2022

@huangdijia if we use jsonSerialize there, it will affect the request body. Manually calling jsonSerialize will only serialize one level deep – which is probably fine for assertions – while json_encode does this recursive. A class can have different forms for Arrayable and JsonSerializable. So if we jsonSerialize one level deep and then toArray all child items, the payload is partially serialized for JSON and partially 'arrayed', while the user would expect it's passed 1:1 to Guzzle and entirely serialized for JSON.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants