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

[8.x] Fix replacing request options #40954

Merged
merged 4 commits into from
Feb 11, 2022
Merged

[8.x] Fix replacing request options #40954

merged 4 commits into from
Feb 11, 2022

Conversation

driesvints
Copy link
Member

@driesvints driesvints commented Feb 11, 2022

This is an attempt at fixing #40952. Right now, when using array_merge_recursive, options which already exist will be merged into a new array. See the behavior here: https://3v4l.org/joqh9

It seems to be this is always unwanted as you'd simply want to replace the previous option that was present. I've added a test for the new behavior. As you can see it verifies the originally set option of http_errors stays set properly and doesn't gets converted to an array.

This issue was uncovered in Laravel 9 where we now set the default connect_timeout. Apps that were overriding this with withOptions saw their apps and integrations break because connect_timeout was suddenly an array instead of an integer.

I sent this in to 8.x because Laravel 8 also suffers from this. I have to admit that I don't know of any situation where the behavior is wanted but that couldn't mean it's not used. Therefor I'm not 100% if this will break anything. But the current behavior definitely isn't the correct one either.

After merging this this will need to be merged into 9.x as well by merging the 8.x branch into the 9.x branch.

Fixes #40952

@taylorotwell taylorotwell merged commit d037fc5 into 8.x Feb 11, 2022
@taylorotwell taylorotwell deleted the http-client-replace branch February 11, 2022 15:07
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.

2 participants