[Webhook] Allow request parsers to return multiple RemoteEvent's

[kbond]

Q	A
Branch?	7.2
Bug fix?	no
New feature?	yes
Deprecations?	no
Issues	Unblocks #50324
License	MIT

I've run into services that send their webhook events in bulk - a single request containing multiple events.

This is one idea to allow this. I'd like to get some input before going further.

TODO:

• Update changelog
• Update/add tests

[alexandre-daubois]

Makes sense to me. I think I never came across a service that sends webhook events by batches like this. But to me, that's a nice idea if this exists. I'd be 👍 on this, especially since the implementation is pretty simple!

[kbond]

I think I never came across a service that sends webhook events by batches like this.

Until yesterday, I hadn't either. When working on #58252 I discovered Mailtrap does this and then saw #50324 is blocked because of this also.

[stof]

This is technically a BC break for callers of the parser (as the caller needs to be able to deal with an array).

However, as the typical usage of this parser is to be called by the WebhookController of symfony/webhook (while external packages implement the interface), I would not oppose shipping that in a minor version of the component, with an explicit mention about that in the changelog and upgrade guide. But I'd like to know what other members of the core team think about it.

[stof]

Based on the comment in #50324 (comment), it seems like Sendgrid also batches webhooks but our parser silently ignore all events except the first one.

[kbond]

our parser silently ignore all events except the first one.

😬

Once multiple messages can be parsed in one request, what are your thoughts on what to do when just one fails? Each parser could do there own thing if I'm understanding correctly.

[stof]

Each parser can have its own logic to decide when it returns null (to ignore the webhook) and when it throws an exception

[kbond]

I've added tests, updated changelog and upgrade guide. If this minor BC Break is acceptable, I think this is good to go.

it seems like Sendgrid also batches webhooks but our parser silently ignore all events except the first one.

When/if merged, I'll fix the sendgrid bridge.

[fabpot]

As we already have 3 bridges that would work correctly without this change, I would consider this BC break as something we need to fix in 7.2.

[nicolas-grekas]

What about introducing a parseAll method?
RemoteEvent|array<RemoteEvent> is tricky to deal with I fear.

[stof]

Adding a new method will be a BC break for all implementations . And right now, we have dozens of implementation, all in external packages (either in bridge components or third-party) while we have only 1 known caller in the webhook component itself (and so no impacted by a BC break as its update is shipped in the same package).

[nicolas-grekas]

With an @method of course.

[nicolas-grekas]

Or a new interface / abstract class.

[stof]

but that would still force bridges to implement the parse method returning a single one (and so being broken for batched webhooks)

[mahono]

Just for curiosity: does this somehow allow streaming events to a webhook?

[kbond]

Just for curiosity: does this somehow allow streaming events to a webhook?

No, just accounts for some webhook providers that send multiple events in a single request.

[kbond]

What about introducing a parseAll method?

I thought about this and then, in WebhookController, use parseAll when available and parse if not (trigger deprecation in this case). I ran into the same thing that @stof describes - a lot more 3rd party code that would need updating.

FYI, I did a search on packagist and couldn't find any open source code that consumes parsers.

[fabpot]

Thank you @kbond.