HTML emails

Summary:
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
 - Mail.app

====Bad clients====

- [[ http://airmailapp.com/ | Airmail ]]. They confuse Gmail too, though.

====Need testing====
 - Outlook (Windows + Mac)

Reviewers: chad, #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: webframp, taoqiping, chad, epriestley, Korvin

Maniphest Tasks: T992

Differential Revision: https://secure.phabricator.com/D9375

src/__phutil_library_map__.php

@@ -1728,6 +1728,7 @@ phutil_register_library_map(array(
     'PhabricatorMetaMTAMail' => 'applications/metamta/storage/PhabricatorMetaMTAMail.php',
     'PhabricatorMetaMTAMailBody' => 'applications/metamta/view/PhabricatorMetaMTAMailBody.php',
     'PhabricatorMetaMTAMailBodyTestCase' => 'applications/metamta/view/__tests__/PhabricatorMetaMTAMailBodyTestCase.php',
+    'PhabricatorMetaMTAMailSection' => 'applications/metamta/view/PhabricatorMetaMTAMailSection.php',
     'PhabricatorMetaMTAMailTestCase' => 'applications/metamta/storage/__tests__/PhabricatorMetaMTAMailTestCase.php',
     'PhabricatorMetaMTAMailableDatasource' => 'applications/metamta/typeahead/PhabricatorMetaMTAMailableDatasource.php',
     'PhabricatorMetaMTAMailgunReceiveController' => 'applications/metamta/controller/PhabricatorMetaMTAMailgunReceiveController.php',

src/applications/config/option/PhabricatorMetaMTAConfigOptions.php

@@ -331,7 +331,7 @@ EODOC
             'in bytes.'))
         ->setSummary(pht('Global cap for size of generated emails (bytes).'))
         ->addExample(524288, pht('Truncate at 512KB'))
-        ->addExample(1048576, pht('Truncate at 1MB'))
+        ->addExample(1048576, pht('Truncate at 1MB')),
     );
   }
 

src/applications/differential/editor/DifferentialTransactionEditor.php

@@ -1179,20 +1179,21 @@ final class DifferentialTransactionEditor
       $config_attach = PhabricatorEnv::getEnvConfig($config_key_attach);
 
       if ($config_inline || $config_attach) {
-        $patch = $this->renderPatchForMail($diff);
-        $lines = count(phutil_split_lines($patch));
+        $patch_section = $this->renderPatchForMail($diff);
+        $lines = count(phutil_split_lines($patch_section->getPlaintext()));
 
         if ($config_inline && ($lines <= $config_inline)) {
           $body->addTextSection(
             pht('CHANGE DETAILS'),
-            $patch);
+            $patch_section);
         }
 
         if ($config_attach) {
           $name = pht('D%s.%s.patch', $object->getID(), $diff->getID());
           $mime_type = 'text/x-patch; charset=utf-8';
           $body->addAttachment(
-            new PhabricatorMetaMTAAttachment($patch, $name, $mime_type));
+            new PhabricatorMetaMTAAttachment(
+              $patch_section->getPlaintext(), $name, $mime_type));
         }
       }
     }
@@ -1330,7 +1331,7 @@ final class DifferentialTransactionEditor
       $hunk_parser = new DifferentialHunkParser();
     }
 
-    $result = array();
+    $section = new PhabricatorMetaMTAMailSection();
     foreach ($inline_groups as $changeset_id => $group) {
       $changeset = idx($changesets, $changeset_id);
       if (!$changeset) {
@@ -1351,25 +1352,27 @@ final class DifferentialTransactionEditor
         $inline_content = $comment->getContent();
 
         if (!$show_context) {
-          $result[] = "{$file}:{$range} {$inline_content}";
+          $section->addFragment("{$file}:{$range} {$inline_content}");
         } else {
-          $result[] = '================';
-          $result[] = 'Comment at: '.$file.':'.$range;
-          $result[] = $hunk_parser->makeContextDiff(
+          $patch = $hunk_parser->makeContextDiff(
             $changeset->getHunks(),
             $comment->getIsNewFile(),
             $comment->getLineNumber(),
             $comment->getLineLength(),
             1);
-          $result[] = '----------------';
 
-          $result[] = $inline_content;
-          $result[] = null;
+          $section->addFragment('================')
+                  ->addFragment('Comment at: '.$file.':'.$range)
+                  ->addPlaintextFragment($patch)
+                  ->addHTMLFragment($this->renderPatchHTMLForMail($patch))
+                  ->addFragment('----------------')
+                  ->addFragment($inline_content)
+                  ->addFragment(null);
         }
       }
     }
 
-    return implode("\n", $result);
+    return $section;
   }
 
   private function loadDiff($phid, $need_changesets = false) {
@@ -1762,14 +1765,25 @@ final class DifferentialTransactionEditor
     return implode("\n", $filenames);
   }
 
