From d179d0150c4ce5e980e6d19eca21f2aaaf38f700 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 20 Mar 2017 14:02:08 -0700 Subject: [PATCH] Remove obsolete "relationships" code from Differential Summary: Ref T10967. There have been two different ways to load reviewers for a while: `needReviewerStatus()` and `needRelationships()`. The `needRelationships()` stuff was a false start along time ago that didn't really go anywhere. I believe the idea was that we might want to load several different types of edges (subscribers, reviewers, etc) on lots of different types of objects. However, all that stuff pretty much ended up modularizing so that main `Query` classes did not need to know about it, so `needRelationships()` never got generalized or went anywhere. A handful of things still use it, but get rid of them: they should either `needReviewerStatus()` to get reviewer info, or the ~3 callsites that care about subscribers can just load them directly. Test Plan: - Grepped for removed methods (`needRelationships()`, `getReviewers()`, `getCCPHIDs()`, etc). - Browsed Diffusion, Differential. - Called `differential.query`. It's possible I missed some stuff, but it should mostly show up as super obvious fatals ("call needReviewerStatus() before getReviewerStatus()!"). Reviewers: chad Reviewed By: chad Maniphest Tasks: T10967 Differential Revision: https://secure.phabricator.com/D17518 --- ...ifferentialGetRevisionConduitAPIMethod.php | 3 +- .../DifferentialQueryConduitAPIMethod.php | 14 +++- .../DifferentialRevisionViewController.php | 10 +-- ...alDoorkeeperRevisionFeedStoryPublisher.php | 9 +-- .../HeraldDifferentialRevisionAdapter.php | 1 - .../query/DifferentialRevisionQuery.php | 53 --------------- .../DifferentialRevisionSearchEngine.php | 1 - .../storage/DifferentialRevision.php | 65 ++----------------- .../view/DifferentialRevisionListView.php | 8 +-- .../controller/DiffusionBrowseController.php | 2 +- ...sionCommitRevisionReviewersHeraldField.php | 2 +- ...mitContentRevisionReviewersHeraldField.php | 4 +- .../diffusion/herald/HeraldCommitAdapter.php | 1 - .../herald/HeraldPreCommitContentAdapter.php | 2 +- 14 files changed, 35 insertions(+), 140 deletions(-) diff --git a/src/applications/differential/conduit/DifferentialGetRevisionConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialGetRevisionConduitAPIMethod.php index a158d86d9f..3b4d9a6a63 100644 --- a/src/applications/differential/conduit/DifferentialGetRevisionConduitAPIMethod.php +++ b/src/applications/differential/conduit/DifferentialGetRevisionConduitAPIMethod.php @@ -42,7 +42,6 @@ final class DifferentialGetRevisionConduitAPIMethod $revision = id(new DifferentialRevisionQuery()) ->withIDs(array($revision_id)) ->setViewer($request->getUser()) - ->needRelationships(true) ->needReviewerStatus(true) ->executeOne(); @@ -50,7 +49,7 @@ final class DifferentialGetRevisionConduitAPIMethod throw new ConduitException('ERR_BAD_REVISION'); } - $reviewer_phids = array_values($revision->getReviewers()); + $reviewer_phids = $revision->getReviewerPHIDs(); $diffs = id(new DifferentialDiffQuery()) ->setViewer($request->getUser()) diff --git a/src/applications/differential/conduit/DifferentialQueryConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialQueryConduitAPIMethod.php index 720361367a..c9f90d0877 100644 --- a/src/applications/differential/conduit/DifferentialQueryConduitAPIMethod.php +++ b/src/applications/differential/conduit/DifferentialQueryConduitAPIMethod.php @@ -182,7 +182,7 @@ final class DifferentialQueryConduitAPIMethod $query->withBranches($branches); } - $query->needRelationships(true); + $query->needReviewerStatus(true); $query->needCommitPHIDs(true); $query->needDiffIDs(true); $query->needActiveDiffs(true); @@ -194,6 +194,14 @@ final class DifferentialQueryConduitAPIMethod $request->getUser(), $revisions); + if ($revisions) { + $ccs = id(new PhabricatorSubscribersQuery()) + ->withObjectPHIDs(mpull($revisions, 'getPHID')) + ->execute(); + } else { + $ccs = array(); + } + $results = array(); foreach ($revisions as $revision) { $diff = $revision->getActiveDiff(); @@ -224,8 +232,8 @@ final class DifferentialQueryConduitAPIMethod 'activeDiffPHID' => $diff->getPHID(), 'diffs' => $revision->getDiffIDs(), 'commits' => $revision->getCommitPHIDs(), - 'reviewers' => array_values($revision->getReviewers()), - 'ccs' => array_values($revision->getCCPHIDs()), + 'reviewers' => $revision->getReviewerPHIDs(), + 'ccs' => idx($ccs, $phid, array()), 'hashes' => $revision->getHashes(), 'auxiliary' => idx($field_data, $phid, array()), 'repositoryPHID' => $diff->getRepositoryPHID(), diff --git a/src/applications/differential/controller/DifferentialRevisionViewController.php b/src/applications/differential/controller/DifferentialRevisionViewController.php index d31da8fefc..9b87d1163d 100644 --- a/src/applications/differential/controller/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/DifferentialRevisionViewController.php @@ -17,7 +17,6 @@ final class DifferentialRevisionViewController extends DifferentialController { $revision = id(new DifferentialRevisionQuery()) ->withIDs(array($this->revisionID)) ->setViewer($viewer) - ->needRelationships(true) ->needReviewerStatus(true) ->needReviewerAuthority(true) ->executeOne(); @@ -103,9 +102,12 @@ final class DifferentialRevisionViewController extends DifferentialController { $this->loadDiffProperties($diffs); $props = $target_manual->getDiffProperties(); + $subscriber_phids = PhabricatorSubscribersQuery::loadSubscribersForPHID( + $revision->getPHID()); + $object_phids = array_merge( - $revision->getReviewers(), - $revision->getCCPHIDs(), + $revision->getReviewerPHIDs(), + $subscriber_phids, $revision->loadCommitPHIDs(), array( $revision->getAuthorPHID(), @@ -782,7 +784,7 @@ final class DifferentialRevisionViewController extends DifferentialController { ->setLimit(10) ->needFlags(true) ->needDrafts(true) - ->needRelationships(true); + ->needReviewerStatus(true); foreach ($path_map as $path => $path_id) { $query->withPath($repository->getID(), $path_id); diff --git a/src/applications/differential/doorkeeper/DifferentialDoorkeeperRevisionFeedStoryPublisher.php b/src/applications/differential/doorkeeper/DifferentialDoorkeeperRevisionFeedStoryPublisher.php index 23ad165ca9..135e9b560d 100644 --- a/src/applications/differential/doorkeeper/DifferentialDoorkeeperRevisionFeedStoryPublisher.php +++ b/src/applications/differential/doorkeeper/DifferentialDoorkeeperRevisionFeedStoryPublisher.php @@ -26,7 +26,7 @@ final class DifferentialDoorkeeperRevisionFeedStoryPublisher return id(new DifferentialRevisionQuery()) ->setViewer($this->getViewer()) ->withIDs(array($object->getID())) - ->needRelationships(true) + ->needReviewerStatus(true) ->executeOne(); } @@ -37,7 +37,7 @@ final class DifferentialDoorkeeperRevisionFeedStoryPublisher public function getActiveUserPHIDs($object) { $status = $object->getStatus(); if ($status == ArcanistDifferentialRevisionStatus::NEEDS_REVIEW) { - return $object->getReviewers(); + return $object->getReviewerPHIDs(); } else { return array(); } @@ -48,12 +48,13 @@ final class DifferentialDoorkeeperRevisionFeedStoryPublisher if ($status == ArcanistDifferentialRevisionStatus::NEEDS_REVIEW) { return array(); } else { - return $object->getReviewers(); + return $object->getReviewerPHIDs(); } } public function getCCUserPHIDs($object) { - return $object->getCCPHIDs(); + return PhabricatorSubscribersQuery::loadSubscribersForPHID( + $object->getPHID()); } public function getObjectTitle($object) { diff --git a/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php b/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php index 53fd62fe23..19d15c5c3f 100644 --- a/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php +++ b/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php @@ -85,7 +85,6 @@ final class HeraldDifferentialRevisionAdapter $revision = id(new DifferentialRevisionQuery()) ->withIDs(array($revision->getID())) ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->needRelationships(true) ->needReviewerStatus(true) ->executeOne(); diff --git a/src/applications/differential/query/DifferentialRevisionQuery.php b/src/applications/differential/query/DifferentialRevisionQuery.php index c3fb98e607..1b60df777a 100644 --- a/src/applications/differential/query/DifferentialRevisionQuery.php +++ b/src/applications/differential/query/DifferentialRevisionQuery.php @@ -43,7 +43,6 @@ final class DifferentialRevisionQuery const ORDER_MODIFIED = 'order-modified'; const ORDER_CREATED = 'order-created'; - private $needRelationships = false; private $needActiveDiffs = false; private $needDiffIDs = false; private $needCommitPHIDs = false; @@ -227,20 +226,6 @@ final class DifferentialRevisionQuery } - - /** - * Set whether or not the query will load and attach relationships. - * - * @param bool True to load and attach relationships. - * @return this - * @task config - */ - public function needRelationships($need_relationships) { - $this->needRelationships = $need_relationships; - return $this; - } - - /** * Set whether or not the query should load the active diff for each * revision. @@ -425,10 +410,6 @@ final class DifferentialRevisionQuery $table = new DifferentialRevision(); $conn_r = $table->establishConnection('r'); - if ($this->needRelationships) { - $this->loadRelationships($conn_r, $revisions); - } - if ($this->needCommitPHIDs) { $this->loadCommitPHIDs($conn_r, $revisions); } @@ -854,40 +835,6 @@ final class DifferentialRevisionQuery ); } - private function loadRelationships($conn_r, array $revisions) { - assert_instances_of($revisions, 'DifferentialRevision'); - - $type_reviewer = DifferentialRevisionHasReviewerEdgeType::EDGECONST; - $type_subscriber = PhabricatorObjectHasSubscriberEdgeType::EDGECONST; - - $edges = id(new PhabricatorEdgeQuery()) - ->withSourcePHIDs(mpull($revisions, 'getPHID')) - ->withEdgeTypes(array($type_reviewer, $type_subscriber)) - ->setOrder(PhabricatorEdgeQuery::ORDER_OLDEST_FIRST) - ->execute(); - - $type_map = array( - DifferentialRevision::RELATION_REVIEWER => $type_reviewer, - DifferentialRevision::RELATION_SUBSCRIBED => $type_subscriber, - ); - - foreach ($revisions as $revision) { - $data = array(); - foreach ($type_map as $rel_type => $edge_type) { - $revision_edges = $edges[$revision->getPHID()][$edge_type]; - foreach ($revision_edges as $dst_phid => $edge_data) { - $data[] = array( - 'relation' => $rel_type, - 'objectPHID' => $dst_phid, - 'reasonPHID' => null, - ); - } - } - - $revision->attachRelationships($data); - } - } - private function loadCommitPHIDs($conn_r, array $revisions) { assert_instances_of($revisions, 'DifferentialRevision'); $commit_phids = queryfx_all( diff --git a/src/applications/differential/query/DifferentialRevisionSearchEngine.php b/src/applications/differential/query/DifferentialRevisionSearchEngine.php index 9212860292..2c40be4813 100644 --- a/src/applications/differential/query/DifferentialRevisionSearchEngine.php +++ b/src/applications/differential/query/DifferentialRevisionSearchEngine.php @@ -19,7 +19,6 @@ final class DifferentialRevisionSearchEngine return id(new DifferentialRevisionQuery()) ->needFlags(true) ->needDrafts(true) - ->needRelationships(true) ->needReviewerStatus(true); } diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index ff826a36b7..062503451f 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -38,7 +38,6 @@ final class DifferentialRevision extends DifferentialDAO protected $editPolicy = PhabricatorPolicies::POLICY_USER; protected $properties = array(); - private $relationships = self::ATTACHABLE; private $commits = self::ATTACHABLE; private $activeDiff = self::ATTACHABLE; private $diffIDs = self::ATTACHABLE; @@ -69,7 +68,6 @@ final class DifferentialRevision extends DifferentialDAO return id(new DifferentialRevision()) ->setViewPolicy($view_policy) ->setAuthorPHID($actor->getPHID()) - ->attachRelationships(array()) ->attachRepository(null) ->attachActiveDiff(null) ->attachReviewerStatus(array()) @@ -238,64 +236,6 @@ final class DifferentialRevision extends DifferentialDAO return parent::save(); } - public function loadRelationships() { - if (!$this->getID()) { - $this->relationships = array(); - return; - } - - $data = array(); - - $subscriber_phids = PhabricatorEdgeQuery::loadDestinationPHIDs( - $this->getPHID(), - PhabricatorObjectHasSubscriberEdgeType::EDGECONST); - $subscriber_phids = array_reverse($subscriber_phids); - foreach ($subscriber_phids as $phid) { - $data[] = array( - 'relation' => self::RELATION_SUBSCRIBED, - 'objectPHID' => $phid, - 'reasonPHID' => null, - ); - } - - $reviewer_phids = PhabricatorEdgeQuery::loadDestinationPHIDs( - $this->getPHID(), - DifferentialRevisionHasReviewerEdgeType::EDGECONST); - $reviewer_phids = array_reverse($reviewer_phids); - foreach ($reviewer_phids as $phid) { - $data[] = array( - 'relation' => self::RELATION_REVIEWER, - 'objectPHID' => $phid, - 'reasonPHID' => null, - ); - } - - return $this->attachRelationships($data); - } - - public function attachRelationships(array $relationships) { - $this->relationships = igroup($relationships, 'relation'); - return $this; - } - - public function getReviewers() { - return $this->getRelatedPHIDs(self::RELATION_REVIEWER); - } - - public function getCCPHIDs() { - return $this->getRelatedPHIDs(self::RELATION_SUBSCRIBED); - } - - private function getRelatedPHIDs($relation) { - $this->assertAttached($this->relationships); - - return ipull($this->getRawRelations($relation), 'objectPHID'); - } - - public function getRawRelations($relation) { - return idx($this->relationships, $relation, array()); - } - public function getHashes() { return $this->assertAttached($this->hashes); } @@ -402,6 +342,11 @@ final class DifferentialRevision extends DifferentialDAO return $this; } + public function getReviewerPHIDs() { + $reviewers = $this->getReviewerStatus(); + return mpull($reviewers, 'getReviewerPHID'); + } + public function getReviewerPHIDsForEdit() { $reviewers = $this->getReviewerStatus(); diff --git a/src/applications/differential/view/DifferentialRevisionListView.php b/src/applications/differential/view/DifferentialRevisionListView.php index 811e74fd65..5aacafbb69 100644 --- a/src/applications/differential/view/DifferentialRevisionListView.php +++ b/src/applications/differential/view/DifferentialRevisionListView.php @@ -52,10 +52,7 @@ final class DifferentialRevisionListView extends AphrontView { $phids = array(); foreach ($this->revisions as $revision) { $phids[] = array($revision->getAuthorPHID()); - - // TODO: Switch to getReviewerStatus(), but not all callers pass us - // revisions with this data loaded. - $phids[] = $revision->getReviewers(); + $phids[] = $revision->getReviewerPHIDs(); } return array_mergev($phids); } @@ -132,8 +129,7 @@ final class DifferentialRevisionListView extends AphrontView { } $reviewers = array(); - // TODO: As above, this should be based on `getReviewerStatus()`. - foreach ($revision->getReviewers() as $reviewer) { + foreach ($revision->getReviewerPHIDs() as $reviewer) { $reviewers[] = $this->handles[$reviewer]->renderLink(); } if (!$reviewers) { diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php index aa8e370e25..dc0131356d 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseController.php @@ -1765,7 +1765,7 @@ final class DiffusionBrowseController extends DiffusionController { ->withUpdatedEpochBetween($recent, null) ->setOrder(DifferentialRevisionQuery::ORDER_MODIFIED) ->setLimit(10) - ->needRelationships(true) + ->needReviewerStatus(true) ->needFlags(true) ->needDrafts(true) ->execute(); diff --git a/src/applications/diffusion/herald/DiffusionCommitRevisionReviewersHeraldField.php b/src/applications/diffusion/herald/DiffusionCommitRevisionReviewersHeraldField.php index 684f5d75d6..50900ede78 100644 --- a/src/applications/diffusion/herald/DiffusionCommitRevisionReviewersHeraldField.php +++ b/src/applications/diffusion/herald/DiffusionCommitRevisionReviewersHeraldField.php @@ -20,7 +20,7 @@ final class DiffusionCommitRevisionReviewersHeraldField return array(); } - return $revision->getReviewers(); + return $revision->getReviewerPHIDs(); } protected function getHeraldFieldStandardType() { diff --git a/src/applications/diffusion/herald/DiffusionPreCommitContentRevisionReviewersHeraldField.php b/src/applications/diffusion/herald/DiffusionPreCommitContentRevisionReviewersHeraldField.php index bce6b5b517..936126ba89 100644 --- a/src/applications/diffusion/herald/DiffusionPreCommitContentRevisionReviewersHeraldField.php +++ b/src/applications/diffusion/herald/DiffusionPreCommitContentRevisionReviewersHeraldField.php @@ -20,8 +20,8 @@ final class DiffusionPreCommitContentRevisionReviewersHeraldField return array(); } - return $revision->getReviewers(); - } + return $revision->getReviewerPHIDs(); + } protected function getHeraldFieldStandardType() { return self::STANDARD_PHID_LIST; diff --git a/src/applications/diffusion/herald/HeraldCommitAdapter.php b/src/applications/diffusion/herald/HeraldCommitAdapter.php index 9d63b9db3d..3465924f92 100644 --- a/src/applications/diffusion/herald/HeraldCommitAdapter.php +++ b/src/applications/diffusion/herald/HeraldCommitAdapter.php @@ -190,7 +190,6 @@ final class HeraldCommitAdapter $revision = id(new DifferentialRevisionQuery()) ->withIDs(array($revision_id)) ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->needRelationships(true) ->needReviewerStatus(true) ->executeOne(); if ($revision) { diff --git a/src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php b/src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php index 13c2695fed..5fbc970dfa 100644 --- a/src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php +++ b/src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php @@ -190,7 +190,7 @@ final class HeraldPreCommitContentAdapter extends HeraldPreCommitAdapter { $this->revision = id(new DifferentialRevisionQuery()) ->setViewer(PhabricatorUser::getOmnipotentUser()) ->withIDs(array($revision_id)) - ->needRelationships(true) + ->needReviewerStatus(true) ->executeOne(); } }