Use events/listeners with webhooks

[felorhik]

This is just an idea based on my own use case, but could likely be implemented without breaking changes and add more customization to cashier in general. Just an idea though, but felt like sharing.

My Use Case

First, my use case. The app I am working on uses stripe connect, so it has a full webhook system implemented. Subscriptions is just an add on, so I didn't want to use the regular cashier webhook so I disabled routes and recreated the functionality cashier was doing through the methods I already had in place.

How I handled webhooks was a central system for all webhooks from all services (SNS/Stripe), it just receives it, parses what event it is and then triggers a custom laravel event for it, which I then listen for.

The benefit here is that you could queue the events, which would make sure that if lets say the app is down for database reasons stripe does not constantly retry sending the events and eventually just turning off the webhook. Since as soon as the event is received it is sent into the queue many possible errors are bypassed and easier to track.

Proposed change here

So the proposed change is instead of handling everything inside the cashier webhook controller, trigger an event and listen to that event to run the code. This would likely add an extra function Cashier::disableEvents to not listen to the events if you wanted to do something custom, but also opens the door to extending the event to force it to queue, which would allow cashier to be updated without worrying that something has changed in the event. You could even make your own webhooks and just trigger the cashier events manually.

[felorhik]

I realized after posting this is similar to #687 just rather then jobs, events.

I think events bring more power and make less assumptions on the app, and events can be run as jobs.

[driesvints]

Heya, thanks for submitting this.

This seems like a feature request or an improvement. It's best to post these at the laravel/ideas repository to get support for your idea. After that you may send a PR to the framework. Please only use this issue tracker to report bugs and issues with the framework.

Thanks!

[felorhik]

@driesvints But this is a package for the framework, not the framework itself, I just tried to follow https://github.com/laravel/cashier/blob/10.0/contributing.md when posting this, so adding that to the contributing guidelines would be good to clear up the confusion there.

Anyway having this linked with your other issue should serve the purpose I intended.

[driesvints]

@jordanramstad oh man I'm sorry 🤦‍♂

I haven't had time to look at this in detail but queued jobs are definitely a preference after watching Lorna's talk. But could be done in combination with events.

[felorhik]

I guess from a mindset perspective, a Job is "I need to execute this task" and an event is "This just happened". A webhook is an indication of something happening, an event from an external service. So passing it to the internal event handler would make sense in my mind.

Anyway Lorna discussed it as just specifically queuing, and splitting event types into different queued items, which the event system would more closely support. Though I will admit, for every event you need to create an event and at least one listener, though it gives more overall control in the long run (since people using cashier could just add their own listener as well, if they want to do anything additional).

[driesvints]

I guess from a mindset perspective, a Job is "I need to execute this task" and an event is "This just happened". A webhook is an indication of something happening, an event from an external service. So passing it to the internal event handler would make sense in my mind.

So what needs to happen is that a webhook needs to be notified immediately if a payload was delivered. If you have blocking processing during the timeframe when an okay response gets sent then that can be a problem for the webhook. A webhook doesn't care about a failing process it only cares about delivering a payload. Queuing the process makes the response cycle much faster.

[felorhik]

Exactly, event listeners can be queued as well, but I see what you mean, a job unless on sync is always none-blocking, but the event is more dependent on the way the app is setup to be none-blocking.

By that same logic an event also does not care, it just wants to notify its listeners.

I guess this goes into an interesting thought. Forcing all event listeners to queue if the event is setup as a queued event, but that gets into laravel/ideas territory (I also think I did that before, but just could not find any documentation on it).

I still think events make more sense logically, but I do see it could bring more complexity when you have to juggle queuing every listener to follow a none-blocking pattern.

Maybe both could be done like you said? The core webhook is handled by a job, but it also triggers an event that can be hooked into if people want to do anything additional, that event could be triggered by the job to enforce a none-blocking style. This would also allow any listeners to not technically need to queue, just runs the risk if the job fails / retries then it will try to run all the listeners again.

