[Mailer] Dispatch event for Postmark's "inactive recipient" API error #52916

vicdelfant · 2023-12-06T16:48:55Z

Q	A
Branch?	7.1
Bug fix?	no
New feature?	yes
Deprecations?	no
Issues	Fix #50165
License	MIT

Given the use of Postmark and a recipient that previously generated a bounce, attempting to send another email to that email address results in an HTTP 422 response, along with the error code 406 - Inactive Recipient. In the real world, this situation can arise easily in cases of a typo or an email address that had temporary issues.

Because PostmarkApiTransport requires an HTTP 200 and throws a HttpTransportException for any other HTTP code, something that's of minor interest to the application itself (i.e. a possibly inactive e-mail address) now causes an exception. Depending on the userland logic, this can halt a process that sends out survey reminders, cause the Messenger component to queue the message for retrying etc.

To handle this more elegantly, I'm proposing the following changes:

Add a PostmarkDeliveryEventFactory for casting any (future) delivery events to an instance of PostmarkDeliveryEvent. Currently, only support the 406 'inactive recipient' error is included;
Adjust the PostmarkApiTransport so it checks for supported delivery events on an HTTP code other than 200, and if so, dispatches the event accordingly.

We cannot port this logic to the Postmark SMTP transport because, according to Postmark's own documentation, these error response codes are only provided by their API endpoints.

