Added support for side-by-side HTML and plaintext email building.
We can control if the HTML stuff is sent by by a new config, metamta.html-emails
Details
Details
- Reviewers
epriestley chad - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T992: Send rich HTML mail bodies without impacting reply handling functionality
- Commits
- Restricted Diffusion Commit
rP4c57e6d34d7a: HTML emails - Required Signatures
L28 Phacility Individual Contributor License Agreement
Been running this in our deployment for a few months now.
Well behaved clients
- Gmail
- Mail.app
Bad clients
- Airmail. They confuse Gmail too, though.
Need testing
- Outlook (Windows + Mac)
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
Is there any interest in this patch?
We've been dogfooding it for a while now, and it works pretty well (we also reply from emails a lot).
Comment Actions
We're likely 'sorta' interested in it, but not likely to find time to review/support it for a while. Our main concern will likely be support costs to the upstream.
Comment Actions
@chad: yeah, that's fair. It also needs testing across more email clients to verify that it doesn't break replies and threading.
Comment Actions
added support for colorized diffs for inline comments
added PhabricatorMetaMTAMailSection helper for building email sections
Comment Actions
Awesome! Can you arc patch D9375 locally and see if:
- rendering looks reasonable
- reply-by-email (especially to Differential which has additional html) doesn't introduce the quoted text problem seen on T185
Comment Actions
Will be back at a windows machine tomorrow, and definitely test this part
- reply-by-email (especially to Differential which has additional html) doesn't introduce the quoted text problem seen on T185
I don't currently have an inbound email setup configured, but I'll see what I can do here
Comment Actions
Actually I think I can test the rendering with something like litmus, so it should be pretty easy to verify this one.
The main thing that needs testing is the reply by mail. Do you happen to have a Mailgun/AWS/Sendgrid account?
Comment Actions
- added per-user config and removed diff colorization (will come back again in a separate patch)
Comment Actions
Some really nitpicky inlines.
Some of the handling of HTML looks like it should be broken to me. In particular, I would expect treating phutil_tag()s like strings to produce too much escaping.
I think the HTML APIs should use the HTML rendering code (i.e., accept phutil_tag(), not plain strings), and this mostly does that correctly, but I would expect stuff like this:
$body->addRawHTMLSection(implode('<br>', $html_headers));...to fail (applying too much escaping). Let me poke at this locally and see if I'm crazy...
| src/applications/config/option/PhabricatorMetaMTAConfigOptions.php | ||
|---|---|---|
| 335 | I think we should avoid introducing this option for now so we can default it to true when we do introduce it. Changing defaults later is a little sketchier than just adding a new option to control a new behavior. | |
| 343–344 | Missing and/or extra words, "HTML is enabled reply addresses" | |
| src/applications/differential/editor/DifferentialTransactionEditor.php | ||
| 1182–1183 | Prefer $patch_section. | |
| src/applications/metamta/adapter/PhabricatorMailImplementationPHPMailerLiteAdapter.php | ||
| 78 | Prefer Javadoc comments, with /** on its own line. /** * blah blah */ | |
| src/applications/metamta/management/PhabricatorMailManagementSendTestWorkflow.php | ||
| 123 | Maybe something like: pht('(This is a placeholder plaintext email body for a test message sent with --html.)'); | |
| src/applications/metamta/storage/PhabricatorMetaMTAMail.php | ||
| 562 | Oh -- although it's outside of the scope of this diff, this isn't quite correct. I can send you a followup to fix it. | |
Comment Actions
It looks like this narrowly escapes actually breaking anything, but the handling isn't quite correct, and will break if we make minor changes in the future (like wrapping the entire mail body in a <div> to apply styling in renderHTML(). See inlines to clean this up.
| src/applications/metamta/view/PhabricatorMetaMTAMailBody.php | ||
|---|---|---|
| 45–47 | strlen() on a phutil_tag() isn't meaningful -- just add this section unconditionally? These strlen() checks probably aren't really necessary anyway. | |
| 81–85 | Use an array, not string concatenation: $this->htmlSections[] = array( phutil_tag(...), $html_fragment, ); | |
| 156 | Use: $br = phutil_tag('br');
$body = phutil_implode_html(array($br, $br), $this->htmlSections);
return array($body, $br); | |
| src/applications/metamta/view/PhabricatorMetaMTAMailSection.php | ||
| 16 | Just return $this->htmlFragments. | |
| 20 | A \n inside single quotes is not a newline in PHP, it's a literal backslash + n. | |
| src/applications/settings/panel/PhabricatorSettingsPanelEmailFormat.php | ||
| 160 | Label this control and add explanatory text. | |
| src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php | ||
| 2111 | Not all transactions have colors, this sometimes produces style="color:". The transaction colors are not all HTML colors. This also seems super annoying, design-wise. Let's just drop it and we can find some more tasteful approach later (like a colored bullet)? | |
| 2120 | Use phutil_implode_html(). | |
Comment Actions
Also, let's put an explicit caveat on the option in the settings panel about it being beta/not-expected-to-work-perfectly.
| src/applications/config/option/PhabricatorMetaMTAConfigOptions.php | ||
|---|---|---|
| 335 | So should I remove the checks? I've just defaulted it to true for now | |
| 343–344 | I have no idea what I was thinking | |
| src/applications/metamta/view/PhabricatorMetaMTAMailBody.php | ||
| 156 | clients expect renderHTML() to a return a string, no an array | |
| src/applications/metamta/view/PhabricatorMetaMTAMailSection.php | ||
| 20 | I totally should've written test cases for this class. | |
| src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php | ||
| 2111 | Yeah, it's pretty crude but was necessary for us due to the long list of actions Phabricator sends ("X added a reviewer: X", "X added a subscriber: X", "X accepted your review"). | |
Comment Actions
Yeah, just remove the check against the config option for now. Once this is stable we can add the option and default it to true (unless we discover that, say, several common clients have major problems we aren't able to fix, in which case we might default it to false).
| src/applications/metamta/view/PhabricatorMetaMTAMailBody.php | ||
|---|---|---|
| 156 | Oh, sorry -- do this at the end: hsprintf('%s', array($body, $br)) | |
| src/applications/metamta/view/PhabricatorMetaMTAMailBody.php | ||
|---|---|---|
| 158 | This is a weird hack to force it to emit a string. a PhutilSafeHTML doesn't seem to get serialized? | |
Comment Actions
One inline. I'll tweak that in the pull if it seems to wokr.
| src/applications/metamta/view/PhabricatorMetaMTAMailBody.php | ||
|---|---|---|
| 158 | This will probably work and is a little more conventional: return (string)hsprintf('%s', array($body, $br)); | |
| src/applications/metamta/view/PhabricatorMetaMTAMailBody.php | ||
|---|---|---|
| 158 | Oooh you can cast things! | |