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] Allow HTTP client requests with retries to optionally throw RequestExceptions #40079

Merged
merged 3 commits into from
Dec 17, 2021
Merged

Conversation

mattkingshott
Copy link
Contributor

@mattkingshott mattkingshott commented Dec 16, 2021

Edit: Changed to master branch at suggestion of @driesvints.

This PR addresses #40055 by adding functionality to the HTTP client so that you can prevent it from throwing a RequestException when retries are configured.

The problem

Presently, and as highlighted in the docs, the HTTP client does not throw exceptions when a request fails. As a result, you can make use of methods like failed on the response e.g.

$response = Http::get('http://example.com');

if ($response->failed())
{
    // Handle the error
}

However, if you configure retries on the HTTP client, it will throw a RequestException if all attempts fail.

$response = Http::retries(3, 1000)->get('http://example.com');

This means:

  1. You cannot configure throw() on the client.
  2. You cannot chain methods e.g. throw()->json().
  3. You cannot use throwIf.
  4. You cannot use the response's error handling methods e.g. $response->failed().

In addition, you have to wrap the request within a try/catch block if you want to handle the error.

The solution

The PR adds a fourth parameter throw to the retries method, which allows you to control whether the HTTP client should throw an exception when the retry limit is reached.

By default, this parameter is set to true to preserve backward compatibility.

Http::retries(3, 1000); // throws a RequestException if all retries fail
Http::retries(3, 1000, null, true); // throws a RequestException if all retries fail
Http::retries(3, 1000, null, false); // does not throw a RequestException if all retries fail

@koenhoeijmakers
Copy link
Contributor

koenhoeijmakers commented Dec 17, 2021

I think it would be clearer if we have a separate method instead of a boolean param

@mattkingshott
Copy link
Contributor Author

I’m happy to change it if the Laravel team think it would be a better approach?

Any ideas for the method name? Maybe disableRetryExceptions()

@bryanlopezinc
Copy link

Maybe Http::catchRetriesExceptions ?

@mattkingshott
Copy link
Contributor Author

As currently implemented, it does throw exceptions. Would that method name not imply that the default is that it doesn't?

@taylorotwell taylorotwell merged commit ec7045e into laravel:master Dec 17, 2021
@taylorotwell
Copy link
Member

taylorotwell commented Dec 17, 2021

I don't think the boolean argument is that bad when combined with named parameters:

Http::retry(3, throw: false)->get(...);

@mattkingshott mattkingshott deleted the master branch December 17, 2021 16:45
@mattkingshott
Copy link
Contributor Author

Woo!! 🍿🙏🏻

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.

4 participants