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.

Sign up for GitHub

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] Add throwWith to the HTTP client response #34558

Merged
merged 2 commits into from
Sep 28, 2020
Merged

[8.x] Add throwWith to the HTTP client response #34558

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c131f47
Add throwWith to the HTTP client response
timacdonald Sep 27, 2020
571b36f
Implement `onError` instead of the combined `throwWith` method
timacdonald Sep 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions src/Illuminate/Http/Client/Response.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,8 +208,25 @@ public function toPsrResponse()
* @throws \Illuminate\Http\Client\RequestException
timacdonald marked this conversation as resolved.
Show resolved Hide resolved
*/
public function throw()
{
return $this->throwWith(function () {
//
});
}

/**
* Execute the callback and throw an exception if a server of client error occurred.
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the comment should be changed as an exception is not thrown anymore

*
* @param \Closure $callback
* @return $this
*
* @throws \Illuminate\Http\Client\RequestException
*/
public function throwWith($callback)
Copy link
Contributor

Choose a reason for hiding this comment

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

What about beforeThrowing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Then it'd make sense to call beforeThrowing()->throw(), which IMO is too bervose

Copy link
Member Author

Choose a reason for hiding this comment

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

agreed on that being to verbose. I was hoping to find something that could express "When throwing, do this thing and then throw" rather than two methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

onError also makes sense to me. throwWith doesn't make sense to me because it has an indication of which parameters would be given to throw (like the Cotnainer's makeWith). Though I still prefer beforeThrowing as it indicates what to do (callback) right before an exception is thrown. I guess whenThrowing could also work (like Query Builder when()).

Copy link
Member Author

@timacdonald timacdonald Sep 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't disagree with your objections. As mentioned I didn't know if this was really a great method name.

Maybe there just isn't a nice "all-in-one" option, as whenThrowing also makes me wonder if I need to call throw or not.

Maybe it is just an onError callback (as you and others suggested on Twitter).

return $client->withHeaders($headers)
    ->post($url, $payload)
    ->onError(fn ($response) =>
        Log::error('Twitter API failed posting Tweet', [
            'url' => $url,
            'payload' => $payload,
            'headers' => $headers,
            'response' => $response->body(),
        ])
    )->throw()->json();

Or maybe I just need to wrap up the more verbose middleware as @sebdesign pointed out in my own app 🤷‍♂️

I just liked how the error callback and throw was on the other side of the post call, as it felt nicer to me being in sequential order, but that is just an opinion that might not be shared.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I prefer the onError callback instead of the Middleware::mapResponse stuff. It feels nicer and you get to work with the Laravel\Http\Client\Response instead of the Guzzle response.

{
if ($this->serverError() || $this->clientError()) {
$callback($this);

throw new RequestException($this);
Copy link
Member

Choose a reason for hiding this comment

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

What if we do it as:

throw tap(new RequestException($this), function ($exception) use ($callback) {
   $callback($this, $exception);
});

}

Expand Down
36 changes: 36 additions & 0 deletions tests/Http/HttpClientTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,6 +476,42 @@ public function testRequestExceptionEmptyBody()
throw new RequestException(new Response($response));
}

public function testThrowWithCallsClosureOnError()
{
$status = 0;
$client = $this->factory->fake([
'laravel.com' => $this->factory::response('', 401),
]);
$response = $client->get('laravel.com');

try {
$response->throwWith(function ($response) use (&$status) {
$status = $response->status();
});
} catch (RequestException $e) {
//
}

$this->assertSame(401, $status);
$this->assertSame(401, $response->status());
}

public function testThrowWithDoesntCallClosureOnSuccess()
{
$status = 0;
$client = $this->factory->fake([
'laravel.com' => $this->factory::response('', 201),
]);
$response = $client->get('laravel.com');

$response->throwWith(function ($response) use (&$status) {
$status = $response->status();
});

$this->assertSame(0, $status);
$this->assertSame(201, $response->status());
}

public function testSinkToFile()
{
$this->factory->fakeSequence()->push('abc123');
Expand Down