Upgrade Sendgrid to the modern mailer API; removes "api-user" option
ClosedPublic
Actions

Authored by epriestley on Jan 5 2019, 2:32 PM.

Details

Reviewers

Maniphest Tasks

T920: Provide SMS Support
T5969: Add timeouts to service calls to external mailers (was: PhabricatorMetaMTAWorker may hang indefinitely if "sendmail" hangs indefinitely)

Commits

rP64e3296fe682: Upgrade Sendgrid to the modern mailer API; removes "api-user" option

Summary

Ref T920. Ref T5969.

Update to the new "$message" API.
Update to Sendgrid v3.
Add a timeout.
This removes the "api-user" option, which Sendgrid no longer seems to use.

Test Plan

Sent Sendgrid messages with bin/mail send-test ... using subject/headers/attachments/html/to/cc.

Diff Detail

Repository

rP Phabricator

Branch

mfa8

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 21502
Build 29291: Run Core Tests
Build 29290: arc lint + arc unit

Event Timeline

epriestley created this revision.Jan 5 2019, 2:32 PM

Harbormaster completed remote builds in B21501: Diff 47647.Jan 5 2019, 2:34 PM

epriestley requested review of this revision.Jan 5 2019, 2:34 PM

epriestley retitled this revision from Upgrade Sengrid to the modern mailer API; removes "api-user" option to Upgrade Sendgrid to the modern mailer API; removes "api-user" option.Jan 5 2019, 2:35 PM

epriestley added inline comments.

src/applications/metamta/adapter/PhabricatorMailSendGridAdapter.php
128–134	In the several years since I filed this support ticket, SendGrid now officially supports "CC".

Fix a comment.

Harbormaster completed remote builds in B21502: Diff 47648.Jan 5 2019, 2:39 PM

amckinley accepted this revision.Jan 10 2019, 4:52 PM

amckinley added inline comments.

src/applications/metamta/adapter/PhabricatorMailSendGridAdapter.php
121	I guess we can't wrap this in an `OpaqueEnvelope` because it needs that "Bearer" prefix?
124	I was thinking about this for the previous revision, too: won't this break the sending of very large attachments? I guess Phabricator is pretty good about not generating multi-gigabyte payloads, so in practice this probably isn't an issue.

This revision is now accepted and ready to land.Jan 10 2019, 4:52 PM

epriestley added inline comments.Jan 14 2019, 10:13 PM

src/applications/metamta/adapter/PhabricatorMailSendGridAdapter.php
121	Right. We could do some kind of `addSensitiveHeader()` mumbo-jumbo (or let you pass either an `OpaqueEnvelope` or a string) but we don't currently dump request headers anywhere.
124	With some caveats in T11767, we "should" never generate a very large attachment. I think sending large attachments via email is probably not a good thing -- if a use case exists, it would be better to put them in Files and link to them, I think? Maybe something weird will crop up eventually, but for now we only attach diff/patch files, and attempt to limit their size to something reasonable.

Is it worth noting somewhere that the V2 API uses a username and password to authenticate (and cannot use an API key) and the V3 API uses just an API key (and cannot use a username or password), and those are generated/managed in totally different parts of the Sendgrid UI? Maybe a note in the documentation?