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

Update HttpDriver.php #185

Closed
wants to merge 1 commit into from
Closed

Conversation

adrianopedro
Copy link

The throw() method on $this->http()->throw() on method process throws is not valid as it is not a method of http anymore. Using laravel 8.12 it works by simply removing the throw() call.

The throw() method on $this->http()->throw() on method process throws is not valid as it is not a method of http anymore.
Using laravel 8.12 it works by simply removing the throw() call.
@jeanlucnguyen
Copy link

The "->connectTimeout(3)" method in
return value($callback, Http::timeout(3)->connectTimeout(3));

does not exist yet in Laravel 8 as well
https://github.com/laravel/framework/blob/8.x/src/Illuminate/Support/Facades/Http.php

@stevebauman
Copy link
Owner

Thanks guys -- apologies on the wait for this. This PR won't work as intended, as false won't properly be returned if the request fails (5XX/4XX status code). I'll patch this and close this PR once complete.

@stevebauman
Copy link
Owner

stevebauman commented Jan 17, 2025

@jeanlucnguyen The connectTimeout method isn't used in this library. A config array is used instead:

$callback = static::$httpResolver ?: fn ($http) => $http;
return value($callback, Http::withOptions(
config('location.http', [
'timeout' => 3,
'connect_timeout' => 3,
])
));

Where did you find that?

@stevebauman
Copy link
Owner

I've created a new release resolving this (v7.4.1), thanks again for the report!

@jeanlucnguyen
Copy link

@stevebauman Thanks for the quick release !

Right, about connectTimeout, I was doing my tests on v7.2 but just noticed you already removed it on the upgrade from v7.2.2 and v7.3.0

@stevebauman
Copy link
Owner

@jeanlucnguyen ahh ok understood!

Happy to help! 🙏

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