From a19f49632fd80ee6cb03746aa80c71ce23475262 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Sat, 8 Mar 2014 12:03:15 -0800
Subject: [PATCH] Remove willWriteRevision/didWriteRevision hooks

Summary:
Ref T2222. DifferentialRevisionEditor has no remaining callsites, but it has a bit of functionality which still needs to be ported forward. I'm going to rip it apart piece by piece.

This removes the willWriteRevision/didWriteRevision hooks. They are completely encapsulated by transactions now, except for a unique piece of branch/task logic, which I migrated forward.

Test Plan:
  - Lots of `grep`.
  - Created a new revision on branch `T25`, saw it associate with the task.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8454
---
 .../editor/DifferentialRevisionEditor.php     | 40 -------------------
 .../editor/DifferentialTransactionEditor.php  | 36 ++++++++++++++++-
 .../DifferentialBranchFieldSpecification.php  | 26 ------------
 .../DifferentialCCsFieldSpecification.php     |  4 --
 ...fferentialEditPolicyFieldSpecification.php |  4 --
 .../DifferentialFieldSpecification.php        | 32 ---------------
 ...DifferentialFreeformFieldSpecification.php | 28 -------------
 ...fferentialJIRAIssuesFieldSpecification.php | 25 ------------
 ...entialManiphestTasksFieldSpecification.php | 28 -------------
 ...fferentialRepositoryFieldSpecification.php |  4 --
 ...ifferentialReviewersFieldSpecification.php |  4 --
 .../DifferentialSummaryFieldSpecification.php |  4 --
 ...DifferentialTestPlanFieldSpecification.php |  4 --
 .../DifferentialTitleFieldSpecification.php   |  4 --
 ...fferentialViewPolicyFieldSpecification.php |  4 --
 15 files changed, 35 insertions(+), 212 deletions(-)

