Remove email from handles

Summary:
Since user emails aren't in the user table, we had to do extra data fetching
for handles, and the emails are only used in MetaMTA, so we move the email
code into MetaMTA and remove it from handles.

Test Plan: send test emails

Reviewers: jungejason, vrana, epriestley

Reviewed By: epriestley

CC: aran, Koolvin

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

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

@@ -301,14 +301,14 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
               $value = array($value);
             }
             foreach (array_filter($value) as $phid) {
-              $phids[] = $phid;
+              $type = phid_get_type($phid);
+              $phids[$phid] = $type;
             }
             break;
         }
       }
 
-      $handles = id(new PhabricatorObjectHandleData($phids))
-        ->loadHandles();
+      $this->loadEmailAndNameDataFromPHIDs($phids);
 
       $exclude = array();
 
@@ -335,14 +335,13 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
         }
 
         if (!PhabricatorEnv::getEnvConfig('metamta.can-send-as-user')) {
-          $handle = $handles[$from];
           if (empty($params['reply-to'])) {
-            $params['reply-to'] = $handle->getEmail();
-            $params['reply-to-name'] = $handle->getFullName();
+            $params['reply-to'] = $phids[$from]['email'];
+            $params['reply-to-name'] = $phids[$from]['name'];
           }
           $mailer->setFrom(
             $default,
-            $handle->getFullName());
+            $phids[$from]['name']);
           unset($params['from']);
         }
       }
