Don't try to implicitly subscribe users who are already subscribed

Summary:
Fixes T2587. Specifically:

  - Don't try to implicitly subscribe the actor if they're already subscribed.
  - Since there are like 5 things that need to interact with subscribers, just load them once upfront for Subscribable objects.

Test Plan: Made a comment on a mock I was CC'd on without an error.

Reviewers: vrana

Reviewed By: vrana

CC: aran

Maniphest Tasks: T2587

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

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -16,6 +16,7 @@ abstract class PhabricatorApplicationTransactionEditor
   private $mentionedPHIDs;
   private $continueOnNoEffect;
   private $parentMessageID;
+  private $subscribers;
 
   private $isPreview;
 
@@ -95,13 +96,7 @@ abstract class PhabricatorApplicationTransactionEditor
     PhabricatorApplicationTransaction $xaction) {
     switch ($xaction->getTransactionType()) {
       case PhabricatorTransactions::TYPE_SUBSCRIBERS:
-        if ($object->getPHID()) {
+        return array_values($this->subscribers);
-          $old_phids = PhabricatorSubscribersQuery::loadSubscribersForPHID(
-            $object->getPHID());
-        } else {
-          $old_phids = array();
-        }
-        return array_values($old_phids);
       case PhabricatorTransactions::TYPE_VIEW_POLICY:
         return $object->getViewPolicy();
       case PhabricatorTransactions::TYPE_EDIT_POLICY:
@@ -222,6 +217,15 @@ abstract class PhabricatorApplicationTransactionEditor
 
     $actor = $this->requireActor();
 
+    if ($object->getPHID() &&
+        ($object instanceof PhabricatorSubscribableInterface)) {
+      $subs = PhabricatorSubscribersQuery::loadSubscribersForPHID(
+        $object->getPHID());
+      $this->subscribers = array_fuse($subs);
+    } else {
+      $this->subscribers = array();
+    }
+
     $xactions = $this->applyImplicitCC($object, $xactions);
 
     $mention_xaction = $this->buildMentionTransaction($object, $xactions);
@@ -422,9 +426,7 @@ abstract class PhabricatorApplicationTransactionEditor
       // Don't try to subscribe already-subscribed mentions: we want to generate
       // a dialog about an action having no effect if the user explicitly adds
       // existing CCs, but not if they merely mention existing subscribers.
-      $current = PhabricatorSubscribersQuery::loadSubscribersForPHID(
+      $phids = array_diff($phids, $this->subscribers);
-        $object->getPHID());
-      $phids = array_diff($phids, $current);
     }
 
     if (!$phids) {
@@ -678,6 +680,11 @@ abstract class PhabricatorApplicationTransactionEditor
     }
 
     if ($object->getPHID()) {
+      if (isset($this->subscribers[$actor_phid])) {
+        // If the user is already subscribed, don't implicitly CC them.
+        return $xactions;
+      }
+
       $unsub = PhabricatorEdgeQuery::loadDestinationPHIDs(
         $object->getPHID(),
         PhabricatorEdgeConfig::TYPE_OBJECT_HAS_UNSUBSCRIBER);
@@ -840,8 +847,7 @@ abstract class PhabricatorApplicationTransactionEditor
    */
   protected function getMailCC(PhabricatorLiskDAO $object) {
     if ($object instanceof PhabricatorSubscribableInterface) {
-      $phid = $object->getPHID();
+      return $this->subscribers;
-      return PhabricatorSubscribersQuery::loadSubscribersForPHID($phid);
     }
     throw new Exception("Capability not supported.");
   }