diff --git a/src/applications/differential/editor/DifferentialRevisionEditor.php b/src/applications/differential/editor/DifferentialRevisionEditor.php
index e128699645..5c156d04be 100644
--- a/src/applications/differential/editor/DifferentialRevisionEditor.php
+++ b/src/applications/differential/editor/DifferentialRevisionEditor.php
@@ -161,8 +161,6 @@ final class DifferentialRevisionEditor extends PhabricatorEditor {
 
     $revision->loadRelationships();
 
-    $this->willWriteRevision();
-
     if ($this->reviewers === null) {
       $this->reviewers = $revision->getReviewers();
     }
@@ -437,8 +435,6 @@ final class DifferentialRevisionEditor extends PhabricatorEditor {
         break;
     }
 
-    $this->didWriteRevision();
-
     $event_data = array(
       'revision_id'          => $revision->getID(),
       'revision_phid'        => $revision->getPHID(),
@@ -690,42 +686,6 @@ final class DifferentialRevisionEditor extends PhabricatorEditor {
     }
   }
 
-  private function willWriteRevision() {
-    foreach ($this->auxiliaryFields as $aux_field) {
-      $aux_field->willWriteRevision($this);
-    }
-
-    $this->dispatchEvent(
-      PhabricatorEventType::TYPE_DIFFERENTIAL_WILLEDITREVISION);
-  }
-
-  private function didWriteRevision() {
-    foreach ($this->auxiliaryFields as $aux_field) {
-      $aux_field->didWriteRevision($this);
-    }
-
-    $this->dispatchEvent(
-      PhabricatorEventType::TYPE_DIFFERENTIAL_DIDEDITREVISION);
-  }
-
-  private function dispatchEvent($type) {
-    $event = new PhabricatorEvent(
-      $type,
-      array(
-        'revision'      => $this->revision,
-        'new'           => $this->isCreate,
-      ));
-
-    $event->setUser($this->getActor());
-
-    $request = $this->getAphrontRequestForEventDispatch();
-    if ($request) {
-      $event->setAphrontRequest($request);
-    }
-
-    PhutilEventEngine::dispatchEvent($event);
-  }
-
   /**
    * Update the table which links Differential revisions to paths they affect,
    * so Diffusion can efficiently find pending revisions for a given file.
diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php
index cefe319ff9..0e094255db 100644
--- a/src/applications/differential/editor/DifferentialTransactionEditor.php
+++ b/src/applications/differential/editor/DifferentialTransactionEditor.php
@@ -226,7 +226,9 @@ final class DifferentialTransactionEditor
     $actor = $this->getActor();
     $actor_phid = $actor->getPHID();
     $type_edge = PhabricatorTransactions::TYPE_EDGE;
+
     $edge_reviewer = PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER;
+    $edge_ref_task = PhabricatorEdgeConfig::TYPE_DREV_HAS_RELATED_TASK;
 
     $results = parent::expandTransaction($object, $xaction);
     switch ($xaction->getTransactionType()) {
@@ -266,6 +268,38 @@ final class DifferentialTransactionEditor
             ->setIgnoreOnNoEffect(true)
             ->setNewValue(array('+' => $edits));
         }
+
+        // When a revision is updated and the diff comes from a branch named
+        // "T123" or similar, automatically associate the commit with the
+        // task that the branch names.
+
+        $maniphest = 'PhabricatorApplicationManiphest';
+        if (PhabricatorApplication::isClassInstalled($maniphest)) {
+          $diff = $this->loadDiff($xaction->getNewValue());
+          if ($diff) {
+            $branch = $diff->getBranch();
+
+            // No "$", to allow for branches like T123_demo.
+            $match = null;
+            if (preg_match('/^T(\d+)/i', $branch, $match)) {
+              $task_id = $match[1];
+              $tasks = id(new ManiphestTaskQuery())
+                ->setViewer($this->getActor())
+                ->withIDs(array($task_id))
+                ->execute();
+              if ($tasks) {
+                $task = head($tasks);
+                $task_phid = $task->getPHID();
+
+                $results[] = id(new DifferentialTransaction())
+                  ->setTransactionType($type_edge)
+                  ->setMetadataValue('edge:type', $edge_ref_task)
+                  ->setIgnoreOnNoEffect(true)
+                  ->setNewValue(array('+' => array($task_phid => $task_phid)));
+              }
+            }
+          }
+        }
         break;
 
       case PhabricatorTransactions::TYPE_COMMENT:
@@ -905,7 +939,7 @@ final class DifferentialTransactionEditor
     switch ($strongest->getTransactionType()) {
       case DifferentialTransaction::TYPE_UPDATE:
         $count = new PhutilNumber($object->getLineCount());
-        $action = pht('%s, %s line(s)', $action, $count);
+        $action = pht('%s, %d line(s)', $action, $count);
         break;
     }
 
diff --git a/src/applications/differential/field/specification/DifferentialBranchFieldSpecification.php b/src/applications/differential/field/specification/DifferentialBranchFieldSpecification.php
index d5fbcf2b49..af427fe34c 100644
--- a/src/applications/differential/field/specification/DifferentialBranchFieldSpecification.php
+++ b/src/applications/differential/field/specification/DifferentialBranchFieldSpecification.php
@@ -43,32 +43,6 @@ final class DifferentialBranchFieldSpecification
     return null;
   }
 
-  public function didWriteRevision(DifferentialRevisionEditor $editor) {
-    $maniphest = 'PhabricatorApplicationManiphest';
-    if (!PhabricatorApplication::isClassInstalled($maniphest)) {
-      return;
-    }
-
-    $branch = $this->getDiff()->getBranch();
-    $match = null;
-    if (preg_match('/^T(\d+)/i', $branch, $match)) { // No $ to allow T123_demo.
-      list(, $task_id) = $match;
-      $task = id(new ManiphestTaskQuery())
-        ->setViewer($editor->requireActor())
-        ->withIDs(array($task_id))
-        ->executeOne();
-      if ($task) {
-        id(new PhabricatorEdgeEditor())
-          ->setActor($this->getUser())
-          ->addEdge(
-            $this->getRevision()->getPHID(),
-            PhabricatorEdgeConfig::TYPE_DREV_HAS_RELATED_TASK,
-            $task->getPHID())
-          ->save();
-      }
-    }
-  }
-
   public function getCommitMessageTips() {
     return array(
       'Name branch "T123" to attach the diff to a task.',
diff --git a/src/applications/differential/field/specification/DifferentialCCsFieldSpecification.php b/src/applications/differential/field/specification/DifferentialCCsFieldSpecification.php
index 92c3106345..ed2e7e9a2f 100644
--- a/src/applications/differential/field/specification/DifferentialCCsFieldSpecification.php
+++ b/src/applications/differential/field/specification/DifferentialCCsFieldSpecification.php
@@ -60,10 +60,6 @@ final class DifferentialCCsFieldSpecification
       ->setValue($cc_map);
   }
 
-  public function willWriteRevision(DifferentialRevisionEditor $editor) {
-    $editor->setCCPHIDs($this->ccs);
-  }
-
   public function shouldAppearOnCommitMessage() {
     return true;
   }
diff --git a/src/applications/differential/field/specification/DifferentialEditPolicyFieldSpecification.php b/src/applications/differential/field/specification/DifferentialEditPolicyFieldSpecification.php
index 62963e3d67..50cf1beecf 100644
--- a/src/applications/differential/field/specification/DifferentialEditPolicyFieldSpecification.php
+++ b/src/applications/differential/field/specification/DifferentialEditPolicyFieldSpecification.php
@@ -35,8 +35,4 @@ final class DifferentialEditPolicyFieldSpecification
       ->setName('editPolicy');
   }
 
-  public function willWriteRevision(DifferentialRevisionEditor $editor) {
-    $this->getRevision()->setEditPolicy($this->value);
-  }
-
 }
diff --git a/src/applications/differential/field/specification/DifferentialFieldSpecification.php b/src/applications/differential/field/specification/DifferentialFieldSpecification.php
index a7d45ab2f9..7ef49e7aed 100644
--- a/src/applications/differential/field/specification/DifferentialFieldSpecification.php
+++ b/src/applications/differential/field/specification/DifferentialFieldSpecification.php
@@ -192,38 +192,6 @@ abstract class DifferentialFieldSpecification {
     return false;
   }
 
-  /**
-   * Hook for applying revision changes via the editor. Normally, you should
-   * not implement this, but a number of builtin fields use the revision object
-   * itself as storage. If you need to do something similar for whatever reason,
-   * this method gives you an opportunity to interact with the editor or
-   * revision before changes are saved (for example, you can write the field's
-   * value into some property of the revision).
-   *
-   * @param DifferentialRevisionEditor  Active editor which is applying changes
-   *                                    to the revision.
-   * @return void
-   * @task edit
-   */
-  public function willWriteRevision(DifferentialRevisionEditor $editor) {
-    return;
-  }
-
-  /**
-   * Hook after an edit operation has completed. This allows you to update
-   * link tables or do other write operations which should happen after the
-   * revision is saved. Normally you don't need to implement this.
-   *
-   *
-   * @param DifferentialRevisionEditor  Active editor which has just applied
-   *                                    changes to the revision.
-   * @return void
-   * @task edit
-   */
-  public function didWriteRevision(DifferentialRevisionEditor $editor) {
-    return;
-  }
-
 
 /* -(  Extending the Revision View Interface  )------------------------------ */
 
diff --git a/src/applications/differential/field/specification/DifferentialFreeformFieldSpecification.php b/src/applications/differential/field/specification/DifferentialFreeformFieldSpecification.php
index a00dc7b9fb..694fbf1b62 100644
--- a/src/applications/differential/field/specification/DifferentialFreeformFieldSpecification.php
+++ b/src/applications/differential/field/specification/DifferentialFreeformFieldSpecification.php
@@ -63,34 +63,6 @@ abstract class DifferentialFreeformFieldSpecification
     return $result;
   }
 
