From 8ec413b97201332affa1151dde5e05cb0832cb27 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 13 Dec 2015 10:11:40 -0800 Subject: [PATCH] Clean up "ids" and "phids" handling in SearchEngines Summary: Ref T9964. I added several hacks to get these working. Clean them up and pull this into a proper extension. The behavior in the web UI is: - they work in all applications; but - they only show up in the UI if a value is specified. So if you visit `/view/?ids=1,2` you get the field, but normally it's not present. We could refine this later. I'm going to add documentation about how to prefill these forms regardless, which should make this discoverable by reading the documentation. There's one teensey weensey hack: in the API, I push these fields to the top of the table. That one feels OK, since it's purely a convenience/display adjustment. Test Plan: Queried by IDs, reviewed docs. Reviewers: chad Reviewed By: chad Maniphest Tasks: T9964 Differential Revision: https://secure.phabricator.com/D14769 --- src/__phutil_library_map__.php | 6 ++ .../ConduitIntListParameterType.php | 2 +- .../parametertype/ConduitParameterType.php | 2 +- .../ConduitResultSearchEngineExtension.php | 2 +- .../query/ManiphestTaskSearchEngine.php | 3 - .../PhabricatorApplicationSearchEngine.php | 35 ------------ .../PhabricatorSearchEngineAPIMethod.php | 7 +++ .../PhabricatorIDsSearchEngineExtension.php | 55 +++++++++++++++++++ .../field/PhabricatorIDsSearchField.php | 30 ++++++++++ .../field/PhabricatorPHIDsSearchField.php | 30 ++++++++++ .../search/field/PhabricatorSearchField.php | 13 ++++- 11 files changed, 142 insertions(+), 43 deletions(-) create mode 100644 src/applications/search/engineextension/PhabricatorIDsSearchEngineExtension.php create mode 100644 src/applications/search/field/PhabricatorIDsSearchField.php create mode 100644 src/applications/search/field/PhabricatorPHIDsSearchField.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 8539271141..e6eaec9be0 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2349,6 +2349,8 @@ phutil_register_library_map(array( 'PhabricatorHovercardView' => 'view/widget/hovercard/PhabricatorHovercardView.php', 'PhabricatorHunksManagementMigrateWorkflow' => 'applications/differential/management/PhabricatorHunksManagementMigrateWorkflow.php', 'PhabricatorHunksManagementWorkflow' => 'applications/differential/management/PhabricatorHunksManagementWorkflow.php', + 'PhabricatorIDsSearchEngineExtension' => 'applications/search/engineextension/PhabricatorIDsSearchEngineExtension.php', + 'PhabricatorIDsSearchField' => 'applications/search/field/PhabricatorIDsSearchField.php', 'PhabricatorIRCProtocolAdapter' => 'infrastructure/daemon/bot/adapter/PhabricatorIRCProtocolAdapter.php', 'PhabricatorIconRemarkupRule' => 'applications/macro/markup/PhabricatorIconRemarkupRule.php', 'PhabricatorImageMacroRemarkupRule' => 'applications/macro/markup/PhabricatorImageMacroRemarkupRule.php', @@ -2625,6 +2627,7 @@ phutil_register_library_map(array( 'PhabricatorPHIDResolver' => 'applications/phid/resolver/PhabricatorPHIDResolver.php', 'PhabricatorPHIDType' => 'applications/phid/type/PhabricatorPHIDType.php', 'PhabricatorPHIDTypeTestCase' => 'applications/phid/type/__tests__/PhabricatorPHIDTypeTestCase.php', + 'PhabricatorPHIDsSearchField' => 'applications/search/field/PhabricatorPHIDsSearchField.php', 'PhabricatorPHPASTApplication' => 'applications/phpast/application/PhabricatorPHPASTApplication.php', 'PhabricatorPHPConfigSetupCheck' => 'applications/config/check/PhabricatorPHPConfigSetupCheck.php', 'PhabricatorPHPMailerConfigOptions' => 'applications/config/option/PhabricatorPHPMailerConfigOptions.php', @@ -6539,6 +6542,8 @@ phutil_register_library_map(array( 'PhabricatorHovercardView' => 'AphrontView', 'PhabricatorHunksManagementMigrateWorkflow' => 'PhabricatorHunksManagementWorkflow', 'PhabricatorHunksManagementWorkflow' => 'PhabricatorManagementWorkflow', + 'PhabricatorIDsSearchEngineExtension' => 'PhabricatorSearchEngineExtension', + 'PhabricatorIDsSearchField' => 'PhabricatorSearchField', 'PhabricatorIRCProtocolAdapter' => 'PhabricatorProtocolAdapter', 'PhabricatorIconRemarkupRule' => 'PhutilRemarkupRule', 'PhabricatorImageMacroRemarkupRule' => 'PhutilRemarkupRule', @@ -6847,6 +6852,7 @@ phutil_register_library_map(array( 'PhabricatorPHIDResolver' => 'Phobject', 'PhabricatorPHIDType' => 'Phobject', 'PhabricatorPHIDTypeTestCase' => 'PhutilTestCase', + 'PhabricatorPHIDsSearchField' => 'PhabricatorSearchField', 'PhabricatorPHPASTApplication' => 'PhabricatorApplication', 'PhabricatorPHPConfigSetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorPHPMailerConfigOptions' => 'PhabricatorApplicationConfigOptions', diff --git a/src/applications/conduit/parametertype/ConduitIntListParameterType.php b/src/applications/conduit/parametertype/ConduitIntListParameterType.php index a2f4c95534..07c87dcd8a 100644 --- a/src/applications/conduit/parametertype/ConduitIntListParameterType.php +++ b/src/applications/conduit/parametertype/ConduitIntListParameterType.php @@ -18,7 +18,7 @@ final class ConduitIntListParameterType } } - return $this->validateIntList($request, $key, $list); + return $list; } protected function getParameterTypeName() { diff --git a/src/applications/conduit/parametertype/ConduitParameterType.php b/src/applications/conduit/parametertype/ConduitParameterType.php index 2c67886f3e..8f02057c0f 100644 --- a/src/applications/conduit/parametertype/ConduitParameterType.php +++ b/src/applications/conduit/parametertype/ConduitParameterType.php @@ -26,7 +26,7 @@ abstract class ConduitParameterType extends Phobject { final public function getExists(array $request, $key) { - return $this->getValueExists($request, $key); + return $this->getParameterExists($request, $key); } diff --git a/src/applications/conduit/query/ConduitResultSearchEngineExtension.php b/src/applications/conduit/query/ConduitResultSearchEngineExtension.php index 70aa1891c7..f3ba5d1694 100644 --- a/src/applications/conduit/query/ConduitResultSearchEngineExtension.php +++ b/src/applications/conduit/query/ConduitResultSearchEngineExtension.php @@ -10,7 +10,7 @@ final class ConduitResultSearchEngineExtension } public function getExtensionOrder() { - return 1000; + return 1500; } public function getExtensionName() { diff --git a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php index 2165111e3d..8355e8947e 100644 --- a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php +++ b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php @@ -94,9 +94,6 @@ final class ManiphestTaskSearchEngine ->setLabel(pht('Group By')) ->setKey('group') ->setOptions($this->getGroupOptions()), - id(new PhabricatorSearchStringListField()) - ->setLabel(pht('Task IDs')) - ->setKey('ids'), id(new PhabricatorSearchDateField()) ->setLabel(pht('Created After')) ->setKey('createdStart'), diff --git a/src/applications/search/engine/PhabricatorApplicationSearchEngine.php b/src/applications/search/engine/PhabricatorApplicationSearchEngine.php index e8d115d9a2..67caaa1035 100644 --- a/src/applications/search/engine/PhabricatorApplicationSearchEngine.php +++ b/src/applications/search/engine/PhabricatorApplicationSearchEngine.php @@ -1070,27 +1070,9 @@ abstract class PhabricatorApplicationSearchEngine extends Phobject { // These are handled separately for Conduit, so don't show them as // supported. - unset($fields['ids']); - unset($fields['phids']); unset($fields['order']); unset($fields['limit']); - // TODO: Clean these up, shortly. - $fields = array( - 'ids' => id(new PhabricatorSearchDatasourceField()) - ->setKey('ids') - ->setLabel(pht('IDs')) - ->setDescription( - pht('Search for objects with specific IDs.')) - ->setConduitParameterType(new ConduitIntListParameterType()), - 'phids' => id(new PhabricatorSearchDatasourceField()) - ->setKey('phids') - ->setLabel(pht('PHIDs')) - ->setDescription( - pht('Search for objects with specific PHIDs.')) - ->setConduitParameterType(new ConduitPHIDListParameterType()), - ) + $fields; - $viewer = $this->requireViewer(); foreach ($fields as $key => $field) { $field->setViewer($viewer); @@ -1145,7 +1127,6 @@ abstract class PhabricatorApplicationSearchEngine extends Phobject { $query = $this->buildQueryFromSavedQuery($saved_query); $pager = $this->newPagerForSavedQuery($saved_query); - $this->setAutomaticConstraintsForConduit($query, $request, $constraints); $this->setQueryOrderForConduit($query, $request); $this->setPagerLimitForConduit($pager, $request); $this->setPagerOffsetsForConduit($pager, $request); @@ -1222,22 +1203,6 @@ abstract class PhabricatorApplicationSearchEngine extends Phobject { return $extensions; } - private function setAutomaticConstraintsForConduit( - $query, - ConduitAPIRequest $request, - array $constraints) { - - $with_ids = idx($constraints, 'ids'); - if ($with_ids) { - $query->withIDs($with_ids); - } - - $with_phids = idx($constraints, 'phids'); - if ($with_phids) { - $query->withPHIDs($with_phids); - } - } - private function setQueryOrderForConduit($query, ConduitAPIRequest $request) { $order = $request->getValue('order'); if ($order === null) { diff --git a/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php b/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php index df46325079..051061d34c 100644 --- a/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php +++ b/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php @@ -163,6 +163,13 @@ EOTEXT $fields = $engine->getSearchFieldsForConduit(); + // As a convenience, put these fields at the very top, even if the engine + // specifies and alternate display order for the web UI. These fields are + // very important in the API and nearly useless in the web UI. + $fields = array_select_keys( + $fields, + array('ids', 'phids')) + $fields; + $rows = array(); foreach ($fields as $field) { $key = $field->getConduitKey(); diff --git a/src/applications/search/engineextension/PhabricatorIDsSearchEngineExtension.php b/src/applications/search/engineextension/PhabricatorIDsSearchEngineExtension.php new file mode 100644 index 0000000000..6247ba2825 --- /dev/null +++ b/src/applications/search/engineextension/PhabricatorIDsSearchEngineExtension.php @@ -0,0 +1,55 @@ +setKey('ids') + ->setLabel(pht('IDs')) + ->setDescription( + pht('Search for objects with specific IDs.')), + id(new PhabricatorPHIDsSearchField()) + ->setKey('phids') + ->setLabel(pht('PHIDs')) + ->setDescription( + pht('Search for objects with specific PHIDs.')), + ); + } + + public function applyConstraintsToQuery( + $object, + $query, + PhabricatorSavedQuery $saved, + array $map) { + + if ($map['ids']) { + $query->withIDs($map['ids']); + } + + if ($map['phids']) { + $query->withPHIDs($map['phids']); + } + + } + +} diff --git a/src/applications/search/field/PhabricatorIDsSearchField.php b/src/applications/search/field/PhabricatorIDsSearchField.php new file mode 100644 index 0000000000..909cce9c4a --- /dev/null +++ b/src/applications/search/field/PhabricatorIDsSearchField.php @@ -0,0 +1,30 @@ +getStrList($key); + } + + protected function newControl() { + if (strlen($this->getValueForControl())) { + return new AphrontFormTextControl(); + } else { + return null; + } + } + + protected function getValueForControl() { + return implode(', ', parent::getValueForControl()); + } + + protected function newConduitParameterType() { + return new ConduitIntListParameterType(); + } + +} diff --git a/src/applications/search/field/PhabricatorPHIDsSearchField.php b/src/applications/search/field/PhabricatorPHIDsSearchField.php new file mode 100644 index 0000000000..459233c472 --- /dev/null +++ b/src/applications/search/field/PhabricatorPHIDsSearchField.php @@ -0,0 +1,30 @@ +getStrList($key); + } + + protected function newControl() { + if (strlen($this->getValueForControl())) { + return new AphrontFormTextControl(); + } else { + return null; + } + } + + protected function getValueForControl() { + return implode(', ', parent::getValueForControl()); + } + + protected function newConduitParameterType() { + return new ConduitPHIDListParameterType(); + } + +} diff --git a/src/applications/search/field/PhabricatorSearchField.php b/src/applications/search/field/PhabricatorSearchField.php index 380b574f17..9624fe22f9 100644 --- a/src/applications/search/field/PhabricatorSearchField.php +++ b/src/applications/search/field/PhabricatorSearchField.php @@ -273,17 +273,26 @@ abstract class PhabricatorSearchField extends Phobject { protected function renderControl() { + $control = $this->newControl(); + + if (!$control) { + return null; + } + // TODO: We should `setError($this->getShortError())` here, but it looks // terrible in the form layout. - return $this->newControl() + return $control ->setValue($this->getValueForControl()) ->setName($this->getKey()) ->setLabel($this->getLabel()); } public function appendToForm(AphrontFormView $form) { - $form->appendControl($this->renderControl()); + $control = $this->renderControl(); + if ($control !== null) { + $form->appendControl($this->renderControl()); + } return $this; }