From c131f471af6e3a45593adbe0ed3bbea074f85500 Mon Sep 17 00:00:00 2001 From: Tim MacDonald Date: Sun, 27 Sep 2020 12:33:36 +1000 Subject: [PATCH 1/2] Add throwWith to the HTTP client response Because: - There is a repeated pattern where you want to perform a particular action if the response has failed, such as logging the error. ```php // before... $response = $client->withHeaders($headers)->post($url, $payload); if ($response->failed()) { Log::error('Twitter API failed posting Tweet', [ 'url' => $url, 'payload' => $payload, 'headers' => $headers, 'response' => $response->body(), ]); $response->throw(); } return $response->json(); // after... return $client->withHeaders($headers) ->post($url, $payload) ->throwWith(fn ($response) => Log::error('Twitter API failed posting Tweet', [ 'url' => $url, 'payload' => $payload, 'headers' => $headers, 'response' => $response->body(), ]) )->json(); ``` This commit: - Adds the `throwWith` method to the response. - Updates the `throw` method to utilise the new `throwWith` method, just with an empty closure. Notes: - I'm not convinced this is the best name for this method. - I'm also wondering if the method should be a `tap` method of some kind. `tapWhenThrowing` or something along those lines. --- src/Illuminate/Http/Client/Response.php | 17 ++++++++++++ tests/Http/HttpClientTest.php | 36 +++++++++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/src/Illuminate/Http/Client/Response.php b/src/Illuminate/Http/Client/Response.php index 2a34b5f83fef..a66575e89269 100644 --- a/src/Illuminate/Http/Client/Response.php +++ b/src/Illuminate/Http/Client/Response.php @@ -208,8 +208,25 @@ public function toPsrResponse() * @throws \Illuminate\Http\Client\RequestException */ public function throw() + { + return $this->throwWith(function () { + // + }); + } + + /** + * Execute the callback and throw an exception if a server of client error occurred. + * + * @param \Closure $callback + * @return $this + * + * @throws \Illuminate\Http\Client\RequestException + */ + public function throwWith($callback) { if ($this->serverError() || $this->clientError()) { + $callback($this); + throw new RequestException($this); } diff --git a/tests/Http/HttpClientTest.php b/tests/Http/HttpClientTest.php index 45aae46c950e..739657a7d79c 100644 --- a/tests/Http/HttpClientTest.php +++ b/tests/Http/HttpClientTest.php @@ -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'); From 571b36fdbd3087ec5facf134b46e42cc736c984e Mon Sep 17 00:00:00 2001 From: Tim MacDonald Date: Mon, 28 Sep 2020 19:59:15 +1000 Subject: [PATCH 2/2] Implement `onError` instead of the combined `throwWith` method Because: - There wasn't a great name that really expressed what was happening under the hood, i.e. running the closure and throwing the exception This commit: - Reverts the `throw` method and implements a stand-alone `onError` method --- src/Illuminate/Http/Client/Response.php | 14 ++--- tests/Http/HttpClientTest.php | 74 ++++++++++++++++++++----- 2 files changed, 65 insertions(+), 23 deletions(-) diff --git a/src/Illuminate/Http/Client/Response.php b/src/Illuminate/Http/Client/Response.php index a66575e89269..f93dc6c753b2 100644 --- a/src/Illuminate/Http/Client/Response.php +++ b/src/Illuminate/Http/Client/Response.php @@ -209,9 +209,11 @@ public function toPsrResponse() */ public function throw() { - return $this->throwWith(function () { - // - }); + if ($this->serverError() || $this->clientError()) { + throw new RequestException($this); + } + + return $this; } /** @@ -219,15 +221,11 @@ public function throw() * * @param \Closure $callback * @return $this - * - * @throws \Illuminate\Http\Client\RequestException */ - public function throwWith($callback) + public function onError($callback) { if ($this->serverError() || $this->clientError()) { $callback($this); - - throw new RequestException($this); } return $this; diff --git a/tests/Http/HttpClientTest.php b/tests/Http/HttpClientTest.php index 739657a7d79c..6a599b2fdb6d 100644 --- a/tests/Http/HttpClientTest.php +++ b/tests/Http/HttpClientTest.php @@ -476,42 +476,86 @@ public function testRequestExceptionEmptyBody() throw new RequestException(new Response($response)); } - public function testThrowWithCallsClosureOnError() + public function testOnErrorDoesntCallClosureOnInformational() { $status = 0; $client = $this->factory->fake([ - 'laravel.com' => $this->factory::response('', 401), + 'laravel.com' => $this->factory::response('', 101), ]); - $response = $client->get('laravel.com'); - try { - $response->throwWith(function ($response) use (&$status) { + $response = $client->get('laravel.com') + ->onError(function ($response) use (&$status) { $status = $response->status(); }); - } catch (RequestException $e) { - // - } - $this->assertSame(401, $status); - $this->assertSame(401, $response->status()); + $this->assertSame(0, $status); + $this->assertSame(101, $response->status()); } - public function testThrowWithDoesntCallClosureOnSuccess() + public function testOnErrorDoesntCallClosureOnSuccess() { $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(); - }); + $response = $client->get('laravel.com') + ->onError(function ($response) use (&$status) { + $status = $response->status(); + }); $this->assertSame(0, $status); $this->assertSame(201, $response->status()); } + public function testOnErrorDoesntCallClosureOnRedirection() + { + $status = 0; + $client = $this->factory->fake([ + 'laravel.com' => $this->factory::response('', 301), + ]); + + $response = $client->get('laravel.com') + ->onError(function ($response) use (&$status) { + $status = $response->status(); + }); + + $this->assertSame(0, $status); + $this->assertSame(301, $response->status()); + } + + public function testOnErrorCallsClosureOnClientError() + { + $status = 0; + $client = $this->factory->fake([ + 'laravel.com' => $this->factory::response('', 401), + ]); + + $response = $client->get('laravel.com') + ->onError(function ($response) use (&$status) { + $status = $response->status(); + }); + + $this->assertSame(401, $status); + $this->assertSame(401, $response->status()); + } + + public function testOnErrorCallsClosureOnServerError() + { + $status = 0; + $client = $this->factory->fake([ + 'laravel.com' => $this->factory::response('', 501), + ]); + + $response = $client->get('laravel.com') + ->onError(function ($response) use (&$status) { + $status = $response->status(); + }); + + $this->assertSame(501, $status); + $this->assertSame(501, $response->status()); + } + public function testSinkToFile() { $this->factory->fakeSequence()->push('abc123');