Page MenuHomePhabricator

Prepare for multiple mailers of the same type
ClosedPublic

Authored by epriestley on Feb 6 2018, 1:09 PM.

Details

Summary

Depends on D19000. Ref T13053. Ref T12677. Currently, most mailers are configured with a bunch of <mailer>.setting-name global config options.

This means that you can't configure two different SMTP servers, which is a reasonable thing to want to do in the brave new world of mail failover.

It also means you can't configure two Mailgun accounts or two SES accounts. Although this might seem a little silly, we've had more service disruptions because of policy issues / administrative error (where a particular account was disabled) than actual downtime, so maybe it's not completely ridiculous.

Realign mailers so they can take configuration directly in an explicit way. A later change will add new configuration to take advantage of this and let us move away from having ~10 global options for this stuff eventually.

(This also makes writing third-party mailers easier.)

Test Plan

Processed some mail, ran existing unit tests. But I wasn't especially thorough.

I expect later changes to provide some tools to make this more testable, so I'll vet each provider more thoroughly and add coverage for multiple mailers after that stuff is ready.

Diff Detail

Repository
rP Phabricator
Branch
mailer3
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 19387
Build 26224: Run Core Tests
Build 26223: arc lint + arc unit

Event Timeline

amckinley added inline comments.
src/applications/metamta/adapter/PhabricatorMailImplementationPHPMailerLiteAdapter.php
27

Should this be phpmailerlite.smtp-encoding?

src/applications/metamta/adapter/PhabricatorMailImplementationSendGridAdapter.php
15–16

string|null, based on the default options.

30

sendgrid.api-key

This revision now requires changes to proceed.Feb 7 2018, 7:16 PM
src/applications/metamta/adapter/PhabricatorMailImplementationPHPMailerLiteAdapter.php
27

There's no such option. Historically, it was only possible to configure one mailer and they (PHPMailer and PHPMailerLite) shared this option, which could perhaps have been called any-mailer.smtp-encoding or similar, although here it's a sendmail encoding rather than an SMTP encoding, so any-mailer.mail-encoding, maybe. In practice, this should always be set to base64 everywhere, and that's been the default for a long time. The ability to configure this could probably just be removed completely.

src/applications/metamta/adapter/PhabricatorMailImplementationSendGridAdapter.php
15–16

The default option values are intentionally not valid in this case, since we can't provide any kind of default for these values: you need to override them and replace them with strings before the mailer works.

30

Whoops!

I'm planning to go through all these mailers and re-test them with actual production data over the live internet before promoting to stable, it's just a pain to do that so I'm saving it for the end when hopefully I'm mostly done changing things.

  • Fix api-user vs api-key mixup.
This revision is now accepted and ready to land.Feb 7 2018, 7:32 PM
This revision was automatically updated to reflect the committed changes.