Implement PhabricatorApplicationTransactionInterface on ManiphestTask

Summary:
Ref T5245. A very long time ago I had this terrible idea that we'd let objects react to edges being added and insert transactions in response.

This turned out to be a clearly bad idea very quickly, for like 15 different reasons. A big issue is that it inverts the responsibilities of editors. It's also just clumsy and messy.

We now have `PhabricatorApplicationTransactionInterface` instead, which mostly provides a cleaner way to deal with this.

Implement `PhabricatorApplicationTransactionInterface`, implicitly moving all the attach actions (task/task, task/revision, task/commit, task/mock) to proper edge transactions.

The cost of this is that the inverse edges don't write transactions -- if you attach an object to another object, only the object you were acting on posts a transaction record. This is sort of buggy anyway already. I'll fix this in the next diff.

Test Plan: Attached tasks, revisions and mocks to a task, then detached them.

Reviewers: chad, btrahan, joshuaspence

Reviewed By: joshuaspence

Subscribers: epriestley

Maniphest Tasks: T5245

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

src/__phutil_library_map__.php

@@ -926,7 +926,6 @@ phutil_register_library_map(array(
     'ManiphestCustomFieldStorage' => 'applications/maniphest/storage/ManiphestCustomFieldStorage.php',
     'ManiphestCustomFieldStringIndex' => 'applications/maniphest/storage/ManiphestCustomFieldStringIndex.php',
     'ManiphestDAO' => 'applications/maniphest/storage/ManiphestDAO.php',
-    'ManiphestEdgeEventListener' => 'applications/maniphest/event/ManiphestEdgeEventListener.php',
     'ManiphestExcelDefaultFormat' => 'applications/maniphest/export/ManiphestExcelDefaultFormat.php',
     'ManiphestExcelFormat' => 'applications/maniphest/export/ManiphestExcelFormat.php',
     'ManiphestExportController' => 'applications/maniphest/controller/ManiphestExportController.php',
@@ -3674,7 +3673,6 @@ phutil_register_library_map(array(
     'ManiphestCustomFieldStorage' => 'PhabricatorCustomFieldStorage',
     'ManiphestCustomFieldStringIndex' => 'PhabricatorCustomFieldStringIndexStorage',
     'ManiphestDAO' => 'PhabricatorLiskDAO',
-    'ManiphestEdgeEventListener' => 'PhabricatorEventListener',
     'ManiphestExcelDefaultFormat' => 'ManiphestExcelFormat',
     'ManiphestExportController' => 'ManiphestController',
     'ManiphestHovercardEventListener' => 'PhabricatorEventListener',
@@ -3697,6 +3695,7 @@ phutil_register_library_map(array(
       5 => 'PhrequentTrackableInterface',
       6 => 'PhabricatorCustomFieldInterface',
       7 => 'PhabricatorDestructableInterface',
+      8 => 'PhabricatorApplicationTransactionInterface',
     ),
     'ManiphestTaskDescriptionPreviewController' => 'ManiphestController',
     'ManiphestTaskDetailController' => 'ManiphestController',

src/applications/maniphest/editor/ManiphestTransactionEditor.php

@@ -9,6 +9,7 @@ final class ManiphestTransactionEditor
     $types = parent::getTransactionTypes();
 
     $types[] = PhabricatorTransactions::TYPE_COMMENT;
+    $types[] = PhabricatorTransactions::TYPE_EDGE;
     $types[] = ManiphestTransaction::TYPE_PRIORITY;
     $types[] = ManiphestTransaction::TYPE_STATUS;
     $types[] = ManiphestTransaction::TYPE_TITLE;
@@ -16,7 +17,6 @@ final class ManiphestTransactionEditor
     $types[] = ManiphestTransaction::TYPE_OWNER;
     $types[] = ManiphestTransaction::TYPE_CCS;
     $types[] = ManiphestTransaction::TYPE_PROJECTS;
-    $types[] = ManiphestTransaction::TYPE_EDGE;
     $types[] = ManiphestTransaction::TYPE_SUBPRIORITY;
     $types[] = ManiphestTransaction::TYPE_PROJECT_COLUMN;
     $types[] = ManiphestTransaction::TYPE_UNBLOCK;
@@ -57,7 +57,6 @@ final class ManiphestTransactionEditor
         return array_values(array_unique($object->getCCPHIDs()));
       case ManiphestTransaction::TYPE_PROJECTS:
         return array_values(array_unique($object->getProjectPHIDs()));
-      case ManiphestTransaction::TYPE_EDGE:
       case ManiphestTransaction::TYPE_PROJECT_COLUMN:
         // These are pre-populated.
         return $xaction->getOldValue();
@@ -82,7 +81,6 @@ final class ManiphestTransactionEditor
       case ManiphestTransaction::TYPE_STATUS:
       case ManiphestTransaction::TYPE_TITLE:
       case ManiphestTransaction::TYPE_DESCRIPTION:
-      case ManiphestTransaction::TYPE_EDGE:
       case ManiphestTransaction::TYPE_SUBPRIORITY:
       case ManiphestTransaction::TYPE_PROJECT_COLUMN:
       case ManiphestTransaction::TYPE_UNBLOCK:
@@ -155,10 +153,6 @@ final class ManiphestTransactionEditor
         $object->setProjectPHIDs($xaction->getNewValue());
         ManiphestTaskProject::updateTaskProjects($object);
         return $object;
-      case ManiphestTransaction::TYPE_EDGE:
-        // These are a weird, funky mess and are already being applied by the
-        // time we reach this.
-        return;
       case ManiphestTransaction::TYPE_SUBPRIORITY:
         $data = $xaction->getNewValue();
         $new_sub = $this->getNextSubpriority(

src/applications/maniphest/event/ManiphestEdgeEventListener.php

@@ -1,137 +0,0 @@
-<?php
-
-/**
- * Listener for Maniphest Task edge events. When some workflow causes task
- * edges to be added or removed, we consider the edge edit authoritative but
- * duplicate the information into a @{class:ManiphestTansaction} for display.
- */
-final class ManiphestEdgeEventListener extends PhabricatorEventListener {
-
-  private $edges = array();
-  private $tasks = array();
-
-  public function register() {
-    $this->listen(PhabricatorEventType::TYPE_EDGE_WILLEDITEDGES);
-    $this->listen(PhabricatorEventType::TYPE_EDGE_DIDEDITEDGES);
-  }
-
-  public function handleEvent(PhutilEvent $event) {
-    switch ($event->getType()) {
-      case PhabricatorEventType::TYPE_EDGE_WILLEDITEDGES:
-        return $this->handleWillEditEvent($event);
-      case PhabricatorEventType::TYPE_EDGE_DIDEDITEDGES:
-        return $this->handleDidEditEvent($event);
-    }
-  }
-
-  private function handleWillEditEvent(PhutilEvent $event) {
-    // NOTE: Everything is namespaced by `id` so that we aren't left in an
-    // inconsistent state if an edit fails to complete (e.g., something throws)
-    // or an edit happens inside another edit.
-
-    $id = $event->getValue('id');
-
-    $edges = $this->loadAllEdges($event);
-    $tasks = array();
-    if ($edges) {
-      // TODO: T603 This should probably all get nuked. Until then, this isn't
-      // realllllly a policy issue since callers are (or should be) doing
-      // policy checks anyway.
-      $tasks = id(new ManiphestTask())->loadAllWhere(
-        'phid IN (%Ls)',
-        array_keys($edges));
-      $tasks = mpull($tasks, null, 'getPHID');
-    }
-
-    $this->edges[$id] = $edges;
-    $this->tasks[$id] = $tasks;
-  }
-
-  private function handleDidEditEvent(PhutilEvent $event) {
-    $id = $event->getValue('id');
-
-    $old_edges = $this->edges[$id];
-    $tasks = $this->tasks[$id];
-
-    unset($this->edges[$id]);
-    unset($this->tasks[$id]);
-
-    $content_source = PhabricatorContentSource::newForSource(
-      PhabricatorContentSource::SOURCE_LEGACY,
-      array());
-
-    $new_edges = $this->loadAllEdges($event);
-    $editor = id(new ManiphestTransactionEditor())
-      ->setActor($event->getUser())
-      ->setContentSource($content_source)
-      ->setContinueOnNoEffect(true)
-      ->setContinueOnMissingFields(true);
-
-    foreach ($tasks as $phid => $task) {
-      $xactions = array();
-
-      $old = $old_edges[$phid];
-      $new = $new_edges[$phid];
-
-      $types = array_keys($old + $new);
-      foreach ($types as $type) {
-        $old_type = idx($old, $type, array());
-        $new_type = idx($new, $type, array());
-
-        if ($old_type === $new_type) {
-          continue;
-        }
-
-        $xactions[] = id(new ManiphestTransaction())
-          ->setTransactionType(ManiphestTransaction::TYPE_EDGE)
-          ->setOldValue($old_type)
-          ->setNewValue($new_type)
-          ->setMetadataValue('edge:type', $type);
-      }
-
-      if ($xactions) {
-        $editor->applyTransactions($task, $xactions);
-      }
-    }
-  }
-
-  private function filterEdgesBySourceType(array $edges, $type) {
-    foreach ($edges as $key => $edge) {
-      if ($edge['src_type'] !== $type) {
-        unset($edges[$key]);
-      }
-    }
-    return $edges;
-  }
-
-  private function loadAllEdges(PhutilEvent $event) {
-    $add_edges = $event->getValue('add');
-    $rem_edges = $event->getValue('rem');
-
-    $type_task = ManiphestPHIDTypeTask::TYPECONST;
-
-    $all_edges = array_merge($add_edges, $rem_edges);
-    $all_edges = $this->filterEdgesBySourceType($all_edges, $type_task);
-
-    if (!$all_edges) {
-      return;
-    }
-
-    $all_tasks = array();
-    $all_types = array();
-    foreach ($all_edges as $edge) {
-      $all_tasks[$edge['src']] = true;
-      $all_types[$edge['type']] = true;
-    }
-
-    $all_tasks = array_keys($all_tasks);
-    $all_types = array_keys($all_types);
-
-    return id(new PhabricatorEdgeQuery())
-      ->withSourcePHIDs($all_tasks)
-      ->withEdgeTypes($all_types)
-      ->needEdgeData(true)
-      ->execute();
-  }
-
-}

src/applications/maniphest/storage/ManiphestTask.php

@@ -8,7 +8,8 @@ final class ManiphestTask extends ManiphestDAO
     PhabricatorFlaggableInterface,
     PhrequentTrackableInterface,
     PhabricatorCustomFieldInterface,
-    PhabricatorDestructableInterface {
+    PhabricatorDestructableInterface,
+    PhabricatorApplicationTransactionInterface {
 
   const MARKUP_FIELD_DESCRIPTION = 'markup:desc';
 
@@ -303,4 +304,20 @@ final class ManiphestTask extends ManiphestDAO
     $this->saveTransaction();
   }
 
+
+/* -(  PhabricatorApplicationTransactionInterface  )------------------------- */
+
+
+  public function getApplicationTransactionEditor() {
+    return new ManiphestTransactionEditor();
+  }
+
+  public function getApplicationTransactionObject() {
+    return $this;
+  }
+
+  public function getApplicationTransactionTemplate() {
+    return new ManiphestTransaction();
+  }
+
 }

src/applications/search/controller/PhabricatorSearchAttachController.php

@@ -57,45 +57,32 @@ final class PhabricatorSearchAttachController
       $phids = array_values($phids);
 
       if ($edge_type) {
-        $do_txn = $object instanceof PhabricatorApplicationTransactionInterface;
+        if (!$object instanceof PhabricatorApplicationTransactionInterface) {
+          throw new Exception(
+            pht(
+              'Expected object ("%s") to implement interface "%s".',
+              get_class($object),
+              'PhabricatorApplicationTransactionInterface'));
+        }
+
         $old_phids = PhabricatorEdgeQuery::loadDestinationPHIDs(
           $this->phid,
           $edge_type);
         $add_phids = $phids;
         $rem_phids = array_diff($old_phids, $add_phids);
 
-        if ($do_txn) {
+        $txn_editor = $object->getApplicationTransactionEditor()
-
+          ->setActor($user)
-          $txn_editor = $object->getApplicationTransactionEditor()
+          ->setContentSourceFromRequest($request);
-            ->setActor($user)
+        $txn_template = $object->getApplicationTransactionTemplate()
-            ->setContentSourceFromRequest($request);
+          ->setTransactionType(PhabricatorTransactions::TYPE_EDGE)
-          $txn_template = $object->getApplicationTransactionTemplate()
+          ->setMetadataValue('edge:type', $edge_type)
-            ->setTransactionType(PhabricatorTransactions::TYPE_EDGE)
+          ->setNewValue(array(
-            ->setMetadataValue('edge:type', $edge_type)
+            '+' => array_fuse($add_phids),
-            ->setNewValue(array(
+            '-' => array_fuse($rem_phids)));
-              '+' => array_fuse($add_phids),
+        $txn_editor->applyTransactions(
-              '-' => array_fuse($rem_phids)));
+          $object->getApplicationTransactionObject(),
-          $txn_editor->applyTransactions(
+          array($txn_template));
-            $object->getApplicationTransactionObject(),
-            array($txn_template));
-
-        } else {
-
-          $editor = id(new PhabricatorEdgeEditor());
-          $editor->setActor($user);
-          foreach ($add_phids as $phid) {
-            $editor->addEdge($this->phid, $edge_type, $phid);
-          }
-          foreach ($rem_phids as $phid) {
-            $editor->removeEdge($this->phid, $edge_type, $phid);
-          }
-
-          try {
-            $editor->save();
-          } catch (PhabricatorEdgeCycleException $ex) {
-            $this->raiseGraphCycleException($ex);
-          }
-        }
 
         return id(new AphrontReloadResponse())->setURI($handle->getURI());
       } else {

src/infrastructure/events/PhabricatorEventEngine.php

@@ -25,7 +25,6 @@ final class PhabricatorEventEngine {
 
     // Add builtin listeners.
     $listeners[] = new DarkConsoleEventPluginAPI();
-    $listeners[] = new ManiphestEdgeEventListener();
 
     // Add application listeners.
     $applications = PhabricatorApplication::getAllInstalledApplications();