carsonbot · 2023-12-06T16:49:02Z

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (see https://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
Features and deprecations must be submitted against the 7.1 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

src/Symfony/Component/Mailer/Bridge/Postmark/Event/PostmarkDeliveryEventFactory.php

fabpot · 2023-12-08T14:18:02Z

src/Symfony/Component/Mailer/Bridge/Postmark/Transport/PostmarkApiTransport.php

+            if (null !== $this->dispatcher && $eventFactory->supports($result['ErrorCode'])) {
+                $this->dispatcher->dispatch(
+                    $eventFactory->create($result['ErrorCode'], $result['Message'], $email),
+                    PostmarkEvents::DELIVERY


Suggested change

PostmarkEvents::DELIVERY

PostmarkEvents::DELIVERY,

fabpot · 2023-12-08T14:19:10Z

Thank you @vicdelfant.

vicdelfant · 2023-12-08T14:28:33Z

Thanks for merging @fabpot! On to the next one :)

atymic · 2024-02-28T13:38:18Z

Wooo! I assume this will be in the 7.1 release, will check if laravel needs a PR to handle.

stof · 2024-02-28T13:52:13Z

src/Symfony/Component/Mailer/Bridge/Postmark/Event/PostmarkDeliveryEvent.php

+        return $this->headers->get('Message-ID')->getBodyAsString();
+    }
+
+    public function setHeaders(Headers $headers): self


what is the reason for making this mutable instead of passing it in the constructor ?

No particular reason — it's a personal preference to keep the constructor as compact as possible, with the absolute minimal required data to set up the object in a valid, initialized state.

This setter means that listeners can replace headers in the event (which will at least impact next listeners). Anything that should not be mutable from listeners should not have setters.

stof · 2024-02-28T13:53:05Z

src/Symfony/Component/Mailer/Bridge/Postmark/Transport/PostmarkApiTransport.php

@@ -69,6 +74,18 @@ protected function doSendApi(SentMessage $sentMessage, Email $email, Envelope $e
        }

        if (200 !== $statusCode) {
+            $eventFactory = new PostmarkDeliveryEventFactory();


should we instantiate a new one each time or treat it as a dependency of the class stored in a property ?

Certainly, this could be a dependency that we instantiate in the constructor.

stof · 2024-02-28T13:54:14Z

src/Symfony/Component/Mailer/Bridge/Postmark/Event/PostmarkEvents.php

+
+class PostmarkEvents
+{
+    public const DELIVERY = 'postmark.delivery';


I don't think we need this class with event names. For new code, we should rely on the event class name instead (by passing only 1 argument to dispatch())

Fair point, should this be adjusted while the PR has already been merged @fabpot?

This has not been released yet, so we can still adjust it.

stof · 2024-02-28T13:55:08Z

src/Symfony/Component/Mailer/Bridge/Postmark/Event/PostmarkDeliveryEvent.php

+
+    private Headers $headers;
+
+    private ?string $message;


why is this nullable when the constructor does not allow passing null ?

Good eye, this could indeed be a non-nullable property.

already fixed in #53242

stof · 2024-02-28T13:55:34Z

src/Symfony/Component/Mailer/Bridge/Postmark/Event/PostmarkDeliveryEvent.php

+
+class PostmarkDeliveryEvent
+{
+    public const CODE_INACTIVE_RECIPIENT = 406;


what is the reason to expose this constant in the public API of the bridge ?

No specific reason, except for keeping the event with the error code and the error code itself close together. Alternatively, the mapping/constant could be moved to PostmarkDeliveryEventFactory.

As the factory is the only usage of the constant (outside tests), I would suggest moving it there as a private constant (and making tests use the integer directly). Once we introduce a public API, it becomes covered by the BC promise of Symfony. So we should not introduce them without reason (it only make maintenance harder)

stof · 2024-02-28T13:58:21Z

src/Symfony/Component/Mailer/Bridge/Postmark/Transport/PostmarkApiTransport.php

+                    PostmarkEvents::DELIVERY,
+                );
+
+                return $response;


shouldn't we still throw an exception by default to report that sending the email failed ? Right now, if you have no listener doing something special, the failure will be silently ignored.
To me, skipping the exception should require that a listener marks the failure as handled in some way (by calling a method on the event object)

In my opinion, this would defeat the purpose of the PR. If we would take a traditional SMTP server as a reference, any e-mail that is valid gets sent - simple as that. This also means that you can send 100 e-mails to an e-mail address that bounced on the first try, because the SMTP server will simply accept the e-mails and try to deliver them. There would be no way for the SMTP server or the application to know that delivering the e-mail, in fact, failed (or would fail down the line). Ergo, no exceptions.

Postmark's API, on the other hand, has stricter policies and is kind enough to throw an 'inactive recipient' API error to let you know that the e-mail address bounced before, or is otherwise suppressed. This is where the problem lies: with the pre-PR implementation, the application all of a sudden has to deal with exceptions while trying to deliver an otherwise valid e-mail. Also because the exception means that the recipient could be inactive, it's in fact not 100% sure that it still is.

Sure we could wrap every sending operation in a try/catch, but in my opinion we should keep the logic of the mailer as close to its core as possible: using a traditional SMTP server that just queues mails and tries to deliver them. Anything extra is, well, extra.

Then again this is my opinion, but if we would be throwing an exception I'd rather revert the PR so keep things as simple as possible, and rely on the webhooks to detect post-delivery issues.

but this won't be sent to webhooks either (as the API will reject the sending)

If the intent is to have an exception then reverting this PR would do just that, you'd get the exception back. The bounce will actually trigger a webhook, but only the first time: https://postmarkapp.com/developer/webhooks/bounce-webhook.

carsonbot added Status: Needs Review Feature Mailer labels Dec 6, 2023

carsonbot added this to the 7.1 milestone Dec 6, 2023

OskarStark reviewed Dec 7, 2023

View reviewed changes

src/Symfony/Component/Mailer/Bridge/Postmark/Event/PostmarkDeliveryEventFactory.php Outdated Show resolved Hide resolved

fabpot approved these changes Dec 8, 2023

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Dec 8, 2023

[Mailer] Dispatch event for Postmark's "inactive recipient" API error

6ffd173

fabpot force-pushed the postmark-mailer-events branch from 4ae69c0 to 6ffd173 Compare December 8, 2023 14:19

fabpot merged commit 05b677c into symfony:7.1 Dec 8, 2023

fabpot mentioned this pull request Dec 8, 2023

[Mailer] Dispatch event for Postmark's "inactive recipient" API error symfony/symfony-docs#19233

Closed

stof reviewed Feb 28, 2024

View reviewed changes

fabpot mentioned this pull request May 2, 2024

Release v7.1.0-BETA1 #54820

Merged

Uh oh!

[Mailer] Dispatch event for Postmark's "inactive recipient" API error #52916

[Mailer] Dispatch event for Postmark's "inactive recipient" API error #52916

Uh oh!

Conversation

vicdelfant commented Dec 6, 2023

Uh oh!

carsonbot commented Dec 6, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Dec 8, 2023

Uh oh!

vicdelfant commented Dec 8, 2023

Uh oh!

atymic commented Feb 28, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!