-  public function didWriteRevision(DifferentialRevisionEditor $editor) {
-    $message = $this->renderValueForCommitMessage(false);
-
-    $tasks = $this->findMentionedTasks($message);
-    if ($tasks) {
-      $tasks = id(new ManiphestTaskQuery())
-        ->setViewer($editor->getActor())
-        ->withIDs(array_keys($tasks))
-        ->execute();
-      $this->saveFieldEdges(
-        $editor->getRevision(),
-        PhabricatorEdgeConfig::TYPE_DREV_HAS_RELATED_TASK,
-        mpull($tasks, 'getPHID'));
-    }
-
-    $dependents = $this->findDependentRevisions($message);
-    if ($dependents) {
-      $dependents = id(new DifferentialRevisionQuery())
-        ->setViewer($editor->getActor())
-        ->withIDs($dependents)
-        ->execute();
-      $this->saveFieldEdges(
-        $editor->getRevision(),
-        PhabricatorEdgeConfig::TYPE_DREV_DEPENDS_ON_DREV,
-        mpull($dependents, 'getPHID'));
-    }
-  }
-
   private function saveFieldEdges(
     DifferentialRevision $revision,
     $edge_type,
diff --git a/src/applications/differential/field/specification/DifferentialJIRAIssuesFieldSpecification.php b/src/applications/differential/field/specification/DifferentialJIRAIssuesFieldSpecification.php
index 8f9394d069..27a2db98e6 100644
--- a/src/applications/differential/field/specification/DifferentialJIRAIssuesFieldSpecification.php
+++ b/src/applications/differential/field/specification/DifferentialJIRAIssuesFieldSpecification.php
@@ -167,29 +167,4 @@ final class DifferentialJIRAIssuesFieldSpecification
     return $xobjs;
   }
 
