Page MenuHomePhabricator

Update Postmark adapter for multiple mail media
ClosedPublic

Authored by epriestley on Jan 4 2019, 8:09 PM.

Details

Summary

Depends on D19955. Ref T920. Ref T5969. Update Postmark to accept new Message objects. Also:

  • Update the inbound whitelist.
  • Add a little support for media configuration.
  • Add a service call timeout (see T5969).
  • Drop the needless word "Implementation" from the Adapter class tree. I could call these "Mailers" instead of "Adapters", but then we get "PhabricatorMailMailer" which feels questionable.
Test Plan

Used bin/mail send-test to send mail via Postmark with various options (mulitple recipients, text vs html, attachments).

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 4 2019, 8:09 PM
Harbormaster returned this revision to the author for changes because remote builds failed.Jan 4 2019, 8:11 PM
Harbormaster failed remote builds in B21485: Diff 47632!
epriestley requested review of this revision.Jan 4 2019, 8:13 PM

I believe this change is substantially correct if we accept that D19955 is substantially correct, but tests should pass again soon to provide slightly more confidence.

src/applications/metamta/adapter/PhabricatorMailImplementationPostmarkAdapter.php
3

(This was moved, not deleted, but the new file isn't similar enough for Git to figure it out.)

amckinley accepted this revision.Jan 5 2019, 12:03 AM
amckinley added inline comments.
src/applications/metamta/adapter/PhabricatorMailAdapter.php
26–30

Are these commented out as a migration step?

src/applications/metamta/adapter/PhabricatorMailPHPMailerLiteAdapter.php
6

So I googled this and only got hits for Phabricator revisions. This annotation is here to satisfy the linter for classes that can't be marked as abstract or final?

This revision is now accepted and ready to land.Jan 5 2019, 12:03 AM
epriestley added inline comments.Jan 5 2019, 12:12 AM
src/applications/metamta/adapter/PhabricatorMailAdapter.php
26–30

Yeah, since I haven't updated all the adapters we get runtime fatals loading them if these don't have default implementations. I'll remove these once I update all the adapters.

src/applications/metamta/adapter/PhabricatorMailPHPMailerLiteAdapter.php
6

Yeah, the linter was complaining.

This is a pretty old rule out of T795 very long ago, which waffled a bit around @stable and then got swapped to @concrete-extensible later or something.

The original issue was that people kept sending us diffs to replace final/private/protected with public so they could extend random things in weird ways. In the cheery days of 2011 I was initially onboard with this, but soured quickly and moved toward "final absolutely everything we can".

Over time we made it to a world where every class is either abstract or final, and I think that's the right outcome. There's a linter to warn about cases which aren't abstract or final.

This class is "wrong": it should be final. However, SES extends it to use SMTP logic. An SES adapter is not a kind of PHPMailerLite adapter and this inheritance is definitely not "pure" or "correct", but I'll hopefully fix that a diff or two down the line and remove this comment/annotation/TODO, or fix it whenever T12404 happens if not here.

Ideally, we should get rid of all of these and then make the linter rule stop allowing the exception. We only have 8 left including this one, although some may be a bit tricky to exorcise.

epriestley added inline comments.Jan 14 2019, 10:15 PM
src/applications/metamta/adapter/PhabricatorMailPHPMailerLiteAdapter.php
6

D19965 eventually fixed these classtrees to get rid of the @concrete-extensible bit.

amckinley added inline comments.Jan 16 2019, 7:15 PM
src/applications/metamta/adapter/PhabricatorMailPHPMailerLiteAdapter.php
6
This revision was automatically updated to reflect the committed changes.