[Webhook] Add Mailchimp webhook #50324
Conversation
src/Symfony/Component/Mailer/Bridge/Mailchimp/Tests/Webhook/MailchimpRequestParserTest.php
Outdated
Show resolved
Hide resolved
|
Any plans to finish this PR @JohJohan ? |
Yes will wrap up today. For #50324 (comment) i will add the values in the test.json files and unset the values. |
|
I see now MailChimp will send multiple events at once see https://mailchimp.com/developer/transactional/guides/track-respond-activity-webhooks/#handling-the-webhook-response-in-your-app I'm thinking to make a class public function consume(RemoteEvent|MailerDeliveriesEvent $event): void
{
foreach ($event as $deliveryEvent) {
// Handle the event based on email
$deliveryEvent->getEmail()
}
}Maybe you also have other suggestion? I dont like it that |
| if (!\in_array($payload['event'], ['send', 'deferral', 'hard_bounce', 'soft_bounce', 'delivered', 'open', 'click', 'spam', 'unsub', 'reject'], true)) { | ||
| throw new ParseException(sprintf('Unsupported event "%s".', $payload['event'])); | ||
| } |
There was a problem hiding this comment.
We can avoid this redundant condition
See MailgunPayloadConverter
There was a problem hiding this comment.
Done added the same default case
| 'deferral', 'soft_bounce' => MailerDeliveryEvent::DEFERRED, | ||
| 'hard_bounce' => MailerDeliveryEvent::BOUNCE, |
There was a problem hiding this comment.
In BrevoPayloadConverter, soft_bounce and hard_bounce match MailerDeliveryEvent::BOUNCE, should we do same here ?
There was a problem hiding this comment.
I would say yes. A soft bounce is not deffered
There was a problem hiding this comment.
I added soft_bounce to MailerDeliveryEvent::BOUNCE
| if (null !== $payload['msg']['smtp_events'] && null !== $payload['msg']['smtp_events']['diag']) { | ||
| return $payload['msg']['smtp_events']['diag']; | ||
| } |
There was a problem hiding this comment.
According to https://mailchimp.com/developer/transactional/docs/webhooks/#detailed-webhook-format-responses, is an array of object right ?
There was a problem hiding this comment.
Your right an array of JSON objects, each of which is an SMTP response received for the message. Each item in the array will contain the following keys:
I will have to update to read every diag and concat them together
There was a problem hiding this comment.
Now i go over the events and add them all to the reason log with type and diag
Indeed, bulk event in payload is complicated with this signature (Fore this case, SendgridRequestParser consume only the first event and ignore others 😨) I'm not sure if it's the best way, but changing signature (without BC break) to return an array could be cleaner |
Yes i think only parsing the first message is not an option as you will miss the rest and in our cases allot is bulked together so it would not be worth implementing that i think. Indeed updating TLDR; The problem is that we get multiple message in one webhook request and the current interface allows for only parsing one message in a webhook request. |
| if (!\in_array($payload['event'], ['send', 'deferral', 'hard_bounce', 'soft_bounce', 'delivered', 'open', 'click', 'spam', 'unsub', 'reject'], true)) { | ||
| throw new ParseException(sprintf('Unsupported event "%s".', $payload['event'])); | ||
| } |
There was a problem hiding this comment.
Done added the same default case
| 'deferral', 'soft_bounce' => MailerDeliveryEvent::DEFERRED, | ||
| 'hard_bounce' => MailerDeliveryEvent::BOUNCE, |
There was a problem hiding this comment.
I added soft_bounce to MailerDeliveryEvent::BOUNCE
| if (null !== $payload['msg']['smtp_events'] && null !== $payload['msg']['smtp_events']['diag']) { | ||
| return $payload['msg']['smtp_events']['diag']; | ||
| } |
There was a problem hiding this comment.
Now i go over the events and add them all to the reason log with type and diag
…moteEvent`'s (kbond) This PR was merged into the 7.2 branch. Discussion ---------- [Webhook] Allow request parsers to return multiple `RemoteEvent`'s | 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: - [x] Update changelog - [x] Update/add tests Commits ------- a1fcfae [Webhook] Allow request parsers to return multiple `RemoteEvent`'s
Thanks for the feedback i updated 7.2
---
* Add support for webhook |
src/Symfony/Component/Mailer/Bridge/Mailchimp/Webhook/MailchimpRequestParser.php
Outdated
Show resolved
Hide resolved
|
Its ready for review, i dont know why https://ci.appveyor.com/project/fabpot/symfony/builds/50744376 fails: Testing C:\projects\symfony\src\Symfony\Component\Lock
SSSSSSSSSSSSSSSS..............SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS. 63 / 393 ( 16%)
.S.........SSSSSSSSSSSSSSSSSSSSSSSSSSF..........SSSSSSSSSSS.... 126 / 393 ( 32%)
..............SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS
Time: 00:50.021, Memory: 8.00 MB
There was 1 failure:
1) Symfony\Component\Lock\Tests\Store\RedisArrayStoreTest::testBackwardCompatibility
Failed asserting that false is true.
C:\projects\symfony\src\Symfony\Component\Lock\Tests\Store\AbstractRedisStoreTestCase.php:49
FAILURES!
Tests: 178, Assertions: 201, Failures: 1, Skipped: 124.
KO src\Symfony/Component/Lock
PHPUnit 9.6.21 by Sebastian Bergmann and contributors. |
|
Thank you @JohJohan. |
…are) This PR was merged into the 7.2 branch. Discussion ---------- [Mailer][Webhook] add Mandrill webhook docs Code PR: symfony/symfony#50324 Commits ------- e8084a2 [Mailer][Webhook] Mandrill Webhook support
This will add Mailchimp webhook see https://mailchimp.com/developer/transactional/docs/webhooks/