-  public function didWriteRevision(DifferentialRevisionEditor $editor) {
-    $revision = $editor->getRevision();
-    $revision_phid = $revision->getPHID();
-
-    $edge_type = PhabricatorEdgeConfig::TYPE_PHOB_HAS_JIRAISSUE;
-    $edge_dsts = mpull($this->loadDoorkeeperExternalObjects(), 'getPHID');
-
-    $edges = PhabricatorEdgeQuery::loadDestinationPHIDs(
-      $revision_phid,
-      $edge_type);
-
-    $editor = id(new PhabricatorEdgeEditor())
-      ->setActor($this->getUser());
-
-    foreach (array_diff($edges, $edge_dsts) as $rem_edge) {
-      $editor->removeEdge($revision_phid, $edge_type, $rem_edge);
-    }
-
-    foreach (array_diff($edge_dsts, $edges) as $add_edge) {
-      $editor->addEdge($revision_phid, $edge_type, $add_edge);
-    }
-
-    $editor->save();
-  }
-
 }
diff --git a/src/applications/differential/field/specification/DifferentialManiphestTasksFieldSpecification.php b/src/applications/differential/field/specification/DifferentialManiphestTasksFieldSpecification.php
index dd851590ab..9cdb7b72a3 100644
--- a/src/applications/differential/field/specification/DifferentialManiphestTasksFieldSpecification.php
+++ b/src/applications/differential/field/specification/DifferentialManiphestTasksFieldSpecification.php
@@ -43,34 +43,6 @@ final class DifferentialManiphestTasksFieldSpecification
       PhabricatorEdgeConfig::TYPE_DREV_HAS_RELATED_TASK);
   }
 
-  /**
-   * Attach the revision to the task(s) and the task(s) to the revision.
-   *
-   * @return void
-   */
-  public function didWriteRevision(DifferentialRevisionEditor $editor) {
-    $revision = $editor->getRevision();
-    $revision_phid = $revision->getPHID();
-    $edge_type = PhabricatorEdgeConfig::TYPE_DREV_HAS_RELATED_TASK;
-
-    $old_phids = $this->oldManiphestTasks;
-    $add_phids = $this->maniphestTasks;
-    $rem_phids = array_diff($old_phids, $add_phids);
-
-    $edge_editor = id(new PhabricatorEdgeEditor())
-      ->setActor($this->getUser());
-
-    foreach ($add_phids as $phid) {
-      $edge_editor->addEdge($revision_phid, $edge_type, $phid);
-    }
-
-    foreach ($rem_phids as $phid) {
-      $edge_editor->removeEdge($revision_phid, $edge_type, $phid);
-    }
-
-    $edge_editor->save();
-  }
-
   protected function didSetRevision() {
     $this->maniphestTasks = $this->getManiphestTaskPHIDs();
     $this->oldManiphestTasks = $this->maniphestTasks;
diff --git a/src/applications/differential/field/specification/DifferentialRepositoryFieldSpecification.php b/src/applications/differential/field/specification/DifferentialRepositoryFieldSpecification.php
index 36adc65d1f..6cd56fa681 100644
--- a/src/applications/differential/field/specification/DifferentialRepositoryFieldSpecification.php
+++ b/src/applications/differential/field/specification/DifferentialRepositoryFieldSpecification.php
@@ -41,8 +41,4 @@ final class DifferentialRepositoryFieldSpecification
       ->setValue($value);
   }
 
