From ace1feb70220a589386d0fa7fd9163998e13f631 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 17 Jul 2014 15:40:52 -0700 Subject: [PATCH] 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 | 3 +- .../editor/ManiphestTransactionEditor.php | 8 +- .../event/ManiphestEdgeEventListener.php | 137 ------------------ .../maniphest/storage/ManiphestTask.php | 19 ++- .../PhabricatorSearchAttachController.php | 53 +++---- .../events/PhabricatorEventEngine.php | 1 - 6 files changed, 40 insertions(+), 181 deletions(-) delete mode 100644 src/applications/maniphest/event/ManiphestEdgeEventListener.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 39eb658120..2fbc1f25fd 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php index a8c69cba24..3bca85686d 100644 --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/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( diff --git a/src/applications/maniphest/event/ManiphestEdgeEventListener.php b/src/applications/maniphest/event/ManiphestEdgeEventListener.php deleted file mode 100644 index 8e2552d110..0000000000 --- a/src/applications/maniphest/event/ManiphestEdgeEventListener.php +++ /dev/null @@ -1,137 +0,0 @@ -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(); - } - -} diff --git a/src/applications/maniphest/storage/ManiphestTask.php b/src/applications/maniphest/storage/ManiphestTask.php index 2a88ba9864..9f9d9ca26e 100644 --- a/src/applications/maniphest/storage/ManiphestTask.php +++ b/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(); + } + } diff --git a/src/applications/search/controller/PhabricatorSearchAttachController.php b/src/applications/search/controller/PhabricatorSearchAttachController.php index a799aa7b67..1459e404e1 100644 --- a/src/applications/search/controller/PhabricatorSearchAttachController.php +++ b/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) - ->setContentSourceFromRequest($request); - $txn_template = $object->getApplicationTransactionTemplate() - ->setTransactionType(PhabricatorTransactions::TYPE_EDGE) - ->setMetadataValue('edge:type', $edge_type) - ->setNewValue(array( - '+' => array_fuse($add_phids), - '-' => array_fuse($rem_phids))); - $txn_editor->applyTransactions( - $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); - } - } + $txn_editor = $object->getApplicationTransactionEditor() + ->setActor($user) + ->setContentSourceFromRequest($request); + $txn_template = $object->getApplicationTransactionTemplate() + ->setTransactionType(PhabricatorTransactions::TYPE_EDGE) + ->setMetadataValue('edge:type', $edge_type) + ->setNewValue(array( + '+' => array_fuse($add_phids), + '-' => array_fuse($rem_phids))); + $txn_editor->applyTransactions( + $object->getApplicationTransactionObject(), + array($txn_template)); return id(new AphrontReloadResponse())->setURI($handle->getURI()); } else { diff --git a/src/infrastructure/events/PhabricatorEventEngine.php b/src/infrastructure/events/PhabricatorEventEngine.php index 9959efe664..67807317c6 100644 --- a/src/infrastructure/events/PhabricatorEventEngine.php +++ b/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();