Page MenuHomePhabricator

HTML emails

Authored by talshiri on Jun 4 2014, 8:35 PM.
Referenced Files
Unknown Object (File)
Mon, Feb 19, 4:44 AM
Unknown Object (File)
Sun, Feb 11, 5:06 AM
Unknown Object (File)
Sat, Feb 10, 9:17 AM
Unknown Object (File)
Thu, Feb 8, 3:33 AM
Unknown Object (File)
Sun, Feb 4, 3:56 AM
Unknown Object (File)
Jan 14 2024, 7:16 AM
Unknown Object (File)
Jan 12 2024, 11:26 AM
Unknown Object (File)
Jan 6 2024, 4:23 AM
"Evil Spooky Haunted Tree" token, awarded by chad.



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

Test Plan

Been running this in our deployment for a few months now.

Well behaved clients
  • Gmail
Bad clients
  • Airmail. They confuse Gmail too, though.
Need testing
  • Outlook (Windows + Mac)

Diff Detail

rP Phabricator
Lint Skipped
Tests Skipped

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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).

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.

@chad: yeah, that's fair. It also needs testing across more email clients to verify that it doesn't break replies and threading.

added support for colorized diffs for inline comments
added PhabricatorMetaMTAMailSection helper for building email sections

talshiri edited the test plan for this revision. (Show Details)
talshiri edited the test plan for this revision. (Show Details)
talshiri edited the test plan for this revision. (Show Details)

few fixes + rebase

I can test Outlook (Windows) rendering

Awesome! Can you arc patch D9375 locally and see if:

  1. rendering looks reasonable
  2. reply-by-email (especially to Differential which has additional html) doesn't introduce the quoted text problem seen on T185
In D9375#45, @talshiri wrote:

Awesome! Can you arc patch D9375 locally and see if:

  1. rendering looks reasonable

Will be back at a windows machine tomorrow, and definitely test this part

  1. 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

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?

  • added per-user config and removed diff colorization (will come back again in a separate patch)
talshiri retitled this revision from HTML emails + basic DIFF colorizing to HTML emails.Aug 14 2014, 4:52 AM
talshiri updated this object.
talshiri removed a reviewer: chad.

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)); fail (applying too much escaping). Let me poke at this locally and see if I'm crazy...


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.


Missing and/or extra words, "HTML is enabled reply addresses"


Prefer $patch_section.


Prefer Javadoc comments, with /** on its own line.

 * blah blah

Maybe something like:

pht('(This is a placeholder plaintext email body for a test message sent with --html.)');

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.

epriestley edited edge metadata.

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.


strlen() on a phutil_tag() isn't meaningful -- just add this section unconditionally? These strlen() checks probably aren't really necessary anyway.


Use an array, not string concatenation:

$this->htmlSections[] = array(


$br = phutil_tag('br');
$body = phutil_implode_html(array($br, $br), $this->htmlSections);
return array($body, $br);

Just return $this->htmlFragments.


A \n inside single quotes is not a newline in PHP, it's a literal backslash + n.


Label this control and add explanatory text.


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)?


Use phutil_implode_html().

This revision now requires changes to proceed.Aug 14 2014, 2:41 PM

Also, let's put an explicit caveat on the option in the settings panel about it being beta/not-expected-to-work-perfectly.


So should I remove the checks? I've just defaulted it to true for now


I have no idea what I was thinking


clients expect renderHTML() to a return a string, no an array


I totally should've written test cases for this class.


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").
This can probably be refined and addressed in other ways though. Nuking it.

talshiri edited edge metadata.
  • cosmetics
talshiri edited edge metadata.

my arcanist seems to be messed up?

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).


Oh, sorry -- do this at the end:

hsprintf('%s', array($body, $br))
  • cleaned out escaping and removed colorful headers
  • removed server config option for html email

This is a weird hack to force it to emit a string. a PhutilSafeHTML doesn't seem to get serialized?

epriestley edited edge metadata.

One inline. I'll tweak that in the pull if it seems to wokr.


This will probably work and is a little more conventional:

return (string)hsprintf('%s', array($body, $br));
This revision is now accepted and ready to land.Aug 15 2014, 3:03 PM
epriestley updated this revision to Diff 24738.

Closed by commit rP4c57e6d34d7a (authored by @talshiri, committed by @epriestley).


Oooh you can cast things!