+  private function renderPatchHTMLForMail($patch) {
+    return phutil_tag('pre',
+      array('style' => 'font-family: monospace;'), $patch);
+  }
+
   private function renderPatchForMail(DifferentialDiff $diff) {
     $format = PhabricatorEnv::getEnvConfig('metamta.differential.patch-format');
 
-    return id(new DifferentialRawDiffRenderer())
+    $patch = id(new DifferentialRawDiffRenderer())
       ->setViewer($this->getActor())
       ->setFormat($format)
       ->setChangesets($diff->getChangesets())
       ->buildPatch();
+
+    $section = new PhabricatorMetaMTAMailSection();
+    $section->addHTMLFragment($this->renderPatchHTMLForMail($patch));
+    $section->addPlaintextFragment($patch);
+
+    return $section;
   }
 
 }

src/applications/metamta/adapter/PhabricatorMailImplementationAdapter.php

@@ -8,9 +8,9 @@ abstract class PhabricatorMailImplementationAdapter {
   abstract public function addCCs(array $emails);
   abstract public function addAttachment($data, $filename, $mimetype);
   abstract public function addHeader($header_name, $header_value);
-  abstract public function setBody($body);
+  abstract public function setBody($plaintext_body);
+  abstract public function setHTMLBody($html_body);
   abstract public function setSubject($subject);
-  abstract public function setIsHTML($is_html);
 
   /**
    * Some mailers, notably Amazon SES, do not support us setting a specific

src/applications/metamta/adapter/PhabricatorMailImplementationMailgunAdapter.php

@@ -57,13 +57,13 @@ final class PhabricatorMailImplementationMailgunAdapter
     return $this;
   }
 
-  public function setSubject($subject) {
-    $this->params['subject'] = $subject;
+  public function setHTMLBody($html_body) {
+    $this->params['html-body'] = $html_body;
     return $this;
   }
 
-  public function setIsHTML($is_html) {
-    $this->params['is-html'] = $is_html;
+  public function setSubject($subject) {
+    $this->params['subject'] = $subject;
     return $this;
   }
 
@@ -78,11 +78,10 @@ final class PhabricatorMailImplementationMailgunAdapter
 
     $params['to'] = implode(', ', idx($this->params, 'tos', array()));
     $params['subject'] = idx($this->params, 'subject');
-
-    if (idx($this->params, 'is-html')) {
-      $params['html'] = idx($this->params, 'body');
-    } else {
     $params['text'] = idx($this->params, 'body');
+
+    if (idx($this->params, 'html-body')) {
+      $params['html'] = idx($this->params, 'html-body');
     }
 
     $from = idx($this->params, 'from');

src/applications/metamta/adapter/PhabricatorMailImplementationPHPMailerAdapter.php

@@ -91,20 +91,22 @@ final class PhabricatorMailImplementationPHPMailerAdapter
   }
 
   public function setBody($body) {
+    $this->mailer->IsHTML(false);
     $this->mailer->Body = $body;
     return $this;
   }
 
+  public function setHTMLBody($html_body) {
+    $this->mailer->IsHTML(true);
+    $this->mailer->Body = $html_body;
+    return $this;
+  }
+
   public function setSubject($subject) {
     $this->mailer->Subject = $subject;
     return $this;
   }
 
-  public function setIsHTML($is_html) {
-    $this->mailer->IsHTML($is_html);
-    return $this;
-  }
-
   public function hasValidRecipients() {
     return true;
   }

src/applications/metamta/adapter/PhabricatorMailImplementationPHPMailerLiteAdapter.php

@@ -70,6 +70,19 @@ class PhabricatorMailImplementationPHPMailerLiteAdapter
 
   public function setBody($body) {
     $this->mailer->Body = $body;
+    $this->mailer->IsHTML(false);
+    return $this;
+  }
+
+
+  /**
+   * Note: phpmailer-lite does NOT support sending messages with mixed version
+   * (plaintext and html). So for now lets just use HTML if it's available.
+   * @param $html
+   */
+  public function setHTMLBody($html_body) {
+    $this->mailer->Body = $html_body;
+    $this->mailer->IsHTML(true);
     return $this;
   }
 
@@ -78,11 +91,6 @@ class PhabricatorMailImplementationPHPMailerLiteAdapter
     return $this;
   }
 
