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

timacdonald · 2020-09-27T02:57:08Z

Implementation updated: #34558 (comment)

Because:

There is a repeated pattern where you want to perform a particular action if the response has failed, such as logging an error, but still have the exception throw afterwards.

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.

crynobone · 2020-09-27T07:12:11Z

src/Illuminate/Http/Client/Response.php

    {
        if ($this->serverError() || $this->clientError()) {
+            $callback($this);
+
            throw new RequestException($this);


What if we do it as:

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

deleugpn · 2020-09-27T10:16:01Z

src/Illuminate/Http/Client/Response.php

+     *
+     * @throw \Illuminate\Http\Client\RequestException
+     */
+    public function throwWith($callback)


What about beforeThrowing?

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

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.

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()).

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.

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.

src/Illuminate/Http/Client/Response.php

sebdesign · 2020-09-27T19:24:55Z

I think the same can be achieved by using middleware.

Example:

return $client->withHeaders($headers)
    ->withMiddleware(Middleware::mapResponse(function ($response) {
        if ($response->getStatusCode() >= 400) {
            Log::error('Twitter API failed posting Tweet', [
                'url' => $url,
                'payload' => $payload,
                'headers' => $headers,
                'response' => (string) $response->getBody(),
            ]);
        }

        return $response;
    }))->post($url, $payload)->throw()->json();

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.

timacdonald · 2020-09-28T10:05:19Z

Although it isn't as nice as I had hoped, I've moved to an onError method that only deals with calling the callback and doesn't also throw the exception. Although we had some back and forth (#34558 (comment)) I don't think we got an awesome option, so here we are.

New implementation example:

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)
    ->onError(fn ($response) =>
        Log::error('Twitter API failed posting Tweet', [
            'url' => $url,
            'payload' => $payload,
            'headers' => $headers,
            'response' => $response->body(),
        ])
    )->throw()->json();

As @sebdesign pointed out, using the method instead of the suggested middleware approach above you get to use the Illuminate response instead of the Guzzle response in the callback. See: #34558 (comment)

I also think it feels nicer as it is expressed in sequential order.

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

rodrigopedra · 2020-09-28T13:16:35Z

src/Illuminate/Http/Client/Response.php

@@ -216,6 +216,21 @@ public function throw()
        return $this;
    }

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


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

deleugpn · 2020-09-28T13:24:00Z

I think there's one last option for this that technically has to be split into 2 PRs:

PR 1 (8.x)

    public function throw()
    {
        $callback = func_get_arg(0);
            
        if ($this->serverError() || $this->clientError()) {
            if ($callback && is_callable($callback)) {
                $callback($this);
            }
            
            throw new RequestException($this);
        }

        return $this;
    }

PR 2 (9.x)

    public function throw(?callable $callable = null)
    {
        if ($this->serverError() || $this->clientError()) {
            if ($callback) {
                $callback($this);
            }
            
            throw new RequestException($this);
        }

        return $this;
    }

Usage:

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

m1guelpf · 2020-09-28T13:30:17Z

I like that, but I fear the name throw might lead people into believing you should throw the exception there instead of the callback running before the exception gets thrown.

deleugpn · 2020-09-28T13:32:47Z

@m1guelpf I don't see that as a problem, but rather as a feature.

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

        throw new MyCustomExceptionThatWillTakePrecedenceOverTheNativeException();
    })->json();

taylorotwell · 2020-09-28T17:36:41Z

So I kept the onError method and also added support for passing a Closure to throw per @deleugpn suggestion. I kept both because I figured sometimes you may want to onError but not throw anything.

timacdonald · 2020-09-28T20:35:00Z

Nice! Thanks for the collab everyone. Really dig where this landed with everyone’s input. Best of all worlds! ❤️

timacdonald changed the title ~~Add throwWith to the HTTP client response~~ [8.x] Add throwWith to the HTTP client response Sep 27, 2020

crynobone reviewed Sep 27, 2020

View reviewed changes

deleugpn reviewed Sep 27, 2020

View reviewed changes

GrahamCampbell marked this pull request as draft September 27, 2020 11:31

GrahamCampbell requested changes Sep 27, 2020

View reviewed changes

src/Illuminate/Http/Client/Response.php Outdated Show resolved Hide resolved

timacdonald marked this pull request as ready for review September 28, 2020 10:10

rodrigopedra reviewed Sep 28, 2020

View reviewed changes

taylorotwell merged commit 571b36f into laravel:8.x Sep 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

timacdonald commented Sep 27, 2020 •

edited

Loading

crynobone Sep 27, 2020

deleugpn Sep 27, 2020

m1guelpf Sep 27, 2020

timacdonald Sep 28, 2020

deleugpn Sep 28, 2020

timacdonald Sep 28, 2020 •

edited

Loading

sebdesign Sep 28, 2020

sebdesign commented Sep 27, 2020 •

edited

Loading

timacdonald commented Sep 28, 2020 •

edited

Loading

rodrigopedra Sep 28, 2020

deleugpn commented Sep 28, 2020 •

edited

Loading

m1guelpf commented Sep 28, 2020

deleugpn commented Sep 28, 2020

taylorotwell commented Sep 28, 2020

timacdonald commented Sep 28, 2020 •

edited

Loading

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

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

Conversation

timacdonald commented Sep 27, 2020 • edited Loading

crynobone Sep 27, 2020

Choose a reason for hiding this comment

deleugpn Sep 27, 2020

Choose a reason for hiding this comment

m1guelpf Sep 27, 2020

Choose a reason for hiding this comment

timacdonald Sep 28, 2020

Choose a reason for hiding this comment

deleugpn Sep 28, 2020

Choose a reason for hiding this comment

timacdonald Sep 28, 2020 • edited Loading

Choose a reason for hiding this comment

sebdesign Sep 28, 2020

Choose a reason for hiding this comment

sebdesign commented Sep 27, 2020 • edited Loading

timacdonald commented Sep 28, 2020 • edited Loading

rodrigopedra Sep 28, 2020

Choose a reason for hiding this comment

deleugpn commented Sep 28, 2020 • edited Loading

m1guelpf commented Sep 28, 2020

deleugpn commented Sep 28, 2020

taylorotwell commented Sep 28, 2020

timacdonald commented Sep 28, 2020 • edited Loading

timacdonald commented Sep 27, 2020 •

edited

Loading

timacdonald Sep 28, 2020 •

edited

Loading

sebdesign commented Sep 27, 2020 •

edited

Loading

timacdonald commented Sep 28, 2020 •

edited

Loading

deleugpn commented Sep 28, 2020 •

edited

Loading

timacdonald commented Sep 28, 2020 •

edited

Loading