-
-
Notifications
You must be signed in to change notification settings - Fork 149
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
Webhook response customisation #56
Webhook response customisation #56
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this quality PR. I very much like this clean implementation 👍
I only got one nitpick. What do you think about adding a Response
return type?
Thank you! |
@freekmurze Should this not have been tagged as a new major version? 🤔It's not an issue for me, but maybe for others that update their composer packages without knowing about this new, required config-field? |
The config in your app will be merged with the one in the package
So this shouldn't be a breaking change for people that already have the package installed. |
I just tested this, and it does not seem to merge the config, probably because the config-file is a list of arrays (I think) |
Does this fix it for you? d21404f (thanks for bringing this problem to my attention) |
No, it's fixed if I do the following: use Spatie\WebhookClient\WebhookResponse\DefaultRespondsTo;
$webhookResponseClass = $properties['webhook_response'] ?? DefaultRespondsTo::class;
if (! is_subclass_of($webhookResponseClass, RespondsToWebhook::class)) {
throw InvalidConfig::invalidWebhookResponse($webhookResponseClass);
}
$this->webhookResponse = app($webhookResponseClass); |
Could you send a quick PR for that? |
Yes 👍 |
This PR is inspired by the PR #44.
It allows you to customize the webhook response. This makes it possible to use this package in situations where a special webhook response is needed.