[driesvints]

Yeah I think we can do both.

[netpok]

Note that currently if an error happens (like a database concurrency exception) and the the webhook fails with an error code (or it sends a HTTP error code on purpose) Stripe will retry the webhook and leaves the developer time to fix the problem. If the webhook handling is done by a queued job this cannot be done since it's asynchronous.

For example, we want to add an extra charge to the invoice based on some metric (probably there is an other way to do this but that's not the point) so on invoice created webhook we add the charge.

If for example the db is unavailable because it's crashed the current implementation replies with 500 server error, and Stripe will hold off the invoicing for 24 hours while retrying the webhook.

If it's handled by an async job, this action probably can't be done even without an error, because the webhook responds with a 200 webhook accepted and when the job is executed the invoice is already finalized.

[felorhik]

@netpok That is a good point, though the queue system also implements a similar system for failed jobs, so it just moves the focus.

The issue with stripes is that if you don't fix the problem fast enough, it disables the webhook altogether. It can be avoided but you must check through stripe and although it sends emails to warn you, if you miss it then eventually, no webhooks will be sent.

For finalizing invoices, if you need to add additional items, you could also have the webhook event tell stripe to finalize the invoice on completion (stripe allows you to control the stages of the invoice manually). So its more of a different thought process.

In that respect as well though, a custom queue could be used, that leverages its own connection settings. I have done that before to have a sync queue and a database queue at the same time locally (to allow 1 queue to be shared between 2 vm's using a sqlite database). A sync queue could still be used if the application requires that its not asynchronous and in that case if the job fails, an error would still be returned, leading to a 500 error.

[netpok]

I think your original approach with events is a better solution because a listener can be easily defined as queable or not.

It also makes extending the webhooks easier, because currently you can only extend the StripeWebhookController but with the event you just need to subscribe to the StripeWebhookReceived event.

[driesvints]

If for example the db is unavailable because it's crashed the current implementation replies with 500 server error, and Stripe will hold off the invoicing for 24 hours while retrying the webhook.

That's the point I'm trying to make above. Webhooks shouldn't care about 500 server errors. They only care about delivering the data. Webhooks should only try again when the data was failed to deliver, not when an app error occurred. If you experience an app error after the webhook was successfully delivered then you'll need to mitigate the error afterwards yourself.

[mateo2181]

This package can be useful: https://github.com/spatie/laravel-stripe-webhooks

[netpok]

I would say a webhook cannot be considered delivered until it was successfully handled, depending on a situation this could mean a specific action like adding a new line or storing it for later processing in the queue or the database.

Like if I receive an insured mail the postman can't just throw into the mailbox and go away, he needs a signature from me so he needs to try again if I'm not home (or leave a notification).

[driesvints]

@netpok https://sendgrid.com/blog/whats-webhook/

Webhooks deliver data to your application and may stop paying attention after making a request. This means if your application has an error your data may be lost. Many webhooks will pay attention to responses and re-send requests if your application errors out. If your application processed the request and still sent an error, there may be duplicate data in your app.

So in a nutshell: a webhook should only be concerned about delivering the data. You app needs to send a 200 OK as soon as it has received the data properly so the webhook knows the data was delivered. If you app handles webhooks synchronously then it may do some data processing, error out and the webhook might think the data wasn't delivered properly so it tries again and the same data processing might happen again which could be unwanted. This is an important gotcha when working with webhooks. I only discovered this myself about half a year ago which prompted me to open this issue: #687

[netpok]

That's true but Stripe also doesn't guarantee that a webhook will be only sent once so if that's a problem it should be handled. So while I think we should offload things for quicker response we should also keep the possibility to handle some webhooks synchronously.

Also webhooks are about events so it makes sense for me to handle them as events. It also makes extending easier and most listener can just simply implement ShouldQueue, so its like dispatching many small jobs instead of one big.

[driesvints]

With #810 two new events WebhookReceived and WebhookHandled will be available. Thanks @mitulgolakiya!