-  public function willWriteRevision(DifferentialRevisionEditor $editor) {
-    $this->getRevision()->setRepositoryPHID($this->value);
-  }
-
 }
diff --git a/src/applications/differential/field/specification/DifferentialReviewersFieldSpecification.php b/src/applications/differential/field/specification/DifferentialReviewersFieldSpecification.php
index 35c47afef7..d6965b7900 100644
--- a/src/applications/differential/field/specification/DifferentialReviewersFieldSpecification.php
+++ b/src/applications/differential/field/specification/DifferentialReviewersFieldSpecification.php
@@ -100,10 +100,6 @@ final class DifferentialReviewersFieldSpecification
       ->setError($this->error);
   }
 
-  public function willWriteRevision(DifferentialRevisionEditor $editor) {
-    $editor->setReviewers($this->reviewers);
-  }
-
   public function shouldAppearOnCommitMessage() {
     return true;
   }
diff --git a/src/applications/differential/field/specification/DifferentialSummaryFieldSpecification.php b/src/applications/differential/field/specification/DifferentialSummaryFieldSpecification.php
index cb1515d6f7..19f6004a07 100644
--- a/src/applications/differential/field/specification/DifferentialSummaryFieldSpecification.php
+++ b/src/applications/differential/field/specification/DifferentialSummaryFieldSpecification.php
@@ -38,10 +38,6 @@ final class DifferentialSummaryFieldSpecification
     return true;
   }
 
-  public function willWriteRevision(DifferentialRevisionEditor $editor) {
-    $this->getRevision()->setSummary($this->summary);
-  }
-
   public function shouldAppearOnCommitMessage() {
     return true;
   }
diff --git a/src/applications/differential/field/specification/DifferentialTestPlanFieldSpecification.php b/src/applications/differential/field/specification/DifferentialTestPlanFieldSpecification.php
index 74dcb42014..7928902843 100644
--- a/src/applications/differential/field/specification/DifferentialTestPlanFieldSpecification.php
+++ b/src/applications/differential/field/specification/DifferentialTestPlanFieldSpecification.php
@@ -42,10 +42,6 @@ final class DifferentialTestPlanFieldSpecification
     return true;
   }
 
-  public function willWriteRevision(DifferentialRevisionEditor $editor) {
-    $this->getRevision()->setTestPlan($this->plan);
-  }
-
   public function validateField() {
     if ($this->isRequired()) {
       if (!strlen($this->plan)) {
diff --git a/src/applications/differential/field/specification/DifferentialTitleFieldSpecification.php b/src/applications/differential/field/specification/DifferentialTitleFieldSpecification.php
index 59fb375e32..b05a210b05 100644
--- a/src/applications/differential/field/specification/DifferentialTitleFieldSpecification.php
+++ b/src/applications/differential/field/specification/DifferentialTitleFieldSpecification.php
@@ -33,10 +33,6 @@ final class DifferentialTitleFieldSpecification
     return true;
   }
 
-  public function willWriteRevision(DifferentialRevisionEditor $editor) {
-    $this->getRevision()->setTitle($this->title);
-  }
-
   public function validateField() {
     if (!strlen($this->title)) {
       $this->error = 'Required';
diff --git a/src/applications/differential/field/specification/DifferentialViewPolicyFieldSpecification.php b/src/applications/differential/field/specification/DifferentialViewPolicyFieldSpecification.php
index df8568c33e..f8205afdfe 100644
--- a/src/applications/differential/field/specification/DifferentialViewPolicyFieldSpecification.php
+++ b/src/applications/differential/field/specification/DifferentialViewPolicyFieldSpecification.php
@@ -35,8 +35,4 @@ final class DifferentialViewPolicyFieldSpecification
       ->setName('viewPolicy');
   }
 
-  public function willWriteRevision(DifferentialRevisionEditor $editor) {
-    $this->getRevision()->setViewPolicy($this->value);
-  }
-
 }