From c2a9a8079f22c6bfd82c2659df9c4fd009e94ef1 Mon Sep 17 00:00:00 2001 From: Nick Harper Date: Thu, 17 May 2012 21:46:45 -0700 Subject: [PATCH] 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 --- .../storage/mail/PhabricatorMetaMTAMail.php | 117 ++++++++++++------ .../metamta/storage/mail/__init__.php | 5 +- .../people/storage/user/PhabricatorUser.php | 4 + .../phid/handle/PhabricatorObjectHandle.php | 9 -- .../data/PhabricatorObjectHandleData.php | 15 +-- .../phid/handle/data/__init__.php | 1 - 6 files changed, 91 insertions(+), 60 deletions(-) diff --git a/src/applications/metamta/storage/mail/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/mail/PhabricatorMetaMTAMail.php index 5c67c3cf3f..ae845745ac 100644 --- a/src/applications/metamta/storage/mail/PhabricatorMetaMTAMail.php +++ b/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; - } - if (!$handles[$phid]->isComplete()) { - 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(); + } + $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() { diff --git a/src/applications/metamta/storage/mail/__init__.php b/src/applications/metamta/storage/mail/__init__.php index 7df205fc8c..2470c3d0ac 100644 --- a/src/applications/metamta/storage/mail/__init__.php +++ b/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'); diff --git a/src/applications/people/storage/user/PhabricatorUser.php b/src/applications/people/storage/user/PhabricatorUser.php index 0ce125d13e..639b984a33 100644 --- a/src/applications/people/storage/user/PhabricatorUser.php +++ b/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', diff --git a/src/applications/phid/handle/PhabricatorObjectHandle.php b/src/applications/phid/handle/PhabricatorObjectHandle.php index 56fa21e2d4..fb28c92bc9 100644 --- a/src/applications/phid/handle/PhabricatorObjectHandle.php +++ b/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; diff --git a/src/applications/phid/handle/data/PhabricatorObjectHandleData.php b/src/applications/phid/handle/data/PhabricatorObjectHandleData.php index 8fa322a56d..ae87809767 100644 --- a/src/applications/phid/handle/data/PhabricatorObjectHandleData.php +++ b/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()); diff --git a/src/applications/phid/handle/data/__init__.php b/src/applications/phid/handle/data/__init__.php index d2622bba45..309d3dc001 100644 --- a/src/applications/phid/handle/data/__init__.php +++ b/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');