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

Set response field on request failure #16

Merged
merged 2 commits into from
Sep 4, 2019
Merged

Set response field on request failure #16

merged 2 commits into from
Sep 4, 2019

Conversation

dmason30
Copy link
Contributor

Fixes spatie/laravel-webhook-client#19

(Sorry I created the original issue on the wrong repository 🤣 ).

The new response properties in PR #14 should also be added to the new section added here.

@freekmurze
Copy link
Member

This seems good, but I don't like the fact that we expose a property to show the response. Could you make it private and add a method getResponse.

Also add a section to the readme explaining how to get the response.

And after that, it'll be perfect 👍

@dmason30
Copy link
Contributor Author

dmason30 commented Sep 2, 2019

I am happy to make those changes but just check with you changing the access for the response property it would be a breaking change?

I can see currently in the readme the public property is currently mentioned so chances are it is being used by a decent number of people (including me 😄)

response: the response returned by the remote app. Can be an instance of \GuzzleHttp\Psr7\Response or null.

Once confirmed it is ok I will get this done as soon as I can.

@judgej
Copy link

judgej commented Sep 2, 2019

If it's PSR-7, should the response not be \Psr\Http\Message\ResponseInterface rather than \GuzzleHttp\Psr7\Response? It's a pedantic point, but the whole idea of the PSRs is that they offer interfaces that can have any number of implementations from different vendors, and people should be hinting against the interfaces and not the implementations.

@freekmurze freekmurze merged commit 7974547 into spatie:master Sep 4, 2019
@freekmurze
Copy link
Member

Thanks!

@dmason30 dmason30 deleted the response-on-failure branch September 4, 2019 23:26
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.

Set response on webhook request error?
3 participants