-  public function setIsHTML($is_html) {
-    $this->mailer->IsHTML($is_html);
-    return $this;
-  }
-
   public function hasValidRecipients() {
     return true;
   }

src/applications/metamta/adapter/PhabricatorMailImplementationSendGridAdapter.php

@@ -56,13 +56,14 @@ final class PhabricatorMailImplementationSendGridAdapter
     return $this;
   }
 
-  public function setSubject($subject) {
-    $this->params['subject'] = $subject;
+  public function setHTMLBody($body) {
+    $this->params['html-body'] = $body;
     return $this;
   }
 
-  public function setIsHTML($is_html) {
-    $this->params['is-html'] = $is_html;
+
+  public function setSubject($subject) {
+    $this->params['subject'] = $subject;
     return $this;
   }
 
@@ -89,10 +90,10 @@ final class PhabricatorMailImplementationSendGridAdapter
     }
 
     $params['subject'] = idx($this->params, 'subject');
-    if (idx($this->params, 'is-html')) {
-      $params['html'] = idx($this->params, 'body');
-    } else {
     $params['text'] = idx($this->params, 'body');
+
+    if (idx($this->params, 'html-body')) {
+      $params['html'] = idx($this->params, 'html-body');
     }
 
     $params['from'] = idx($this->params, 'from');

src/applications/metamta/adapter/PhabricatorMailImplementationTestAdapter.php

@@ -64,13 +64,13 @@ final class PhabricatorMailImplementationTestAdapter
     return $this;
   }
 
-  public function setSubject($subject) {
-    $this->guts['subject'] = $subject;
+  public function setHTMLBody($html_body) {
+    $this->guts['html-body'] = $html_body;
     return $this;
   }
 
-  public function setIsHTML($is_html) {
-    $this->guts['is-html'] = $is_html;
+  public function setSubject($subject) {
+    $this->guts['subject'] = $subject;
     return $this;
   }
 

src/applications/metamta/management/PhabricatorMailManagementSendTestWorkflow.php

@@ -105,7 +105,6 @@ final class PhabricatorMailManagementSendTestWorkflow
     $subject = $args->getArg('subject');
     $tags = $args->getArg('tag');
     $attach = $args->getArg('attach');
-    $is_html = $args->getArg('html');
     $is_bulk = $args->getArg('bulk');
 
     $console->writeErr("%s\n", pht('Reading message body from stdin...'));
@@ -117,10 +116,19 @@ final class PhabricatorMailManagementSendTestWorkflow
       ->addCCs($ccs)
       ->setSubject($subject)
       ->setBody($body)
-      ->setIsHTML($is_html)
       ->setIsBulk($is_bulk)
       ->setMailTags($tags);
 
+    if ($args->getArg('html')) {
+      $mail->setBody(
+        pht('(This is a placeholder plaintext email body for a test message '.
+            'sent with --html.)'));
+
+      $mail->setHTMLBody($body);
+    } else {
+      $mail->setBody($body);
+    }
+
     if ($from) {
       $mail->setFrom($from->getPHID());
     }

src/applications/metamta/storage/PhabricatorMetaMTAMail.php

@@ -212,13 +212,13 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
     return $this;
   }
 
-  public function getBody() {
-    return $this->getParam('body');
+  public function setHTMLBody($html) {
+    $this->setParam('html-body', $html);
+    return $this;
   }
 
-  public function setIsHTML($html) {
-    $this->setParam('is-html', $html);
-    return $this;
+  public function getBody() {
+    return $this->getParam('body');
   }
 
   public function setIsErrorEmail($is_error) {
@@ -377,6 +377,32 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
       $add_cc = array();
       $add_to = array();
 
+      // Only try to use preferences if everything is multiplexed, so we
+      // get consistent behavior.
+      $use_prefs = self::shouldMultiplexAllMail();
+
+      $prefs = null;
+      if ($use_prefs) {
+
+        // If multiplexing is enabled, some recipients will be in "Cc"
+        // rather than "To". We'll move them to "To" later (or supply a
+        // dummy "To") but need to look for the recipient in either the
+        // "To" or "Cc" fields here.
+        $target_phid = head(idx($params, 'to', array()));
+        if (!$target_phid) {
+          $target_phid = head(idx($params, 'cc', array()));
+        }
+
+        if ($target_phid) {
+          $user = id(new PhabricatorUser())->loadOneWhere(
+            'phid = %s',
+            $target_phid);
+          if ($user) {
+            $prefs = $user->loadPreferences();
+          }
+        }
+      }
+
       foreach ($params as $key => $value) {
         switch ($key) {
           case 'from':
@@ -444,42 +470,7 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
                 $attachment->getMimeType());
             }
             break;
-          case 'body':
-            $max = PhabricatorEnv::getEnvConfig('metamta.email-body-limit');
-            if (strlen($value) > $max) {
-              $value = phutil_utf8_shorten($value, $max);
-              $value .= "\n";
-              $value .= pht('(This email was truncated at %d bytes.)', $max);
-            }
-            $mailer->setBody($value);
-            break;
           case 'subject':
-            // Only try to use preferences if everything is multiplexed, so we
-            // get consistent behavior.
-            $use_prefs = self::shouldMultiplexAllMail();
-
-            $prefs = null;
-            if ($use_prefs) {
-
-              // If multiplexing is enabled, some recipients will be in "Cc"
-              // rather than "To". We'll move them to "To" later (or supply a
-              // dummy "To") but need to look for the recipient in either the
-              // "To" or "Cc" fields here.
-              $target_phid = head(idx($params, 'to', array()));
-              if (!$target_phid) {
-                $target_phid = head(idx($params, 'cc', array()));
-              }
-
-              if ($target_phid) {
-                $user = id(new PhabricatorUser())->loadOneWhere(
-                  'phid = %s',
-                  $target_phid);
-                if ($user) {
-                  $prefs = $user->loadPreferences();
-                }
-              }
-            }
-
             $subject = array();
 
             if ($is_threaded) {
@@ -518,11 +509,6 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
 
             $mailer->setSubject(implode(' ', array_filter($subject)));
             break;
-          case 'is-html':
-            if ($value) {
-              $mailer->setIsHTML(true);
-            }
-            break;
           case 'is-bulk':
             if ($value) {
               if (PhabricatorEnv::getEnvConfig('metamta.precedence-bulk')) {
@@ -570,6 +556,26 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
         }
       }
 
+      $body = idx($params, 'body', '');
+      $max = PhabricatorEnv::getEnvConfig('metamta.email-body-limit');
+      if (strlen($body) > $max) {
+        $body = phutil_utf8_shorten($body, $max);
+        $body .= "\n";
+        $body .= pht('(This email was truncated at %d bytes.)', $max);
+      }
+      $mailer->setBody($body);
+
+      $html_emails = false;
+      if ($use_prefs && $prefs) {
+        $html_emails = $prefs->getPreference(
+          PhabricatorUserPreferences::PREFERENCE_HTML_EMAILS,
+          $html_emails);
+      }
+
+      if ($html_emails && isset($params['html-body'])) {
+        $mailer->setHTMLBody($params['html-body']);
+      }
+
       if (!$add_to && !$add_cc) {
         $this->setStatus(self::STATUS_VOID);
         $this->setMessage(

src/applications/metamta/view/PhabricatorMetaMTAMailBody.php

@@ -9,6 +9,7 @@
 final class PhabricatorMetaMTAMailBody {
 
   private $sections = array();
+  private $htmlSections = array();
   private $attachments = array();
 
 
@@ -24,11 +25,27 @@ final class PhabricatorMetaMTAMailBody {
    */
   public function addRawSection($text) {
     if (strlen($text)) {
-      $this->sections[] = rtrim($text);
+      $text = rtrim($text);
+      $this->sections[] = $text;
+      $this->htmlSections[] = phutil_escape_html_newlines(
+        phutil_tag('div', array(), $text));
     }
     return $this;
   }
 
+  public function addRawPlaintextSection($text) {
+    if (strlen($text)) {
+      $text = rtrim($text);
+      $this->sections[] = $text;
+    }
+    return $this;
+  }
+
+  public function addRawHTMLSection($html) {
+    $this->htmlSections[] = phutil_safe_html($html);
+    return $this;
+  }
+
 
   /**
    * Add a block of text with a section header. This is rendered like this:
@@ -41,11 +58,32 @@ final class PhabricatorMetaMTAMailBody {
    * @return this
    * @task compose
    */
-  public function addTextSection($header, $text) {
+  public function addTextSection($header, $section) {
+    if ($section instanceof PhabricatorMetaMTAMailSection) {
+      $plaintext = $section->getPlaintext();
+      $html = $section->getHTML();
+    } else {
+      $plaintext = $section;
+      $html = phutil_escape_html_newlines(phutil_tag('div', array(), $section));
+    }
+
+    $this->addPlaintextSection($header, $plaintext);
+    $this->addHTMLSection($header, $html);
+    return $this;
+  }
+
+  public function addPlaintextSection($header, $text) {
     $this->sections[] = $header."\n".$this->indent($text);
     return $this;
   }
 
+  public function addHTMLSection($header, $html_fragment) {
+    $this->htmlSections[] = array(
+      phutil_tag('div', array('style' => 'font-weight:800;'), $header),
+      $html_fragment);
+
+    return $this;
+  }
 
   /**
    * Add a Herald section with a rule management URI and a transcript URI.
@@ -114,6 +152,11 @@ final class PhabricatorMetaMTAMailBody {
     return implode("\n\n", $this->sections)."\n";
   }
 
+  public function renderHTML() {
+    $br = phutil_tag('br');
+    $body = phutil_implode_html(array($br, $br), $this->htmlSections);
+    return (string)hsprintf('%s', array($body, $br));
+  }
 
   /**
    * Retrieve attachments.

src/applications/metamta/view/PhabricatorMetaMTAMailSection.php

@@ -0,0 +1,40 @@
+<?php
+/**
+ * Helper for building a rendered section.
+ *
+ * @task compose  Composition
+ * @task render   Rendering
+ * @group metamta
+ */
+
+final class PhabricatorMetaMTAMailSection {
+  private $plaintextFragments = array();
+  private $htmlFragments = array();
+
+  public function getHTML() {
+    return $this->htmlFragments;
+  }
+
+  public function getPlaintext() {
+    return implode("\n", $this->plaintextFragments);
+  }
+
+  public function addHTMLFragment($fragment) {
+    $this->htmlFragments[] = $fragment;
+    return $this;
+
+  }
+
+  public function addPlaintextFragment($fragment) {
+    $this->plaintextFragments[] = $fragment;
+    return $this;
+  }
+
+  public function addFragment($fragment) {
+    $this->plaintextFragments[] = $fragment;
+    $this->htmlFragments[] =
+      phutil_escape_html_newlines(phutil_tag('div', array(), $fragment));
+
+    return $this;
+  }
+}

src/applications/settings/panel/PhabricatorSettingsPanelEmailFormat.php

@@ -22,6 +22,7 @@ final class PhabricatorSettingsPanelEmailFormat
 
     $pref_re_prefix = PhabricatorUserPreferences::PREFERENCE_RE_PREFIX;
     $pref_vary = PhabricatorUserPreferences::PREFERENCE_VARY_SUBJECT;
+    $prefs_html_email = PhabricatorUserPreferences::PREFERENCE_HTML_EMAILS;
 
     $errors = array();
     if ($request->isFormPost()) {
@@ -42,6 +43,14 @@ final class PhabricatorSettingsPanelEmailFormat
             $pref_vary,
             $request->getBool($pref_vary));
         }
+
+        if ($request->getStr($prefs_html_email) == 'default') {
+          $preferences->unsetPreference($prefs_html_email);
+        } else {
+          $preferences->setPreference(
+            $prefs_html_email,
+            $request->getBool($prefs_html_email));
+        }
       }
 
       $preferences->save();
@@ -58,6 +67,8 @@ final class PhabricatorSettingsPanelEmailFormat
       ? pht('Vary')
       : pht('Do Not Vary');
 
+    $html_emails_default = 'Plain Text';
+
     $re_prefix_value = $preferences->getPreference($pref_re_prefix);
     if ($re_prefix_value === null) {
       $re_prefix_value = 'default';
@@ -76,11 +87,30 @@ final class PhabricatorSettingsPanelEmailFormat
         : 'false';
     }
 
+    $html_emails_value = $preferences->getPreference($prefs_html_email);
+    if ($html_emails_value === null) {
+      $html_emails_value = 'default';
+    } else {
+      $html_emails_value = $html_emails_value
+        ? 'true'
+        : 'false';
+    }
+
     $form = new AphrontFormView();
     $form
       ->setUser($user);
 
     if (PhabricatorMetaMTAMail::shouldMultiplexAllMail()) {
+      $html_email_control = id(new AphrontFormSelectControl())
+        ->setName($prefs_html_email)
+        ->setOptions(
+          array(
+            'default'   => pht('Default (%s)', $html_emails_default),
+            'true'      => pht('Send HTML Email'),
+            'false'     => pht('Send Plain Text Email'),
+          ))
+        ->setValue($html_emails_value);
+
       $re_control = id(new AphrontFormSelectControl())
         ->setName($pref_re_prefix)
         ->setOptions(
@@ -101,6 +131,9 @@ final class PhabricatorSettingsPanelEmailFormat
           ))
         ->setValue($vary_value);
     } else {
+      $html_email_control = id(new AphrontFormStaticControl())
+        ->setValue('Server Default ('.$html_emails_default.')');
+
       $re_control = id(new AphrontFormStaticControl())
         ->setValue('Server Default ('.$re_prefix_default.')');
 
@@ -124,6 +157,18 @@ final class PhabricatorSettingsPanelEmailFormat
     }
 
     $form
+      ->appendRemarkupInstructions(
+        pht(
+          "You can use the **HTML Email** setting to control whether ".
+          "Phabricator send you HTML email (which has more color and ".
+          "formatting) or plain text email (which is more compatible).\n".
+          "\n".
+          "WARNING: This feature is new and experimental! If you enable ".
+          "it, mail may not render properly and replying to mail may not ".
+          "work as well."))
+      ->appendChild(
+        $html_email_control
+          ->setLabel(pht('HTML Email')))
       ->appendRemarkupInstructions('')
       ->appendRemarkupInstructions(
         pht(

src/applications/settings/storage/PhabricatorUserPreferences.php

@@ -15,6 +15,7 @@ final class PhabricatorUserPreferences extends PhabricatorUserDAO {
   const PREFERENCE_NO_MAIL              = 'no-mail';
   const PREFERENCE_MAILTAGS             = 'mailtags';
   const PREFERENCE_VARY_SUBJECT         = 'vary-subject';
+  const PREFERENCE_HTML_EMAILS          = 'html-emails';
 
   const PREFERENCE_SEARCHBAR_JUMP       = 'searchbar-jump';
   const PREFERENCE_SEARCH_SHORTCUT      = 'search-shortcut';

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -1865,7 +1865,8 @@ abstract class PhabricatorApplicationTransactionEditor
       ->setExcludeMailRecipientPHIDs($this->getExcludeMailRecipientPHIDs())
       ->setMailTags($mail_tags)
       ->setIsBulk(true)
-      ->setBody($body->render());
+      ->setBody($body->render())
+      ->setHTMLBody($body->renderHTML());
 
     foreach ($body->getAttachments() as $attachment) {
       $template->addAttachment($attachment);