@@ -361,30 +360,24 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
       foreach ($params as $key => $value) {
         switch ($key) {
           case 'from':
-            $mailer->setFrom($handles[$value]->getEmail());
+            $mailer->setFrom($phids[$from]['email']);
             break;
           case 'reply-to':
             $mailer->addReplyTo($value, $reply_to_name);
             break;
           case 'to':
-            $emails = $this->getDeliverableEmailsFromHandles(
-              $value,
-              $handles,
-              $exclude);
-            if ($emails) {
-              $add_to = array_merge($add_to, $emails);
+            $to_emails = $this->filterSendable($value, $phids, $exclude);
+            if ($to_emails) {
+              $add_to = array_merge($add_to, $to_emails);
             }
             break;
           case 'raw-to':
             $add_to = array_merge($add_to, $value);
             break;
           case 'cc':
-            $emails = $this->getDeliverableEmailsFromHandles(
-              $value,
-              $handles,
-              $exclude);
-            if ($emails) {
-              $add_cc = $emails;
+            $cc_emails = $this->filterSendable($value, $phids, $exclude);
+            if ($cc_emails) {
+              $add_cc = $cc_emails;
             }
             break;
           case 'headers':
@@ -659,27 +652,81 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
     return base64_encode($base);
   }
 
-  private function getDeliverableEmailsFromHandles(
-    array $phids,
-    array $handles,
-    array $exclude) {
-    assert_instances_of($handles, 'PhabricatorObjectHandle');
+  private function loadEmailAndNameDataFromPHIDs(array &$phids) {
+    $users = array();
+    $mlsts = array();
+    // first iteration - group by types to do data fetches
+    foreach ($phids as $phid => $type) {
+      switch ($type) {
+        case PhabricatorPHIDConstants::PHID_TYPE_USER:
+          $users[] = $phid;
+          break;
+        case PhabricatorPHIDConstants::PHID_TYPE_MLST:
+          $mlsts[] = $phid;
+          break;
+      }
+    }
+    $user_emails = array();
+    if ($users) {
+      $user_emails = id(new PhabricatorUserEmail())->loadAllWhere(
+        'userPHID IN (%Ls) AND isPrimary = 1', $users);
+      $users = id(new PhabricatorUser())->loadAllWhere(
+        'phid IN (%Ls)', $users);
+      $user_emails = mpull($user_emails, null, 'getUserPHID');
+      $users = mpull($users, null, 'getPHID');
+    }
+    if ($mlsts) {
+      $mlsts = id(new PhabricatorMetaMTAMailingList())->loadAllWhere(
+        'phid IN (%Ls)', $mlsts);
+      $mlsts = mpull($mlsts, null, 'getPHID');
+    }
 
-    $emails = array();
-    foreach ($phids as $phid) {
-      if ($handles[$phid]->isDisabled()) {
-        continue;
+    // second iteration - create entries for each phid
+    $default = PhabricatorEnv::getEnvConfig('metamta.default-address');
+    foreach ($phids as $phid => &$value) {
+      $name = '';
+      $email = $default;
+      $is_mailable = false;
+      switch ($value) {
+        case PhabricatorPHIDConstants::PHID_TYPE_USER:
+          $user = $users[$phid];
+          if ($user) {
+            $name = $user->getFullName();
+            $is_mailable = !$user->getIsDisabled()
+                        && !$user->getIsSystemAgent();
           }
-      if (!$handles[$phid]->isComplete()) {
-        continue;
+          $email = $user_emails[$phid] ?
+                   $user_emails[$phid]->getAddress() :
+                   $default;
+          break;
+        case PhabricatorPHIDConstants::PHID_TYPE_MLST:
+          $mlst = $mlsts[$phid];
+          if ($mlst) {
+            $name = $mlst->getName();
+            $email = $mlst->getEmail();
+            $is_mailable = true;
           }
+          break;
+      }
+      $value = array(
+        'name' => $name,
+        'email' => $email,
+        'mailable' => $is_mailable,
+      );
+    }
+  }
+
+  private function filterSendable($value, $phids, $exclude) {
+    $result = array();
+    foreach ($value as $phid) {
       if (isset($exclude[$phid])) {
         continue;
       }
-      $emails[$phid] = $handles[$phid]->getEmail();
+      if (isset($phids[$phid]) && $phids[$phid]['mailable']) {
+        $result[$phid] = $phids[$phid]['email'];
       }
-
-    return $emails;
+    }
+    return $result;
   }
 
   public static function shouldMultiplexAllMail() {

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

@@ -7,9 +7,12 @@
 
 
 phutil_require_module('phabricator', 'applications/metamta/storage/base');
+phutil_require_module('phabricator', 'applications/metamta/storage/mailinglist');
+phutil_require_module('phabricator', 'applications/people/storage/email');
 phutil_require_module('phabricator', 'applications/people/storage/preferences');
 phutil_require_module('phabricator', 'applications/people/storage/user');
-phutil_require_module('phabricator', 'applications/phid/handle/data');
+phutil_require_module('phabricator', 'applications/phid/constants');
+phutil_require_module('phabricator', 'applications/phid/utils');
 phutil_require_module('phabricator', 'infrastructure/daemon/workers/storage/task');
 phutil_require_module('phabricator', 'infrastructure/env');
 

src/applications/people/storage/user/PhabricatorUser.php

@@ -581,6 +581,10 @@ EOBODY;
     return self::getDefaultProfileImageURI();
   }
 
+  public function getFullName() {
+    return $this->getUsername().' ('.$this->getRealName().')';
+  }
+
   public static function loadOneWithEmailAddress($address) {
     $email = id(new PhabricatorUserEmail())->loadOneWhere(
       'address = %s',

src/applications/phid/handle/PhabricatorObjectHandle.php

@@ -88,15 +88,6 @@ final class PhabricatorObjectHandle {
     return $this->type;
   }
 
-  public function setEmail($email) {
-    $this->email = $email;
-    return $this;
-  }
-
-  public function getEmail() {
-    return $this->email;
-  }
-
   public function setImageURI($uri) {
     $this->imageURI = $uri;
     return $this;

src/applications/phid/handle/data/PhabricatorObjectHandleData.php

@@ -149,16 +149,6 @@ final class PhabricatorObjectHandleData {
             $images = mpull($images, 'getBestURI', 'getPHID');
           }
 
-          // TODO: This probably should not be part of Handles anymore, only
-          // MetaMTA actually uses it.
-          $emails = id(new PhabricatorUserEmail())->loadAllWhere(
-            'userPHID IN (%Ls) AND isPrimary = 1',
-            $phids);
-          $emails = mpull($emails, 'getAddress', 'getUserPHID');
-
-          $statuses = id(new PhabricatorUserStatus())->loadCurrentStatuses(
-            $phids);
-
           foreach ($phids as $phid) {
             $handle = new PhabricatorObjectHandle();
             $handle->setPHID($phid);
@@ -169,7 +159,6 @@ final class PhabricatorObjectHandleData {
               $user = $users[$phid];
               $handle->setName($user->getUsername());
               $handle->setURI('/p/'.$user->getUsername().'/');
-              $handle->setEmail(idx($emails, $phid));
               $handle->setFullName(
                 $user->getUsername().' ('.$user->getRealName().')');
               $handle->setAlternateID($user->getID());
@@ -177,8 +166,7 @@ final class PhabricatorObjectHandleData {
               if (isset($statuses[$phid])) {
                 $handle->setStatus($statuses[$phid]->getTextStatus());
               }
-              $handle->setDisabled($user->getIsDisabled() ||
-                                   $user->getIsSystemAgent());
+              $handle->setDisabled($user->getIsDisabled());
 
               $img_uri = idx($images, $user->getProfileImagePHID());
               if ($img_uri) {
@@ -208,7 +196,6 @@ final class PhabricatorObjectHandleData {
               $handle->setName('Unknown Mailing List');
             } else {
               $list = $lists[$phid];
-              $handle->setEmail($list->getEmail());
               $handle->setName($list->getName());
               $handle->setURI($list->getURI());
               $handle->setFullName($list->getName());

src/applications/phid/handle/data/__init__.php

@@ -11,7 +11,6 @@ phutil_require_module('arcanist', 'differential/constants/revisionstatus');
 phutil_require_module('phabricator', 'applications/files/storage/file');
 phutil_require_module('phabricator', 'applications/maniphest/constants/owner');
 phutil_require_module('phabricator', 'applications/maniphest/constants/status');
-phutil_require_module('phabricator', 'applications/people/storage/email');
 phutil_require_module('phabricator', 'applications/people/storage/user');
 phutil_require_module('phabricator', 'applications/people/storage/userstatus');
 phutil_require_module('phabricator', 'applications/phid/constants');