From 7ed6996604358c118e6a0366220994a49ee0349b Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 21 Jul 2013 06:34:21 -0700 Subject: [PATCH] Provide basic infrastructure for moving PHIDs, Handles and Object Names to applications Summary: See discussion in T2715. Currently, PHIDs are all hard coded in the PHID application. In the long run, we need to move them out into actual applications. A specific immediate issue is Releeph, which uses a very very old and very broken mechanism to inject PHIDs in a way that only sort of works. Moving forward, every PHID type will be provided by a `PhabricatorPHIDType` subclass, which will manage loading it, etc. This also moves toward cleaning up the "load objects by name" (where "name" means something like `D12`) code, which is an //enormous// mess and spread across at least 4-5 callsites. Test Plan: Used `phid.lookup` and `phid.query` to load Slowvotes. Reviewers: btrahan Reviewed By: btrahan CC: aran Differential Revision: https://secure.phabricator.com/D6502 --- .../sql/patches/20130715.votecomments.php | 2 +- src/__phutil_library_map__.php | 7 +- .../PhabricatorSetupCheckExtraConfig.php | 7 ++ .../phid/PhabricatorObjectHandle.php | 6 +- .../phid/PhabricatorPHIDConstants.php | 1 - .../conduit/ConduitAPI_phid_lookup_Method.php | 10 +++ .../config/PhabricatorPHIDConfigOptions.php | 27 ------ .../handle/PhabricatorObjectHandleData.php | 38 +++------ .../phid/query/PhabricatorObjectQuery.php | 85 +++++++++++++++++++ .../phid/storage/PhabricatorPHID.php | 12 +++ .../phid/type/PhabricatorPHIDType.php | 57 +++++++++++++ .../phid/PhabricatorSlowvotePHIDTypePoll.php | 62 ++++++++++++++ .../storage/PhabricatorSlowvotePoll.php | 2 +- .../PhabricatorSlowvoteTransaction.php | 2 +- .../edges/constants/PhabricatorEdgeConfig.php | 10 ++- 15 files changed, 267 insertions(+), 61 deletions(-) delete mode 100644 src/applications/phid/config/PhabricatorPHIDConfigOptions.php create mode 100644 src/applications/phid/query/PhabricatorObjectQuery.php create mode 100644 src/applications/phid/type/PhabricatorPHIDType.php create mode 100644 src/applications/slowvote/phid/PhabricatorSlowvotePHIDTypePoll.php diff --git a/resources/sql/patches/20130715.votecomments.php b/resources/sql/patches/20130715.votecomments.php index 3611c0eae3..a8b9779d33 100644 --- a/resources/sql/patches/20130715.votecomments.php +++ b/resources/sql/patches/20130715.votecomments.php @@ -44,7 +44,7 @@ foreach ($comments as $comment) { PhabricatorPHIDConstants::PHID_TYPE_XCMT); $xaction_phid = PhabricatorPHID::generateNewPHID( PhabricatorPHIDConstants::PHID_TYPE_XACT, - PhabricatorPHIDConstants::PHID_TYPE_POLL); + PhabricatorSlowvotePHIDTypePoll::TYPECONST); $source = PhabricatorContentSource::newForSource( PhabricatorContentSource::SOURCE_LEGACY, diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 920ebee561..a47a427344 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1313,6 +1313,7 @@ phutil_register_library_map(array( 'PhabricatorObjectListView' => 'view/control/PhabricatorObjectListView.php', 'PhabricatorObjectMailReceiver' => 'applications/metamta/receiver/PhabricatorObjectMailReceiver.php', 'PhabricatorObjectMailReceiverTestCase' => 'applications/metamta/receiver/__tests__/PhabricatorObjectMailReceiverTestCase.php', + 'PhabricatorObjectQuery' => 'applications/phid/query/PhabricatorObjectQuery.php', 'PhabricatorObjectSelectorDialog' => 'view/control/PhabricatorObjectSelectorDialog.php', 'PhabricatorOffsetPagedQuery' => 'infrastructure/query/PhabricatorOffsetPagedQuery.php', 'PhabricatorOwnerPathQuery' => 'applications/owners/query/PhabricatorOwnerPathQuery.php', @@ -1331,10 +1332,10 @@ phutil_register_library_map(array( 'PhabricatorOwnersPath' => 'applications/owners/storage/PhabricatorOwnersPath.php', 'PhabricatorPHDConfigOptions' => 'applications/config/option/PhabricatorPHDConfigOptions.php', 'PhabricatorPHID' => 'applications/phid/storage/PhabricatorPHID.php', - 'PhabricatorPHIDConfigOptions' => 'applications/phid/config/PhabricatorPHIDConfigOptions.php', 'PhabricatorPHIDConstants' => 'applications/phid/PhabricatorPHIDConstants.php', 'PhabricatorPHIDController' => 'applications/phid/controller/PhabricatorPHIDController.php', 'PhabricatorPHIDLookupController' => 'applications/phid/controller/PhabricatorPHIDLookupController.php', + 'PhabricatorPHIDType' => 'applications/phid/type/PhabricatorPHIDType.php', 'PhabricatorPHPMailerConfigOptions' => 'applications/config/option/PhabricatorPHPMailerConfigOptions.php', 'PhabricatorPagedFormExample' => 'applications/uiexample/examples/PhabricatorPagedFormExample.php', 'PhabricatorPaste' => 'applications/paste/storage/PhabricatorPaste.php', @@ -1540,6 +1541,7 @@ phutil_register_library_map(array( 'PhabricatorSlowvoteEditor' => 'applications/slowvote/editor/PhabricatorSlowvoteEditor.php', 'PhabricatorSlowvoteListController' => 'applications/slowvote/controller/PhabricatorSlowvoteListController.php', 'PhabricatorSlowvoteOption' => 'applications/slowvote/storage/PhabricatorSlowvoteOption.php', + 'PhabricatorSlowvotePHIDTypePoll' => 'applications/slowvote/phid/PhabricatorSlowvotePHIDTypePoll.php', 'PhabricatorSlowvotePoll' => 'applications/slowvote/storage/PhabricatorSlowvotePoll.php', 'PhabricatorSlowvotePollController' => 'applications/slowvote/controller/PhabricatorSlowvotePollController.php', 'PhabricatorSlowvoteQuery' => 'applications/slowvote/query/PhabricatorSlowvoteQuery.php', @@ -3275,6 +3277,7 @@ phutil_register_library_map(array( 'PhabricatorObjectListView' => 'AphrontView', 'PhabricatorObjectMailReceiver' => 'PhabricatorMailReceiver', 'PhabricatorObjectMailReceiverTestCase' => 'PhabricatorTestCase', + 'PhabricatorObjectQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorOffsetPagedQuery' => 'PhabricatorQuery', 'PhabricatorOwnersConfigOptions' => 'PhabricatorApplicationConfigOptions', 'PhabricatorOwnersController' => 'PhabricatorController', @@ -3293,7 +3296,6 @@ phutil_register_library_map(array( 'PhabricatorOwnersPackageTestCase' => 'PhabricatorTestCase', 'PhabricatorOwnersPath' => 'PhabricatorOwnersDAO', 'PhabricatorPHDConfigOptions' => 'PhabricatorApplicationConfigOptions', - 'PhabricatorPHIDConfigOptions' => 'PhabricatorApplicationConfigOptions', 'PhabricatorPHIDController' => 'PhabricatorController', 'PhabricatorPHIDLookupController' => 'PhabricatorPHIDController', 'PhabricatorPHPMailerConfigOptions' => 'PhabricatorApplicationConfigOptions', @@ -3513,6 +3515,7 @@ phutil_register_library_map(array( 1 => 'PhabricatorApplicationSearchResultsControllerInterface', ), 'PhabricatorSlowvoteOption' => 'PhabricatorSlowvoteDAO', + 'PhabricatorSlowvotePHIDTypePoll' => 'PhabricatorPHIDType', 'PhabricatorSlowvotePoll' => array( 0 => 'PhabricatorSlowvoteDAO', diff --git a/src/applications/config/check/PhabricatorSetupCheckExtraConfig.php b/src/applications/config/check/PhabricatorSetupCheckExtraConfig.php index 1482e7d2b1..6d6096d4ac 100644 --- a/src/applications/config/check/PhabricatorSetupCheckExtraConfig.php +++ b/src/applications/config/check/PhabricatorSetupCheckExtraConfig.php @@ -141,6 +141,13 @@ final class PhabricatorSetupCheckExtraConfig extends PhabricatorSetupCheck { $ancient_config = array_fill_keys($auth_config, $reason_auth); + $ancient_config += array( + 'phid.external-loaders' => + pht( + 'External loaders have been replaced. Extend `PhabricatorPHIDType` '. + 'to implement new PHID and handle types.'), + ); + return $ancient_config; } } diff --git a/src/applications/phid/PhabricatorObjectHandle.php b/src/applications/phid/PhabricatorObjectHandle.php index c8851196da..392a703572 100644 --- a/src/applications/phid/PhabricatorObjectHandle.php +++ b/src/applications/phid/PhabricatorObjectHandle.php @@ -99,6 +99,11 @@ final class PhabricatorObjectHandle { } public function getTypeName() { + $types = PhabricatorPHIDType::getAllTypes(); + if (isset($types[$this->getType()])) { + return $types[$this->getType()]->getTypeName(); + } + static $map = array( PhabricatorPHIDConstants::PHID_TYPE_USER => 'User', PhabricatorPHIDConstants::PHID_TYPE_TASK => 'Task', @@ -116,7 +121,6 @@ final class PhabricatorObjectHandle { PhabricatorPHIDConstants::PHID_TYPE_PSTE => 'Paste', PhabricatorPHIDConstants::PHID_TYPE_PROJ => 'Project', PhabricatorPHIDConstants::PHID_TYPE_LEGD => 'Legalpad Document', - PhabricatorPHIDConstants::PHID_TYPE_POLL => 'Slowvote', ); return idx($map, $this->getType(), $this->getType()); diff --git a/src/applications/phid/PhabricatorPHIDConstants.php b/src/applications/phid/PhabricatorPHIDConstants.php index c72b9e3a37..25980f3dc9 100644 --- a/src/applications/phid/PhabricatorPHIDConstants.php +++ b/src/applications/phid/PhabricatorPHIDConstants.php @@ -15,7 +15,6 @@ final class PhabricatorPHIDConstants { const PHID_TYPE_OPKG = 'OPKG'; const PHID_TYPE_PSTE = 'PSTE'; const PHID_TYPE_STRY = 'STRY'; - const PHID_TYPE_POLL = 'POLL'; const PHID_TYPE_WIKI = 'WIKI'; const PHID_TYPE_APRJ = 'APRJ'; const PHID_TYPE_ACMT = 'ACMT'; diff --git a/src/applications/phid/conduit/ConduitAPI_phid_lookup_Method.php b/src/applications/phid/conduit/ConduitAPI_phid_lookup_Method.php index ea602ec3cb..60b051b735 100644 --- a/src/applications/phid/conduit/ConduitAPI_phid_lookup_Method.php +++ b/src/applications/phid/conduit/ConduitAPI_phid_lookup_Method.php @@ -34,6 +34,16 @@ final class ConduitAPI_phid_lookup_Method } } + $query = id(new PhabricatorObjectQuery()) + ->setViewer($request->getUser()) + ->withNames($names); + $query->execute(); + $objects = $query->getNamedResults(); + + foreach ($objects as $name => $object) { + $phids[$name] = $object->getPHID(); + } + $handles = id(new PhabricatorObjectHandleData($phids)) ->setViewer($request->getUser()) ->loadHandles(); diff --git a/src/applications/phid/config/PhabricatorPHIDConfigOptions.php b/src/applications/phid/config/PhabricatorPHIDConfigOptions.php deleted file mode 100644 index c59c569700..0000000000 --- a/src/applications/phid/config/PhabricatorPHIDConfigOptions.php +++ /dev/null @@ -1,27 +0,0 @@ -newOption( - 'phid.external-loaders', - 'wild', - null) - ->setDescription( - pht( - 'For each new 4-char PHID type, point to an external loader for '. - 'that type.')), - ); - } - -} diff --git a/src/applications/phid/handle/PhabricatorObjectHandleData.php b/src/applications/phid/handle/PhabricatorObjectHandleData.php index f81b3d6b2a..7acb82edf3 100644 --- a/src/applications/phid/handle/PhabricatorObjectHandleData.php +++ b/src/applications/phid/handle/PhabricatorObjectHandleData.php @@ -22,9 +22,17 @@ final class PhabricatorObjectHandleData { } public function loadObjects() { - $types = phid_group_by_type($this->phids); + $phids = array_fuse($this->phids); - $objects = array(); + $objects = id(new PhabricatorObjectQuery()) + ->setViewer($this->viewer) + ->withPHIDs($phids) + ->execute(); + + // For objects which don't support PhabricatorPHIDType yet, load them the + // old way. + $phids = array_diff_key($phids, array_keys($objects)); + $types = phid_group_by_type($phids); foreach ($types as $type => $phids) { $objects += $this->loadObjectsOfType($type, $phids); } @@ -142,11 +150,6 @@ final class PhabricatorObjectHandleData { ->loadAllWhere('phid IN (%Ls)', $phids); return mpull($images, null, 'getPHID'); - case PhabricatorPHIDConstants::PHID_TYPE_POLL: - $polls = id(new PhabricatorSlowvotePoll()) - ->loadAllWhere('phid IN (%Ls)', $phids); - return mpull($polls, null, 'getPHID'); - case PhabricatorPHIDConstants::PHID_TYPE_XACT: $subtypes = array(); foreach ($phids as $phid) { @@ -238,16 +241,13 @@ final class PhabricatorObjectHandleData { } public function loadHandles() { + $objects = $this->loadObjects(); $types = phid_group_by_type($this->phids); $handles = array(); - $external_loaders = PhabricatorEnv::getEnvConfig('phid.external-loaders'); - foreach ($types as $type => $phids) { - $objects = $this->loadObjectsOfType($type, $phids); - switch ($type) { case PhabricatorPHIDConstants::PHID_TYPE_MAGIC: @@ -674,7 +674,7 @@ final class PhabricatorObjectHandleData { break; - case PhabricatorPHIDConstants::PHID_TYPE_POLL: + case PhabricatorSlowvotePHIDTypePoll::TYPECONST: foreach ($phids as $phid) { $handle = new PhabricatorObjectHandle(); $handle->setPHID($phid); @@ -807,20 +807,6 @@ final class PhabricatorObjectHandleData { default: - $loader = null; - if (isset($external_loaders[$type])) { - $loader = $external_loaders[$type]; - } else if (isset($external_loaders['*'])) { - $loader = $external_loaders['*']; - } - - if ($loader) { - $object = newv($loader, array()); - assert_instances_of(array($type => $object), 'ObjectHandleLoader'); - $handles += $object->loadHandles($phids); - break; - } - foreach ($phids as $phid) { $handle = new PhabricatorObjectHandle(); $handle->setType($type); diff --git a/src/applications/phid/query/PhabricatorObjectQuery.php b/src/applications/phid/query/PhabricatorObjectQuery.php new file mode 100644 index 0000000000..e9b069be43 --- /dev/null +++ b/src/applications/phid/query/PhabricatorObjectQuery.php @@ -0,0 +1,85 @@ +phids = $phids; + return $this; + } + + public function withNames(array $names) { + $this->names = $names; + return $this; + } + + public function loadPage() { + $types = PhabricatorPHIDType::getAllTypes(); + + $this->namedResults = $this->loadObjectsByName($types); + + return $this->loadObjectsByPHID($types) + + mpull($this->namedResults, null, 'getPHID'); + } + + public function getNamedResults() { + if ($this->namedResults === null) { + throw new Exception("Call execute() before getNamedResults()!"); + } + return $this->namedResults; + } + + private function loadObjectsByName(array $types) { + $names = $this->names; + if (!$names) { + return array(); + } + + $groups = array(); + foreach ($names as $name) { + foreach ($types as $type => $type_impl) { + if (!$type_impl->canLoadNamedObject($name)) { + continue; + } + $groups[$type][] = $name; + break; + } + } + + $results = array(); + foreach ($groups as $type => $group) { + $results += $types[$type]->loadNamedObjects($this, $group); + } + + return $results; + } + + private function loadObjectsByPHID(array $types) { + $phids = $this->phids; + if (!$phids) { + return array(); + } + + $groups = array(); + foreach ($phids as $phid) { + $type = phid_get_type($phid); + $groups[$type][] = $phid; + } + + $results = array(); + foreach ($groups as $type => $group) { + if (isset($types[$type])) { + $objects = $types[$type]->loadObjects($this, $group); + $results += mpull($objects, null, 'getPHID'); + } + } + + return $results; + } + +} diff --git a/src/applications/phid/storage/PhabricatorPHID.php b/src/applications/phid/storage/PhabricatorPHID.php index 5f32bd15ab..09ba2f1887 100644 --- a/src/applications/phid/storage/PhabricatorPHID.php +++ b/src/applications/phid/storage/PhabricatorPHID.php @@ -25,6 +25,18 @@ final class PhabricatorPHID { } public static function fromObjectName($name, PhabricatorUser $viewer) { + $query = id(new PhabricatorObjectQuery()) + ->setViewer($viewer) + ->withNames(array($name)); + $query->execute(); + + $objects = $query->getNamedResults(); + if ($objects) { + return head($objects)->getPHID(); + } + + /// TODO: Destroy this legacy stuff. + $object = null; $match = null; if (preg_match('/^PHID-[A-Z]+-.{20}$/', $name)) { diff --git a/src/applications/phid/type/PhabricatorPHIDType.php b/src/applications/phid/type/PhabricatorPHIDType.php new file mode 100644 index 0000000000..bae42fd53d --- /dev/null +++ b/src/applications/phid/type/PhabricatorPHIDType.php @@ -0,0 +1,57 @@ +setAncestorClass(__CLASS__) + ->loadObjects(); + + $map = array(); + $original = array(); + foreach ($objects as $object) { + $type = $object->getTypeConstant(); + if (isset($map[$type])) { + $that_class = $original[$type]; + $this_class = get_class($object); + throw new Exception( + "Two PhabricatorPHIDType classes ({$that_class}, {$this_class}) ". + "both handle PHID type '{$type}'. A type may be handled by only ". + "one class."); + } + + $original[$type] = get_class($object); + $map[$type] = $object; + } + + $types = $map; + } + + return $types; + } + +} diff --git a/src/applications/slowvote/phid/PhabricatorSlowvotePHIDTypePoll.php b/src/applications/slowvote/phid/PhabricatorSlowvotePHIDTypePoll.php new file mode 100644 index 0000000000..56470d07af --- /dev/null +++ b/src/applications/slowvote/phid/PhabricatorSlowvotePHIDTypePoll.php @@ -0,0 +1,62 @@ +setViewer($query->getViewer()) + ->withPHIDs($phids) + ->execute(); + } + + public function loadHandles(array $phids, array $objects) { + throw new Exception("TODO"); + } + + public function canLoadNamedObject($name) { + return preg_match('/^V\d*[1-9]\d*$/i', $name); + } + + public function loadNamedObjects( + PhabricatorObjectQuery $query, + array $names) { + + $id_map = array(); + foreach ($names as $name) { + $id = (int)substr($name, 1); + $id_map[$id][] = $name; + } + + $objects = id(new PhabricatorSlowvoteQuery()) + ->setViewer($query->getViewer()) + ->withIDs(array_keys($id_map)) + ->execute(); + + $results = array(); + foreach ($objects as $id => $object) { + foreach (idx($id_map, $id, array()) as $name) { + $results[$name] = $object; + } + } + + return $results; + } + +} diff --git a/src/applications/slowvote/storage/PhabricatorSlowvotePoll.php b/src/applications/slowvote/storage/PhabricatorSlowvotePoll.php index e8d61448e5..3e3f8ad343 100644 --- a/src/applications/slowvote/storage/PhabricatorSlowvotePoll.php +++ b/src/applications/slowvote/storage/PhabricatorSlowvotePoll.php @@ -36,7 +36,7 @@ final class PhabricatorSlowvotePoll extends PhabricatorSlowvoteDAO public function generatePHID() { return PhabricatorPHID::generateNewPHID( - PhabricatorPHIDConstants::PHID_TYPE_POLL); + PhabricatorSlowvotePHIDTypePoll::TYPECONST); } public function getOptions() { diff --git a/src/applications/slowvote/storage/PhabricatorSlowvoteTransaction.php b/src/applications/slowvote/storage/PhabricatorSlowvoteTransaction.php index d458e58c66..ec9f090f69 100644 --- a/src/applications/slowvote/storage/PhabricatorSlowvoteTransaction.php +++ b/src/applications/slowvote/storage/PhabricatorSlowvoteTransaction.php @@ -13,7 +13,7 @@ final class PhabricatorSlowvoteTransaction } public function getApplicationTransactionType() { - return PhabricatorPHIDConstants::PHID_TYPE_POLL; + return PhabricatorSlowvotePHIDTypePoll::TYPECONST; } public function getApplicationTransactionCommentObject() { diff --git a/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php b/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php index 5061735a2d..6321b0a5e4 100644 --- a/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php +++ b/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php @@ -144,6 +144,15 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants { } public static function establishConnection($phid_type, $conn_type) { + $map = PhabricatorPHIDType::getAllTypes(); + if (isset($map[$phid_type])) { + $type = $map[$phid_type]; + $object = $type->newObject(); + if ($object) { + return $object->establishConnection($conn_type); + } + } + static $class_map = array( PhabricatorPHIDConstants::PHID_TYPE_TASK => 'ManiphestTask', PhabricatorPHIDConstants::PHID_TYPE_CMIT => 'PhabricatorRepository', @@ -167,7 +176,6 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants { PhabricatorPHIDConstants::PHID_TYPE_CHRG => 'PhortuneCharge', PhabricatorPHIDConstants::PHID_TYPE_XOBJ => 'DoorkeeperExternalObject', PhabricatorPHIDConstants::PHID_TYPE_LEGD => 'LegalpadDocument', - PhabricatorPHIDConstants::PHID_TYPE_POLL => 'PhabricatorSlowvotePoll', ); $class = idx($class_map, $phid_type);