revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2025-01-11 07:11:04 +01:00
Code Issues Releases Wiki Activity

Remove obsolete "relationships" code from Differential

Browse source 
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
This commit is contained in:
epriestley 2017-03-20 14:02:08 -07:00
parent dccd799b1b
commit d179d0150c
14 changed files with 35 additions and 140 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
src/applications/differential/conduit/DifferentialGetRevisionConduitAPIMethod.php
View file

@ -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())

14
src/applications/differential/conduit/DifferentialQueryConduitAPIMethod.php
View file

@ -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(),

10
src/applications/differential/controller/DifferentialRevisionViewController.php
View file

@ -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);

9
src/applications/differential/doorkeeper/DifferentialDoorkeeperRevisionFeedStoryPublisher.php
View file

@ -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) {

1
src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php
View file

@ -85,7 +85,6 @@ final class HeraldDifferentialRevisionAdapter
$revision = id(new DifferentialRevisionQuery())
->withIDs(array($revision->getID()))
->setViewer(PhabricatorUser::getOmnipotentUser())
->needRelationships(true)
->needReviewerStatus(true)
->executeOne();

53
src/applications/differential/query/DifferentialRevisionQuery.php
View file

@ -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(

1
src/applications/differential/query/DifferentialRevisionSearchEngine.php
View file

@ -19,7 +19,6 @@ final class DifferentialRevisionSearchEngine
return id(new DifferentialRevisionQuery())
->needFlags(true)
->needDrafts(true)
->needRelationships(true)
->needReviewerStatus(true);
}

65
src/applications/differential/storage/DifferentialRevision.php
View file

@ -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();

8
src/applications/differential/view/DifferentialRevisionListView.php
View file

@ -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) {

2
src/applications/diffusion/controller/DiffusionBrowseController.php
View file

@ -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();

2
src/applications/diffusion/herald/DiffusionCommitRevisionReviewersHeraldField.php
View file

@ -20,7 +20,7 @@ final class DiffusionCommitRevisionReviewersHeraldField
return array();
}
return $revision->getReviewers();
return $revision->getReviewerPHIDs();
}
protected function getHeraldFieldStandardType() {

4
src/applications/diffusion/herald/DiffusionPreCommitContentRevisionReviewersHeraldField.php
View file

@ -20,8 +20,8 @@ final class DiffusionPreCommitContentRevisionReviewersHeraldField
return array();
}
return $revision->getReviewers();
}
return $revision->getReviewerPHIDs();
}
protected function getHeraldFieldStandardType() {
return self::STANDARD_PHID_LIST;

1
src/applications/diffusion/herald/HeraldCommitAdapter.php
View file

@ -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) {

2
src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php
View file

@ -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();
}
}