Prepare for multiple mailers of the same type
ClosedPublic
Actions

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

Details

Reviewers

amckinley

Maniphest Tasks

T12677: Support multiple mail delivery services for automatic failover
T13053: Plans: Mail Tags and Failover

Commits

rP7f2c90fbd12b: Prepare for multiple mailers of the same type

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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley created this revision.Feb 6 2018, 1:09 PM

Harbormaster completed remote builds in B19360: Diff 45567.Feb 6 2018, 1:10 PM

epriestley requested review of this revision.Feb 6 2018, 1:10 PM

epriestley added a child revision: D19003: Introduce and document a new `cluster.mailers` option for configuring multiple mailers.Feb 6 2018, 2:06 PM

amckinley requested changes to this revision.Feb 7 2018, 7:16 PM

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

epriestley added inline comments.Feb 7 2018, 7:23 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.