Simplify and demuddle MetaMTA send pathways

Summary:
I pretty shortsightedly made sending a side effect of save() in the case that a
server is configured for immediate sending. Move this out, make it explicit, and
get rid of all the tangles surrounding it.

The web tool now ignores the server setting and only repsects the checkbox,
which makes far more sense.

Test Plan:
Sent mails from Maniphest, Differential, and the web console. Also ran all the
unit tests. Verified headers from Maniphest.

Reviewed By: rm
Reviewers: aran, rm
CC: tuomaspelkonen, rm, jungejason, aran
Differential Revision: 200

src/applications/auth/controller/email/PhabricatorEmailLoginController.php

@@ -74,9 +74,9 @@ class PhabricatorEmailLoginController extends PhabricatorAuthController {
               $target_user->getPHID(),
             ));
           $mail->setBody(
-            "blah blah blah ".
+            "here is your link ".
               PhabricatorEnv::getURI('/login/etoken/'.$etoken.'/').'?email='.phutil_escape_uri($target_user->getEmail()));
-          $mail->save();
+          $mail->saveAndSend();
 
           $view = new AphrontRequestFailureView();
           $view->setHeader('Check Your Email');

src/applications/differential/mail/base/DifferentialMail.php

@@ -101,8 +101,7 @@ abstract class DifferentialMail {
 
     $mail->setRelatedPHID($this->getRevision()->getPHID());
 
-    // Save this to the MetaMTA queue for later delivery to the MTA.
+    $mail->saveAndSend();
-    $mail->save();
   }
 
   protected function buildSubject() {

src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php

@@ -173,10 +173,7 @@ class ManiphestTransactionEditor {
       "TASK DETAIL\n".
       "  ".$task_uri."\n";
 
-    $base = substr(md5($task->getPHID()), 0, 27).' '.pack("N", time());
+    $thread_id = '<maniphest-task-'.$task->getPHID().'>';
-    $thread_index = base64_encode($base);
-
-    $message_id = '<maniphest-task-'.$task->getPHID().'>';
 
     id(new PhabricatorMetaMTAMail())
       ->setSubject(
@@ -184,12 +181,10 @@ class ManiphestTransactionEditor {
       ->setFrom($transaction->getAuthorPHID())
       ->addTos($email_to)
       ->addCCs($email_cc)
-      ->addHeader('Thread-Index', $thread_index)
       ->addHeader('Thread-Topic', 'Maniphest Task '.$task->getID())
-      ->addHeader('In-Reply-To',  $message_id)
+      ->setThreadID($thread_id, $is_create)
-      ->addHeader('References',   $message_id)
       ->setRelatedPHID($task->getPHID())
       ->setBody($body)
-      ->save();
+      ->saveAndSend();
   }
 }

src/applications/metamta/controller/send/PhabricatorMetaMTASendController.php

@@ -33,9 +33,8 @@ class PhabricatorMetaMTASendController extends PhabricatorMetaMTAController {
       $mail->setSimulatedFailureCount($request->getInt('failures'));
       $mail->setIsHTML($request->getInt('html'));
       $mail->save();
-      if ($request->getInt('immediately') &&
+      if ($request->getInt('immediately')) {
-          !PhabricatorEnv::getEnvConfig('metamta.send-immediately')) {
+        $mail->sendNow();
-        $mail->sendNow($force_send = true);
       }
 
       return id(new AphrontRedirectResponse())

src/applications/metamta/controller/send/__init__.php

@@ -9,7 +9,6 @@
 phutil_require_module('phabricator', 'aphront/response/redirect');
 phutil_require_module('phabricator', 'applications/metamta/controller/base');
 phutil_require_module('phabricator', 'applications/metamta/storage/mail');
-phutil_require_module('phabricator', 'infrastructure/env');
 phutil_require_module('phabricator', 'view/form/base');
 phutil_require_module('phabricator', 'view/form/control/checkbox');
 phutil_require_module('phabricator', 'view/form/control/submit');

src/applications/metamta/storage/mail/PhabricatorMetaMTAMail.php

@@ -35,8 +35,6 @@ class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
   protected $nextRetry;
   protected $relatedPHID;
 
-  private $skipSendOnSave;
-
   public function __construct() {
 
     $this->status     = self::STATUS_QUEUE;
@@ -133,20 +131,27 @@ class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
     return $this;
   }
 
-  public function save() {
+  /**
-    $try_send = (PhabricatorEnv::getEnvConfig('metamta.send-immediately')) &&
+   * Save a newly created mail to the database and attempt to send it
-                (!$this->getID()) &&
+   * immediately if the server is configured for immediate sends. When
-                (!$this->skipSendOnSave);
+   * applications generate new mail they should generally use this method to
+   * deliver it. If the server doesn't use immediate sends, this has the same
+   * effect as calling save(): the mail will eventually be delivered by the
+   * MetaMTA daemon.
+   *
+   * @return this
+   */
+  public function saveAndSend() {
+    $ret = $this->save();
 
-    $ret = parent::save();
+    if (PhabricatorEnv::getEnvConfig('metamta.send-immediately')) {
-
-    if ($try_send) {
       $this->sendNow();
     }
 
     return $ret;
   }
 
+
   public function buildDefaultMailer() {
     $class_name = PhabricatorEnv::getEnvConfig('metamta.mail-adapter');
     PhutilSymbolLoader::loadClass($class_name);
@@ -181,8 +186,6 @@ class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
       }
     }
 
-    $this->skipSendOnSave = true;
-
     try {
       $parameters = $this->parameters;
       $phids = array();

src/applications/repository/worker/herald/PhabricatorRepositoryCommitHeraldWorker.php

@@ -133,6 +133,6 @@ EOBODY;
       $mailer->setFrom($author_phid);
     }
 
-    $mailer->save();
+    $mailer->saveAndSend();
   }
 }