From 220ac48801a10ad9b288e86e708e03db481460d9 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 26 Feb 2016 12:02:04 -0800 Subject: [PATCH] Remove buildable handle / container handle logic form Harbormaster buildable queries Summary: Ref T10457. We currently have these weird, out-of-place methods on Harbormaster queries that just load handles. These were written before HandlePool, and HandlePool is now more convenient, simpler, and more efficient. Drop this stuff in favor of using handle pools off `$viewer`. Test Plan: Looked at buildable list, looked at buildable detail, grepped for removed methods. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10457 Differential Revision: https://secure.phabricator.com/D15354 --- .../HarbormasterBuildableViewController.php | 11 +-- .../HarbormasterBuildableInterface.php | 1 - .../phid/HarbormasterBuildablePHIDType.php | 26 +++++-- .../query/HarbormasterBuildableQuery.php | 67 +++---------------- .../HarbormasterBuildableSearchEngine.php | 31 ++++++--- .../storage/HarbormasterBuildable.php | 24 ++----- 6 files changed, 60 insertions(+), 100 deletions(-) diff --git a/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php b/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php index 91fb45176f..42f2572266 100644 --- a/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php +++ b/src/applications/harbormaster/controller/HarbormasterBuildableViewController.php @@ -9,8 +9,6 @@ final class HarbormasterBuildableViewController $buildable = id(new HarbormasterBuildableQuery()) ->setViewer($viewer) ->withIDs(array($request->getURIData('id'))) - ->needBuildableHandles(true) - ->needContainerHandles(true) ->executeOne(); if (!$buildable) { return new Aphront404Response(); @@ -167,15 +165,18 @@ final class HarbormasterBuildableViewController ->setActionList($actions); $box->addPropertyList($properties); - if ($buildable->getContainerHandle() !== null) { + $container_phid = $buildable->getContainerPHID(); + $buildable_phid = $buildable->getBuildablePHID(); + + if ($container_phid) { $properties->addProperty( pht('Container'), - $buildable->getContainerHandle()->renderLink()); + $viewer->renderHandle($container_phid)); } $properties->addProperty( pht('Buildable'), - $buildable->getBuildableHandle()->renderLink()); + $viewer->renderHandle($buildable_phid)); $properties->addProperty( pht('Origin'), diff --git a/src/applications/harbormaster/interface/HarbormasterBuildableInterface.php b/src/applications/harbormaster/interface/HarbormasterBuildableInterface.php index a5517a6b2b..d2b2d332aa 100644 --- a/src/applications/harbormaster/interface/HarbormasterBuildableInterface.php +++ b/src/applications/harbormaster/interface/HarbormasterBuildableInterface.php @@ -6,7 +6,6 @@ interface HarbormasterBuildableInterface { public function getHarbormasterContainerPHID(); public function getBuildVariables(); - public function getAvailableBuildVariables(); } diff --git a/src/applications/harbormaster/phid/HarbormasterBuildablePHIDType.php b/src/applications/harbormaster/phid/HarbormasterBuildablePHIDType.php index c6ccabf515..fbc08d4ef3 100644 --- a/src/applications/harbormaster/phid/HarbormasterBuildablePHIDType.php +++ b/src/applications/harbormaster/phid/HarbormasterBuildablePHIDType.php @@ -21,8 +21,7 @@ final class HarbormasterBuildablePHIDType extends PhabricatorPHIDType { array $phids) { return id(new HarbormasterBuildableQuery()) - ->withPHIDs($phids) - ->needBuildableHandles(true); + ->withPHIDs($phids); } public function loadHandles( @@ -30,15 +29,30 @@ final class HarbormasterBuildablePHIDType extends PhabricatorPHIDType { array $handles, array $objects) { + $viewer = $this->getViewer(); + + $target_phids = array(); + foreach ($objects as $phid => $object) { + $target_phids[] = $object->getBuildablePHID(); + } + $target_handles = $viewer->loadHandles($target_phids); + foreach ($handles as $phid => $handle) { $buildable = $objects[$phid]; $id = $buildable->getID(); - $target = $buildable->getBuildableHandle()->getFullName(); + $buildable_phid = $buildable->getBuildablePHID(); - $handle->setURI("/B{$id}"); - $handle->setName("B{$id}"); - $handle->setFullName("B{$id}: ".$target); + $target = $target_handles[$buildable_phid]; + $target_name = $target->getFullName(); + + $uri = $buildable->getURI(); + $monogram = $buildable->getMonogram(); + + $handle + ->setURI($uri) + ->setName($monogram) + ->setFullName("{$monogram}: {$target_name}"); } } diff --git a/src/applications/harbormaster/query/HarbormasterBuildableQuery.php b/src/applications/harbormaster/query/HarbormasterBuildableQuery.php index 2986bb45b7..fc069bd031 100644 --- a/src/applications/harbormaster/query/HarbormasterBuildableQuery.php +++ b/src/applications/harbormaster/query/HarbormasterBuildableQuery.php @@ -10,8 +10,6 @@ final class HarbormasterBuildableQuery private $manualBuildables; private $needContainerObjects; - private $needContainerHandles; - private $needBuildableHandles; private $needBuilds; private $needTargets; @@ -45,16 +43,6 @@ final class HarbormasterBuildableQuery return $this; } - public function needContainerHandles($need) { - $this->needContainerHandles = $need; - return $this; - } - - public function needBuildableHandles($need) { - $this->needBuildableHandles = $need; - return $this; - } - public function needBuilds($need) { $this->needBuilds = $need; return $this; @@ -99,60 +87,23 @@ final class HarbormasterBuildableQuery } protected function didFilterPage(array $page) { - if ($this->needContainerObjects || $this->needContainerHandles) { + if ($this->needContainerObjects) { $container_phids = array_filter(mpull($page, 'getContainerPHID')); - if ($this->needContainerObjects) { - $containers = array(); - - if ($container_phids) { - $containers = id(new PhabricatorObjectQuery()) - ->setViewer($this->getViewer()) - ->withPHIDs($container_phids) - ->setParentQuery($this) - ->execute(); - $containers = mpull($containers, null, 'getPHID'); - } - - foreach ($page as $key => $buildable) { - $container_phid = $buildable->getContainerPHID(); - $buildable->attachContainerObject(idx($containers, $container_phid)); - } - } - - if ($this->needContainerHandles) { - $handles = array(); - - if ($container_phids) { - $handles = id(new PhabricatorHandleQuery()) - ->setViewer($this->getViewer()) - ->withPHIDs($container_phids) - ->setParentQuery($this) - ->execute(); - } - - foreach ($page as $key => $buildable) { - $container_phid = $buildable->getContainerPHID(); - $buildable->attachContainerHandle(idx($handles, $container_phid)); - } - } - } - - if ($this->needBuildableHandles) { - $handles = array(); - - $handle_phids = array_filter(mpull($page, 'getBuildablePHID')); - if ($handle_phids) { - $handles = id(new PhabricatorHandleQuery()) + if ($container_phids) { + $containers = id(new PhabricatorObjectQuery()) ->setViewer($this->getViewer()) - ->withPHIDs($handle_phids) + ->withPHIDs($container_phids) ->setParentQuery($this) ->execute(); + $containers = mpull($containers, null, 'getPHID'); + } else { + $containers = array(); } foreach ($page as $key => $buildable) { - $handle_phid = $buildable->getBuildablePHID(); - $buildable->attachBuildableHandle(idx($handles, $handle_phid)); + $container_phid = $buildable->getContainerPHID(); + $buildable->attachContainerObject(idx($containers, $container_phid)); } } diff --git a/src/applications/harbormaster/query/HarbormasterBuildableSearchEngine.php b/src/applications/harbormaster/query/HarbormasterBuildableSearchEngine.php index 195a68a695..982db3dd39 100644 --- a/src/applications/harbormaster/query/HarbormasterBuildableSearchEngine.php +++ b/src/applications/harbormaster/query/HarbormasterBuildableSearchEngine.php @@ -58,9 +58,7 @@ final class HarbormasterBuildableSearchEngine } public function buildQueryFromSavedQuery(PhabricatorSavedQuery $saved) { - $query = id(new HarbormasterBuildableQuery()) - ->needContainerHandles(true) - ->needBuildableHandles(true); + $query = id(new HarbormasterBuildableQuery()); $container_phids = $saved->getParameter('containerPHIDs', array()); if ($container_phids) { @@ -185,23 +183,36 @@ final class HarbormasterBuildableSearchEngine $viewer = $this->requireViewer(); + $phids = array(); + foreach ($buildables as $buildable) { + $phids[] = $buildable->getContainerPHID(); + $phids[] = $buildable->getBuildablePHID(); + } + $handles = $viewer->loadHandles($phids); + + $list = new PHUIObjectItemListView(); foreach ($buildables as $buildable) { $id = $buildable->getID(); + $container_phid = $buildable->getContainerPHID(); + $buildable_phid = $buildable->getBuildablePHID(); + $item = id(new PHUIObjectItemView()) ->setHeader(pht('Buildable %d', $buildable->getID())); - if ($buildable->getContainerHandle() !== null) { - $item->addAttribute($buildable->getContainerHandle()->getName()); - } - if ($buildable->getBuildableHandle() !== null) { - $item->addAttribute($buildable->getBuildableHandle()->getFullName()); + + if ($container_phid) { + $handle = $handles[$container_phid]; + $item->addAttribute($handle->getName()); } - if ($id) { - $item->setHref("/B{$id}"); + if ($buildable_phid) { + $handle = $handles[$buildable_phid]; + $item->addAttribute($handle->getFullName()); } + $item->setHref($buildable->getURI()); + if ($buildable->getIsManualBuildable()) { $item->addIcon('fa-wrench grey', pht('Manual')); } diff --git a/src/applications/harbormaster/storage/HarbormasterBuildable.php b/src/applications/harbormaster/storage/HarbormasterBuildable.php index 6286072b42..7ee6981477 100644 --- a/src/applications/harbormaster/storage/HarbormasterBuildable.php +++ b/src/applications/harbormaster/storage/HarbormasterBuildable.php @@ -13,8 +13,6 @@ final class HarbormasterBuildable extends HarbormasterDAO private $buildableObject = self::ATTACHABLE; private $containerObject = self::ATTACHABLE; - private $buildableHandle = self::ATTACHABLE; - private $containerHandle = self::ATTACHABLE; private $builds = self::ATTACHABLE; const STATUS_BUILDING = 'building'; @@ -70,6 +68,10 @@ final class HarbormasterBuildable extends HarbormasterDAO return 'B'.$this->getID(); } + public function getURI() { + return '/'.$this->getMonogram(); + } + /** * Returns an existing buildable for the object's PHID or creates a * new buildable implicitly if needed. @@ -237,24 +239,6 @@ final class HarbormasterBuildable extends HarbormasterDAO return $this->assertAttached($this->containerObject); } - public function attachContainerHandle($container_handle) { - $this->containerHandle = $container_handle; - return $this; - } - - public function getContainerHandle() { - return $this->assertAttached($this->containerHandle); - } - - public function attachBuildableHandle($buildable_handle) { - $this->buildableHandle = $buildable_handle; - return $this; - } - - public function getBuildableHandle() { - return $this->assertAttached($this->buildableHandle); - } - public function attachBuilds(array $builds) { assert_instances_of($builds, 'HarbormasterBuild'); $this->builds = $builds;