Page MenuHomePhabricator

Rename "MetaMTA" mail attachments and add more mail message objects
ClosedPublic

Authored by epriestley on Jan 4 2019, 1:34 PM.
Tags
None
Referenced Files
F13062510: D19954.diff
Sat, Apr 20, 12:27 AM
Unknown Object (File)
Tue, Apr 16, 10:26 PM
Unknown Object (File)
Mon, Apr 8, 2:39 PM
Unknown Object (File)
Sat, Apr 6, 2:42 AM
Unknown Object (File)
Feb 1 2024, 2:20 PM
Unknown Object (File)
Jan 1 2024, 5:06 PM
Unknown Object (File)
Dec 28 2023, 5:14 PM
Unknown Object (File)
Dec 25 2023, 7:17 AM
Subscribers
None

Details

Summary

Depends on D19953. Ref T9141. We have a "MetaMTAAttachment" object, rename it to "MailAttachment".

Also add a "Header" object and an "EmailMessage" object. Currently, mail adapters have a large number of methods like setSubject(), addTo(), etc, that I would like to remove.

I'd like the API to be more like sendMessage(PhabricatorMailExternalMessage $message). This is likely a significant simplification anyway, since the implementations of all these methods are just copy/pasted boilerplate anyway (lots of $this->subject = $subject;) and this will let Adapters support other message media (SMS, APNS, Whatsapp, etc.)

That's a larger change, but move toward a world where we can build a concrete $message object for "email" or "sms".

The PhabricatorMailEmailMessage object is just a dumb, flat object representation of the information an adapter needs to actually send mail. The existing PhabricatorMetaMTAMail is a much more complex object and has a lot of rich data (delivery status, related object PHIDs, etc) and is a storage object.

The new flow will be something like:

  • PhabricatorMetaMTAMail (possibly renamed) is the storage object for any outbound message on this channel. It tracks message content, acceptable delivery media (SMS vs email), delivery status, related objects, has a PHID, and has a daemon worker associated with delivering it.
  • It builds a PhabricatorMailExternalMessage, which is a simple, flat description of the message it wants to send. The subclass of this object depends on the message medium. For email, this will be an EmailMessage. This is just a "bag of strings" sort of object, with appropriate flattened values for the adapter to work with (e.g., Email has email addresses, SMS has phone numbers).
  • It passes the ExternalMessage (which is a MailMessage or SMSMessage or whatever) to the adapter.
  • The adapter reads the nice flat properties off it and turns them into an API request, SMTP call, etc.

This is sort of how things work today anyway. The major change is that I want to hand off a "bag of strings" object instead of calling setX(), setY(), setZ() with each individual value.

Test Plan

Grepped for MetaMTAAttachment. This doesn't change any behavior yet.

Diff Detail

Repository
rP Phabricator
Branch
mfa13
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 21483
Build 29265: Run Core Tests
Build 29264: arc lint + arc unit

Event Timeline

epriestley retitled this revision from Rename "MetMTA" mail attachments and add more mail message objects to Rename "MetaMTA" mail attachments and add more mail message objects.Jan 4 2019, 1:35 PM
amckinley added inline comments.
src/applications/metamta/message/PhabricatorMailEmailMessage.php
67

Should we subclass this into PhabricatorMailEmailAttachment, etc? I can imagine a world where different delivery mechanisms have different restrictions on attachable content.

This revision is now accepted and ready to land.Jan 4 2019, 10:29 PM
src/applications/metamta/message/PhabricatorMailEmailMessage.php
67

For now, I'm hoping that "Attachment" and "Header" are generic enough that we can share them across different media, maybe by adding a few media-flavored properties/methods but still ending up with relatively cohesive objects.

I'm not sure this will pan out, but I can't immediately come up with any "Attachment" or "Header" behavior that I think will be too different? It seems sort of okay if "Attachment" has a handful of methods like isSmallEnoughForMMS() or getFilenameNormalizedForWhatsapp() or something down the road, if we can't find any better place to put them, since the class is so small/simple today. Or if "header" ends up with an optional "color" field for postcards or something. But if we end up collecting a lot of that, subclassing might be appropriate.

This revision was automatically updated to reflect the committed changes.