Drop empty inbound mail at the beginning of the receive workflow, not inside object handlers

Summary:
Ref T920. Ref T7477. We currently drop empty mail only once it reaches the `ReplyHandler` layer.

I think no plausible receiver can ever do anything useful with this kind of mail, so we can safely drop it earlier and simplify some of the logic. After T7477, we'd end up throwing multiple exceptions if you sent empty mail to several valid receivers.

(I also want to move away from APIs oriented around raw addresses in more specialized layers, and this is one of the few callsites for raw mail address information.)

This requires updating some unit tests to actually have message bodies, since they failed with this error before hitting the other errors otherwise.

Test Plan: Used `bin/mail receive-test` to send empty mail, got appropriate "err:empty" out of it.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T7477, T920

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

src/applications/maniphest/mail/ManiphestTaskMailReceiver.php

@@ -14,14 +14,12 @@ final class ManiphestTaskMailReceiver extends PhabricatorObjectMailReceiver {
   protected function loadObject($pattern, PhabricatorUser $viewer) {
     $id = (int)trim($pattern, 'T');
 
-    $results = id(new ManiphestTaskQuery())
+    return id(new ManiphestTaskQuery())
       ->setViewer($viewer)
       ->withIDs(array($id))
       ->needSubscriberPHIDs(true)
       ->needProjectPHIDs(true)
-      ->execute();
-
-    return head($results);
+      ->executeOne();
   }
 
   protected function getTransactionReplyHandler() {

src/applications/metamta/receiver/__tests__/PhabricatorObjectMailReceiverTestCase.php

@@ -23,14 +23,16 @@ final class PhabricatorObjectMailReceiverTestCase
       $mail->getStatus());
   }
 
-/*
-
-  TODO: Tasks don't support policies yet. Implement this once they do.
-
   public function testDropPolicyViolationMail() {
-    list($task, $user, $mail) = $this->buildMail('public');
+    list($task, $user, $mail) = $this->buildMail('policy');
 
-    // TODO: Set task policy to "no one" here.
+    $task
+      ->setViewPolicy(PhabricatorPolicies::POLICY_NOONE)
+      ->setOwnerPHID(null)
+      ->save();
+
+    $env = PhabricatorEnv::beginScopedEnv();
+    $env->overrideEnvConfig('metamta.public-replies', true);
 
     $mail->save();
     $mail->processReceivedMail();
@@ -40,8 +42,6 @@ final class PhabricatorObjectMailReceiverTestCase
       $mail->getStatus());
   }
 
-*/
-
   public function testDropInvalidObjectMail() {
     list($task, $user, $mail) = $this->buildMail('404');
 
@@ -122,6 +122,11 @@ final class PhabricatorObjectMailReceiverTestCase
         'To'         => $to,
       ));
 
+    $mail->setBodies(
+      array(
+        'text' => 'test',
+      ));
+
     return array($task, $user, $mail);
   }
 

src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php

@@ -67,40 +67,9 @@ abstract class PhabricatorMailReplyHandler extends Phobject {
     PhabricatorMetaMTAReceivedMail $mail);
 
   public function processEmail(PhabricatorMetaMTAReceivedMail $mail) {
-    $this->dropEmptyMail($mail);
-
     return $this->receiveEmail($mail);
   }
 
-  private function dropEmptyMail(PhabricatorMetaMTAReceivedMail $mail) {
-    $body = $mail->getCleanTextBody();
-    $attachments = $mail->getAttachments();
-
-    if (strlen($body) || $attachments) {
-      return;
-    }
-
-     // Only send an error email if the user is talking to just Phabricator.
-     // We can assume if there is only one "To" address it is a Phabricator
-     // address since this code is running and everything.
-    $is_direct_mail = (count($mail->getToAddresses()) == 1) &&
-                      (count($mail->getCCAddresses()) == 0);
-
-    if ($is_direct_mail) {
-      $status_code = MetaMTAReceivedMailStatus::STATUS_EMPTY;
-    } else {
-      $status_code = MetaMTAReceivedMailStatus::STATUS_EMPTY_IGNORED;
-    }
-
-    throw new PhabricatorMetaMTAReceivedMailProcessingException(
-      $status_code,
-      pht(
-        'Your message does not contain any body text or attachments, so '.
-        'Phabricator can not do anything useful with it. Make sure comment '.
-        'text appears at the top of your message: quoted replies, inline '.
-        'text, and signatures are discarded and ignored.'));
-  }
-
   public function supportsPrivateReplies() {
     return (bool)$this->getReplyHandlerDomain() &&
            !$this->supportsPublicReplies();

src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php

@@ -109,6 +109,7 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO {
     try {
       $this->dropMailFromPhabricator();
       $this->dropMailAlreadyReceived();
+      $this->dropEmptyMail();
 
       $receiver = $this->loadReceiver();
       $sender = $receiver->loadSender($this);
@@ -260,6 +261,34 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO {
       $message);
   }
 
+  private function dropEmptyMail() {
+    $body = $this->getCleanTextBody();
+    $attachments = $this->getAttachments();
+
+    if (strlen($body) || $attachments) {
+      return;
+    }
+
+    // Only send an error email if the user is talking to just Phabricator.
+    // We can assume if there is only one "To" address it is a Phabricator
+    // address since this code is running and everything.
+    $is_direct_mail = (count($this->getToAddresses()) == 1) &&
+                      (count($this->getCCAddresses()) == 0);
+
+    if ($is_direct_mail) {
+      $status_code = MetaMTAReceivedMailStatus::STATUS_EMPTY;
+    } else {
+      $status_code = MetaMTAReceivedMailStatus::STATUS_EMPTY_IGNORED;
+    }
+
+    throw new PhabricatorMetaMTAReceivedMailProcessingException(
+      $status_code,
+      pht(
+        'Your message does not contain any body text or attachments, so '.
+        'Phabricator can not do anything useful with it. Make sure comment '.
+        'text appears at the top of your message: quoted replies, inline '.
+        'text, and signatures are discarded and ignored.'));
+  }
 
   /**
    * Load a concrete instance of the @{class:PhabricatorMailReceiver} which

src/applications/metamta/storage/__tests__/PhabricatorMetaMTAReceivedMailTestCase.php

@@ -54,6 +54,10 @@ final class PhabricatorMetaMTAReceivedMailTestCase extends PhabricatorTestCase {
         'Message-ID' => 'test@example.com',
         'To'         => 'does+not+exist@example.com',
       ));
+    $mail->setBodies(
+      array(
+        'text' => 'test',
+      ));
     $mail->save();
 
     $mail->processReceivedMail();
@@ -77,6 +81,10 @@ final class PhabricatorMetaMTAReceivedMailTestCase extends PhabricatorTestCase {
         'To'         => 'bugs@example.com',
         'From'       => 'does+not+exist@example.com',
       ));
+    $mail->setBodies(
+      array(
+        'text' => 'test',
+      ));
     $mail->save();
 
     $mail->processReceivedMail();
@@ -101,6 +109,10 @@ final class PhabricatorMetaMTAReceivedMailTestCase extends PhabricatorTestCase {
         'From'        => $user->loadPrimaryEmail()->getAddress(),
         'To'          => 'bugs@example.com',
       ));
+    $mail->setBodies(
+      array(
+        'text' => 'test',
+      ));
     $mail->save();
 
     $mail->processReceivedMail();