diff --git a/resources/sql/patches/migrate-maniphest-dependencies.php b/resources/sql/patches/migrate-maniphest-dependencies.php new file mode 100644 index 0000000000..cfa93d7e5c --- /dev/null +++ b/resources/sql/patches/migrate-maniphest-dependencies.php @@ -0,0 +1,42 @@ +getID(); + echo "Task {$id}: "; + + $deps = $task->getAttachedPHIDs(PhabricatorPHIDConstants::PHID_TYPE_TASK); + if (!$deps) { + echo "-\n"; + continue; + } + + $editor = new PhabricatorEdgeEditor(); + $editor->setSuppressEvents(true); + foreach ($deps as $dep) { + $editor->addEdge( + $task->getPHID(), + PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK, + $dep); + } + $editor->save(); + echo "OKAY\n"; +} + +echo "Done.\n"; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 4206c02e6e..f36d58484b 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -815,7 +815,6 @@ phutil_register_library_map(array( 'PhabricatorOAuthServerTokenController' => 'applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php', 'PhabricatorOAuthUnlinkController' => 'applications/auth/controller/PhabricatorOAuthUnlinkController.php', 'PhabricatorObjectAttachmentEditor' => 'applications/search/editor/PhabricatorObjectAttachmentEditor.php', - 'PhabricatorObjectGraph' => 'applications/phid/PhabricatorObjectGraph.php', 'PhabricatorObjectHandle' => 'applications/phid/PhabricatorObjectHandle.php', 'PhabricatorObjectHandleConstants' => 'applications/phid/handle/const/PhabricatorObjectHandleConstants.php', 'PhabricatorObjectHandleData' => 'applications/phid/handle/PhabricatorObjectHandleData.php', @@ -1824,7 +1823,6 @@ phutil_register_library_map(array( 'PhabricatorOAuthServerTestController' => 'PhabricatorOAuthServerController', 'PhabricatorOAuthServerTokenController' => 'PhabricatorAuthController', 'PhabricatorOAuthUnlinkController' => 'PhabricatorAuthController', - 'PhabricatorObjectGraph' => 'AbstractDirectedGraph', 'PhabricatorObjectHandleStatus' => 'PhabricatorObjectHandleConstants', 'PhabricatorOffsetPagedQuery' => 'PhabricatorQuery', 'PhabricatorOwnersController' => 'PhabricatorController', diff --git a/src/applications/maniphest/controller/ManiphestTaskDetailController.php b/src/applications/maniphest/controller/ManiphestTaskDetailController.php index 311e537f13..ec0e10003d 100644 --- a/src/applications/maniphest/controller/ManiphestTaskDetailController.php +++ b/src/applications/maniphest/controller/ManiphestTaskDetailController.php @@ -51,11 +51,28 @@ final class ManiphestTaskDetailController extends ManiphestController { 'taskID = %d ORDER BY id ASC', $task->getID()); - $commit_phids = PhabricatorEdgeQuery::loadDestinationPHIDs( - $task->getPHID(), - PhabricatorEdgeConfig::TYPE_TASK_HAS_COMMIT); + $e_commit = PhabricatorEdgeConfig::TYPE_TASK_HAS_COMMIT; + $e_dep_on = PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK; + $e_dep_by = PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK; + + $phid = $task->getPHID(); + + $query = id(new PhabricatorEdgeQuery()) + ->withSourcePHIDs(array($phid)) + ->withEdgeTypes( + array( + $e_commit, + $e_dep_on, + $e_dep_by, + )); + $edges = $query->execute(); + + $commit_phids = array_keys($edges[$phid][$e_commit]); + $dep_on_tasks = array_keys($edges[$phid][$e_dep_on]); + $dep_by_tasks = array_keys($edges[$phid][$e_dep_by]); + + $phids = array_fill_keys($query->getDestinationPHIDs(), true); - $phids = array_fill_keys($commit_phids, true); foreach ($transactions as $transaction) { foreach ($transaction->extractPHIDs() as $phid) { $phids[$phid] = true; @@ -150,33 +167,25 @@ final class ManiphestTaskDetailController extends ManiphestController { } } - $dtasks = idx($attached, PhabricatorPHIDConstants::PHID_TYPE_TASK); - if ($dtasks) { - $dtask_links = array(); - foreach ($dtasks as $dtask => $info) { - $dtask_links[] = $handles[$dtask]->renderLink(); - } - $dtask_links = implode('
', $dtask_links); - $dict['Depends On'] = $dtask_links; + if ($dep_by_tasks) { + $dict['Dependent Tasks'] = $this->renderHandleList( + array_select_keys($handles, $dep_by_tasks)); + } + + if ($dep_on_tasks) { + $dict['Depends On'] = $this->renderHandleList( + array_select_keys($handles, $dep_on_tasks)); } $revs = idx($attached, PhabricatorPHIDConstants::PHID_TYPE_DREV); if ($revs) { - $rev_links = array(); - foreach ($revs as $rev => $info) { - $rev_links[] = $handles[$rev]->renderLink(); - } - $rev_links = implode('
', $rev_links); - $dict['Revisions'] = $rev_links; + $dict['Revisions'] = $this->renderHandleList( + array_select_keys($handles, $revs)); } if ($commit_phids) { - $commit_links = array(); - foreach ($commit_phids as $phid) { - $commit_links[] = $handles[$phid]->renderLink(); - } - $commit_links = implode('
', $commit_links); - $dict['Commits'] = $commit_links; + $dict['Commits'] = $this->renderHandleList( + array_select_keys($handles, $commit_phids)); } $file_infos = idx($attached, PhabricatorPHIDConstants::PHID_TYPE_FILE); @@ -534,4 +543,12 @@ final class ManiphestTaskDetailController extends ManiphestController { )); } + private function renderHandleList(array $handles) { + $links = array(); + foreach ($handles as $handle) { + $links[] = $handle->renderLink(); + } + return implode('
', $links); + } + } diff --git a/src/applications/maniphest/storage/ManiphestTask.php b/src/applications/maniphest/storage/ManiphestTask.php index 97a269d4f7..32d9c618b3 100644 --- a/src/applications/maniphest/storage/ManiphestTask.php +++ b/src/applications/maniphest/storage/ManiphestTask.php @@ -60,6 +60,18 @@ final class ManiphestTask extends ManiphestDAO ) + parent::getConfiguration(); } + public function loadDependsOnTaskPHIDs() { + return PhabricatorEdgeQuery::loadDestinationPHIDs( + $this->getPHID(), + PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK); + } + + public function loadDependedOnByTaskPHIDs() { + return PhabricatorEdgeQuery::loadDestinationPHIDs( + $this->getPHID(), + PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK); + } + public function getAttachedPHIDs($type) { return array_keys(idx($this->attached, $type, array())); } diff --git a/src/applications/maniphest/view/ManiphestTransactionDetailView.php b/src/applications/maniphest/view/ManiphestTransactionDetailView.php index ccb4c7444f..ddaf8092a5 100644 --- a/src/applications/maniphest/view/ManiphestTransactionDetailView.php +++ b/src/applications/maniphest/view/ManiphestTransactionDetailView.php @@ -657,8 +657,16 @@ final class ManiphestTransactionDetailView extends ManiphestView { } } + + /** + * @task strings + */ private function getEdgeEmailTitle($type, $count) { switch ($type) { + case PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK: + return pht('DEPENDS ON %d TASK(S)', $count); + case PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK: + return pht('DEPENDENT %d TASK(s)', $count); case PhabricatorEdgeConfig::TYPE_TASK_HAS_COMMIT: return pht('ATTACHED %d COMMIT(S)', $count); default: @@ -672,6 +680,10 @@ final class ManiphestTransactionDetailView extends ManiphestView { */ private function getEdgeAddVerb($type) { switch ($type) { + case PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK: + return pht('Added Dependency'); + case PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK: + return pht('Added Dependent Task'); case PhabricatorEdgeConfig::TYPE_TASK_HAS_COMMIT: return pht('Added Commit'); default: @@ -685,6 +697,10 @@ final class ManiphestTransactionDetailView extends ManiphestView { */ private function getEdgeRemVerb($type) { switch ($type) { + case PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK: + return pht('Removed Dependency'); + case PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK: + return pht('Removed Dependent Task'); case PhabricatorEdgeConfig::TYPE_TASK_HAS_COMMIT: return pht('Removed Commit'); default: @@ -698,6 +714,10 @@ final class ManiphestTransactionDetailView extends ManiphestView { */ private function getEdgeEditVerb($type) { switch ($type) { + case PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK: + return pht('Changed Dependencies'); + case PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK: + return pht('Changed Dependent Tasks'); case PhabricatorEdgeConfig::TYPE_TASK_HAS_COMMIT: return pht('Changed Commits'); default: @@ -713,6 +733,10 @@ final class ManiphestTransactionDetailView extends ManiphestView { $list = $this->renderHandles(array_keys($add)); switch ($type) { + case PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK: + return pht('added %d dependencie(s): %s', $add, $list); + case PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK: + return pht('added %d dependent task(s): %s', $add, $list); case PhabricatorEdgeConfig::TYPE_TASK_HAS_COMMIT: return pht('added %d commit(s): %s', $add, $list); default: @@ -728,6 +752,10 @@ final class ManiphestTransactionDetailView extends ManiphestView { $list = $this->renderHandles(array_keys($rem)); switch ($type) { + case PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK: + return pht('removed %d dependencie(s): %s', $rem, $list); + case PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK: + return pht('removed %d dependent task(s): %s', $rem, $list); case PhabricatorEdgeConfig::TYPE_TASK_HAS_COMMIT: return pht('removed %d commit(s): %s', $rem, $list); default: @@ -744,6 +772,22 @@ final class ManiphestTransactionDetailView extends ManiphestView { $rem_list = $this->renderHandles(array_keys($rem)); switch ($type) { + case PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK: + return pht( + 'changed %d dependencie(s), added %d: %s; removed %d: %s', + count($add) + count($rem), + $add, + $add_list, + $rem, + $rem_list); + case PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK: + return pht( + 'changed %d dependent task(s), added %d: %s; removed %d: %s', + count($add) + count($rem), + $add, + $add_list, + $rem, + $rem_list); case PhabricatorEdgeConfig::TYPE_TASK_HAS_COMMIT: return pht( 'changed %d commit(s), added %d: %s; removed %d: %s', diff --git a/src/applications/phid/PhabricatorObjectGraph.php b/src/applications/phid/PhabricatorObjectGraph.php deleted file mode 100644 index 4eedc42321..0000000000 --- a/src/applications/phid/PhabricatorObjectGraph.php +++ /dev/null @@ -1,49 +0,0 @@ -edgeType = $edge_type; - return $this; - } - - protected function loadEdges(array $nodes) { - if (!$this->edgeType) { - throw new Exception("Set edge type before loading graph!"); - } - - $handle_data = new PhabricatorObjectHandleData($nodes); - $objects = $handle_data->loadObjects(); - - $result = array(); - foreach ($nodes as $phid) { - $object = idx($objects, $phid); - if ($object) { - $result[$phid] = $object->getAttachedPHIDs($this->edgeType); - } else { - $result[$phid] = array(); - } - } - - return $result; - } - -} diff --git a/src/applications/search/controller/PhabricatorSearchAttachController.php b/src/applications/search/controller/PhabricatorSearchAttachController.php index 670e2103ff..ff9d1815b1 100644 --- a/src/applications/search/controller/PhabricatorSearchAttachController.php +++ b/src/applications/search/controller/PhabricatorSearchAttachController.php @@ -56,74 +56,76 @@ final class PhabricatorSearchAttachController return new Aphront404Response(); } + $edge_type = null; + switch ($this->action) { + case self::ACTION_EDGE: + case self::ACTION_DEPENDENCIES: + $edge_type = $this->getEdgeType($object_type, $attach_type); + break; + } + if ($request->isFormPost()) { $phids = explode(';', $request->getStr('phids')); $phids = array_filter($phids); $phids = array_values($phids); - switch ($this->action) { - case self::ACTION_MERGE: - return $this->performMerge($object, $handle, $phids); + if ($edge_type) { + $old_phids = PhabricatorEdgeQuery::loadDestinationPHIDs( + $this->phid, + $edge_type); + $add_phids = $phids; + $rem_phids = array_diff($old_phids, $add_phids); - case self::ACTION_EDGE: - $edge_type = $this->getEdgeType($object_type, $attach_type); + $editor = id(new PhabricatorEdgeEditor()); + $editor->setUser($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); + } - $old_phids = PhabricatorEdgeQuery::loadDestinationPHIDs( - $this->phid, - $edge_type); - $add_phids = $phids; - $rem_phids = array_diff($old_phids, $add_phids); - $editor = id(new PhabricatorEdgeEditor()); - $editor->setUser($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()); - case self::ACTION_DEPENDENCIES: - case self::ACTION_ATTACH: - $two_way = true; - if ($this->action == self::ACTION_DEPENDENCIES) { - $two_way = false; - $this->detectGraphCycles( - $object, + return id(new AphrontReloadResponse())->setURI($handle->getURI()); + } else { + switch ($this->action) { + case self::ACTION_MERGE: + return $this->performMerge($object, $handle, $phids); + case self::ACTION_ATTACH: + $editor = new PhabricatorObjectAttachmentEditor( + $object_type, + $object); + $editor->setUser($this->getRequest()->getUser()); + $editor->attachObjects( $attach_type, - $phids); - } + $phids, + $two_way = false); - $editor = new PhabricatorObjectAttachmentEditor( - $object_type, - $object); - $editor->setUser($this->getRequest()->getUser()); - $editor->attachObjects( - $attach_type, - $phids, - $two_way); - - return id(new AphrontReloadResponse())->setURI($handle->getURI()); - default: - throw new Exception("Unsupported attach action."); + return id(new AphrontReloadResponse())->setURI($handle->getURI()); + default: + throw new Exception("Unsupported attach action."); + } } } else { - switch ($this->action) { - case self::ACTION_ATTACH: - case self::ACTION_DEPENDENCIES: - $phids = $object->getAttachedPHIDs($attach_type); - break; - case self::ACTION_EDGE: - $edge_type = $this->getEdgeType($object_type, $attach_type); - - $phids = PhabricatorEdgeQuery::loadDestinationPHIDs( - $this->phid, - $edge_type); - break; - default: - $phids = array(); - break; + if ($edge_type) { + $phids = PhabricatorEdgeQuery::loadDestinationPHIDs( + $this->phid, + $edge_type); + } else { + switch ($this->action) { + case self::ACTION_ATTACH: + case self::ACTION_DEPENDENCIES: + $phids = $object->getAttachedPHIDs($attach_type); + break; + default: + $phids = array(); + break; + } } } @@ -269,41 +271,6 @@ final class PhabricatorSearchAttachController ); } - private function detectGraphCycles( - $object, - $attach_type, - array $phids) { - - // Detect graph cycles. - $graph = new PhabricatorObjectGraph(); - $graph->setEdgeType($attach_type); - $graph->addNodes(array( - $object->getPHID() => $phids, - )); - $graph->loadGraph(); - - foreach ($phids as $phid) { - $cycle = $graph->detectCycles($phid); - if (!$cycle) { - continue; - } - - // TODO: Improve this behavior so it's not all-or-nothing? - - $handles = id(new PhabricatorObjectHandleData($cycle)) - ->loadHandles(); - $names = array(); - foreach ($cycle as $cycle_phid) { - $names[] = $handles[$cycle_phid]->getFullName(); - } - $names = implode(" \xE2\x86\x92 ", $names); - $which = $handles[$phid]->getFullName(); - throw new Exception( - "You can not create a dependency on '{$which}' because it ". - "would create a circular dependency: {$names}."); - } - } - private function getEdgeType($src_type, $dst_type) { $t_cmit = PhabricatorPHIDConstants::PHID_TYPE_CMIT; $t_task = PhabricatorPHIDConstants::PHID_TYPE_TASK; @@ -314,14 +281,30 @@ final class PhabricatorSearchAttachController ), $t_task => array( $t_cmit => PhabricatorEdgeConfig::TYPE_TASK_HAS_COMMIT, + $t_task => PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK, ), ); if (empty($map[$src_type][$dst_type])) { - throw new Exception("Unknown edge type!"); + return null; } return $map[$src_type][$dst_type]; } + private function raiseGraphCycleException(PhabricatorEdgeCycleException $ex) { + $cycle = $ex->getCycle(); + + $handles = id(new PhabricatorObjectHandleData($cycle)) + ->loadHandles(); + $names = array(); + foreach ($cycle as $cycle_phid) { + $names[] = $handles[$cycle_phid]->getFullName(); + } + $names = implode(" \xE2\x86\x92 ", $names); + throw new Exception( + "You can not create that dependency, because it would create a ". + "circular dependency: {$names}."); + } + } diff --git a/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php b/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php index 627aa2277a..77a5ba78f1 100644 --- a/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php +++ b/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php @@ -21,15 +21,20 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants { const TABLE_NAME_EDGE = 'edge'; const TABLE_NAME_EDGEDATA = 'edgedata'; - const TYPE_TASK_HAS_COMMIT = 1; - const TYPE_COMMIT_HAS_TASK = 2; + const TYPE_TASK_HAS_COMMIT = 1; + const TYPE_COMMIT_HAS_TASK = 2; + const TYPE_TASK_DEPENDS_ON_TASK = 3; + const TYPE_TASK_DEPENDED_ON_BY_TASK = 4; - const TYPE_TEST_NO_CYCLE = 9000; + const TYPE_TEST_NO_CYCLE = 9000; public static function getInverse($edge_type) { static $map = array( self::TYPE_TASK_HAS_COMMIT => self::TYPE_COMMIT_HAS_TASK, self::TYPE_COMMIT_HAS_TASK => self::TYPE_TASK_HAS_COMMIT, + + self::TYPE_TASK_DEPENDS_ON_TASK => self::TYPE_TASK_DEPENDED_ON_BY_TASK, + self::TYPE_TASK_DEPENDED_ON_BY_TASK => self::TYPE_TASK_DEPENDS_ON_TASK, ); return idx($map, $edge_type); @@ -37,7 +42,8 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants { public static function shouldPreventCycles($edge_type) { static $map = array( - self::TYPE_TEST_NO_CYCLE => true, + self::TYPE_TEST_NO_CYCLE => true, + self::TYPE_TASK_DEPENDS_ON_TASK => true, ); return isset($map[$edge_type]); } diff --git a/src/infrastructure/edges/editor/PhabricatorEdgeEditor.php b/src/infrastructure/edges/editor/PhabricatorEdgeEditor.php index f4f7c811c3..aa3400a3a4 100644 --- a/src/infrastructure/edges/editor/PhabricatorEdgeEditor.php +++ b/src/infrastructure/edges/editor/PhabricatorEdgeEditor.php @@ -41,6 +41,7 @@ final class PhabricatorEdgeEditor { private $remEdges = array(); private $openTransactions = array(); private $user; + private $suppressEvents; public function setUser(PhabricatorUser $user) { $this->user = $user; @@ -370,7 +371,26 @@ final class PhabricatorEdgeEditor { } } + /** + * Suppress edge edit events. This prevents listeners from making updates in + * response to edits, and is primarily useful when performing migrations. You + * should not normally need to use it. + * + * @param bool True to supress events related to edits. + * @return this + * @task internal + */ + public function setSuppressEvents($suppress) { + $this->suppressEvents = $suppress; + return $this; + } + + private function sendEvent($edit_id, $event_type) { + if ($this->suppressEvents) { + return; + } + $event = new PhabricatorEvent( $event_type, array( @@ -436,4 +456,5 @@ final class PhabricatorEdgeEditor { } } + } diff --git a/src/infrastructure/internationalization/PhabricatorBaseEnglishTranslation.php b/src/infrastructure/internationalization/PhabricatorBaseEnglishTranslation.php index d044dc1c29..17ec42e321 100644 --- a/src/infrastructure/internationalization/PhabricatorBaseEnglishTranslation.php +++ b/src/infrastructure/internationalization/PhabricatorBaseEnglishTranslation.php @@ -66,6 +66,43 @@ abstract class PhabricatorBaseEnglishTranslation 'ATTACHED COMMITS', 'ATTACHED COMMIT', ), + + 'added %d dependencie(s): %s' => array( + 'added dependencies: %2$s', + 'added dependency: %2$s', + ), + + 'added %d dependent task(s): %s' => array( + 'added dependent tasks: %2$s', + 'added dependent task: %2$s', + ), + + 'removed %d dependencie(s): %s' => array( + 'removed dependencies: %2$s', + 'removed dependency: %2$s', + ), + + 'removed %d dependent task(s): %s' => array( + 'removed dependent tasks: %2$s', + 'removed dependent task: %2$s', + ), + + 'changed %d dependencie(s), added %d: %s; removed %d: %s' => + 'changed dependencies, added: %3$s; removed: %5$s', + + 'changed %d dependent task(s), added %d: %s; removed %d: %s', + 'changed dependent tasks, added: %3$s; removed: %5$s', + + 'DEPENDENT %d TASK(s)' => array( + 'DEPENDENT TASKS', + 'DEPENDENT TASK', + ), + + 'DEPENDS ON %d TASK(S)' => array( + 'DEPENDS ON TASKS', + 'DEPENDS ON TASK', + ), + ); } diff --git a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php index e4138845b8..4311367798 100644 --- a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php +++ b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php @@ -915,6 +915,10 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList { 'type' => 'sql', 'name' => $this->getPatchPath('maniphestxcache.sql'), ), + 'migrate-maniphest-dependencies.php' => array( + 'type' => 'php', + 'name' => $this->getPatchPath('migrate-maniphest-dependencies.php'), + ), ); }