Page MenuHomePhabricator

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

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

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Jan 5 2019, 2:32 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".

epriestley updated this revision to Diff 47648.Jan 5 2019, 2:37 PM
  • Fix a comment.
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?

This revision was automatically updated to reflect the committed changes.