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

[10.x] Fix Stringable objects not converted to string in HTTP facade Query parameters and Body #48849

Merged
merged 6 commits into from
Oct 30, 2023

Conversation

LasseRafn
Copy link
Contributor

@LasseRafn LasseRafn commented Oct 28, 2023

Seems that in the HTTP Facade, query parameters, json and form bodies cannot process Stringable values, which means they must be cast to string before using.

Here is a failing sample code to use:

\Illuminate\Support\Facades\Http::get( 'https://jsonplaceholder.typicode.com/posts', [
        'id' => \Illuminate\Support\Str::of('99'),
] )->json();

The final url is: https://jsonplaceholder.typicode.com/posts

Notice how the payload contains a lot of data, and including results with a ID differently than 99.

Here is a 'passing' sample code to use:

\Illuminate\Support\Facades\Http::get( 'https://jsonplaceholder.typicode.com/posts', [
        'id' => '99',
] )->json();

The final url is: https://jsonplaceholder.typicode.com/posts?id=99

Notice how there is only one result, which is ID = 99.

Essentially for query parameters and body, Stringable is not handled and is silently ignored, entirely not sending that parameter at all.

I am not fully versed in the PendingRequest logic, and I apologise if this is the wrong approach to handling this. my PR can be treated as a WIP solution / showcase of the issue, that can spawn another PR to solve it more elegantly if the current solution isn't ideal!


Additionally, if a Number class similar to Stringable is added (#48845) I suggest we also ensure that it is supported here 👍🏻. Right now I see that it is a helper class, so not relevant, yet?.. just thought to point it out!

@LasseRafn LasseRafn marked this pull request as draft October 28, 2023 18:21
@LasseRafn LasseRafn changed the title [10.x] BUG: Stringable objects not converted to string in HTTP facade Query parameters [10.x] Fix Stringable objects not converted to string in HTTP facade Query parameters Oct 28, 2023
@LasseRafn LasseRafn marked this pull request as ready for review October 28, 2023 19:05
@LasseRafn LasseRafn changed the title [10.x] Fix Stringable objects not converted to string in HTTP facade Query parameters [10.x] Fix Stringable objects not converted to string in HTTP facade Query parameters and Body Oct 28, 2023
Comment on lines 1039 to 1054
$mergedOptions = $this->mergeOptions([
'laravel_data' => $laravelData,
'on_stats' => $onStats,
], $options));
], $options);

$mergedOptions = array_map(function($mergedOption) {
if ( is_array( $mergedOption ) ) {
array_walk_recursive($mergedOption, function (&$value) {
$value = $value instanceof Stringable ? $value->toString() : $value;
});
}

return $mergedOption instanceof Stringable ? $mergedOption->toString() : $mergedOption;
}, $mergedOptions);

return $this->buildClient()->$clientMethod($method, $url, $mergedOptions);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a gigantic fan of this implementation.

Super happy for someone to come up with a more elegant approach!

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a build in stringable interface. But then you could just cast anything coming true to string instead of using the method. One caveat would be the boolean bug though.

@taylorotwell taylorotwell merged commit 0294be7 into laravel:10.x Oct 30, 2023
19 of 20 checks passed
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.

None yet

3 participants