From 08966ba311cb85a5b7d1b35d6ed13a42941fc871 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 5 Jun 2015 11:21:08 -0700 Subject: [PATCH] Begin modularizing fields in SearchEngines Summary: Ref T8441. Ref T7715. Add a layer of indirection to fields in search engines. This will allow us to: - Simplify SearchEngine code, which has collected a lot of duplication around expressing what is effectively field types. - Automatically add fields like "Spaces" and "Projects" (primary driver for T8441). - Reorder or hide fields (not sure if we really want to do this, but it seems plausible, and this will let us play around with it, at least). - Drive Conduit Query methods via SearchEngines, so the same code specifies both the search UI and the `application.query` endpoint (primary driver in T7715). Test Plan: - Searched for stuff in Paste, everything behaved exaclty like it used to (except that I removed the "no language" checkbox, which seemed like fluff from a bygone era). - Searched for stuff in other applications, saw no changes. - Hit date field errors. - Used query strings to specify values. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T7715, T8441 Differential Revision: https://secure.phabricator.com/D13169 --- src/__phutil_library_map__.php | 11 + .../query/PhabricatorPasteSearchEngine.php | 77 ++---- .../PhabricatorApplicationSearchEngine.php | 55 +++- .../field/PhabricatorSearchDateField.php | 37 +++ .../search/field/PhabricatorSearchField.php | 260 ++++++++++++++++++ .../PhabricatorSearchStringListField.php | 22 ++ .../field/PhabricatorSearchTokenizerField.php | 21 ++ .../field/PhabricatorSearchUsersField.php | 55 ++++ 8 files changed, 472 insertions(+), 66 deletions(-) create mode 100644 src/applications/search/field/PhabricatorSearchDateField.php create mode 100644 src/applications/search/field/PhabricatorSearchField.php create mode 100644 src/applications/search/field/PhabricatorSearchStringListField.php create mode 100644 src/applications/search/field/PhabricatorSearchTokenizerField.php create mode 100644 src/applications/search/field/PhabricatorSearchUsersField.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 4b7222da1b..d660977e49 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2503,6 +2503,7 @@ phutil_register_library_map(array( 'PhabricatorSearchController' => 'applications/search/controller/PhabricatorSearchController.php', 'PhabricatorSearchDAO' => 'applications/search/storage/PhabricatorSearchDAO.php', 'PhabricatorSearchDatasource' => 'applications/search/typeahead/PhabricatorSearchDatasource.php', + 'PhabricatorSearchDateField' => 'applications/search/field/PhabricatorSearchDateField.php', 'PhabricatorSearchDeleteController' => 'applications/search/controller/PhabricatorSearchDeleteController.php', 'PhabricatorSearchDocument' => 'applications/search/storage/document/PhabricatorSearchDocument.php', 'PhabricatorSearchDocumentField' => 'applications/search/storage/document/PhabricatorSearchDocumentField.php', @@ -2513,6 +2514,7 @@ phutil_register_library_map(array( 'PhabricatorSearchDocumentTypeDatasource' => 'applications/search/typeahead/PhabricatorSearchDocumentTypeDatasource.php', 'PhabricatorSearchEditController' => 'applications/search/controller/PhabricatorSearchEditController.php', 'PhabricatorSearchEngine' => 'applications/search/engine/PhabricatorSearchEngine.php', + 'PhabricatorSearchField' => 'applications/search/field/PhabricatorSearchField.php', 'PhabricatorSearchHovercardController' => 'applications/search/controller/PhabricatorSearchHovercardController.php', 'PhabricatorSearchIndexer' => 'applications/search/index/PhabricatorSearchIndexer.php', 'PhabricatorSearchManagementIndexWorkflow' => 'applications/search/management/PhabricatorSearchManagementIndexWorkflow.php', @@ -2523,6 +2525,9 @@ phutil_register_library_map(array( 'PhabricatorSearchRelationship' => 'applications/search/constants/PhabricatorSearchRelationship.php', 'PhabricatorSearchResultView' => 'applications/search/view/PhabricatorSearchResultView.php', 'PhabricatorSearchSelectController' => 'applications/search/controller/PhabricatorSearchSelectController.php', + 'PhabricatorSearchStringListField' => 'applications/search/field/PhabricatorSearchStringListField.php', + 'PhabricatorSearchTokenizerField' => 'applications/search/field/PhabricatorSearchTokenizerField.php', + 'PhabricatorSearchUsersField' => 'applications/search/field/PhabricatorSearchUsersField.php', 'PhabricatorSearchWorker' => 'applications/search/worker/PhabricatorSearchWorker.php', 'PhabricatorSecurityConfigOptions' => 'applications/config/option/PhabricatorSecurityConfigOptions.php', 'PhabricatorSecuritySetupCheck' => 'applications/config/check/PhabricatorSecuritySetupCheck.php', @@ -4679,6 +4684,7 @@ phutil_register_library_map(array( 'PhabricatorApplicationPanelController' => 'PhabricatorApplicationsController', 'PhabricatorApplicationQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorApplicationSearchController' => 'PhabricatorSearchBaseController', + 'PhabricatorApplicationSearchEngine' => 'Phobject', 'PhabricatorApplicationStatusView' => 'AphrontView', 'PhabricatorApplicationTransaction' => array( 'PhabricatorLiskDAO', @@ -5995,6 +6001,7 @@ phutil_register_library_map(array( 'PhabricatorSearchController' => 'PhabricatorSearchBaseController', 'PhabricatorSearchDAO' => 'PhabricatorLiskDAO', 'PhabricatorSearchDatasource' => 'PhabricatorTypeaheadCompositeDatasource', + 'PhabricatorSearchDateField' => 'PhabricatorSearchField', 'PhabricatorSearchDeleteController' => 'PhabricatorSearchBaseController', 'PhabricatorSearchDocument' => 'PhabricatorSearchDAO', 'PhabricatorSearchDocumentField' => 'PhabricatorSearchDAO', @@ -6004,6 +6011,7 @@ phutil_register_library_map(array( 'PhabricatorSearchDocumentRelationship' => 'PhabricatorSearchDAO', 'PhabricatorSearchDocumentTypeDatasource' => 'PhabricatorTypeaheadDatasource', 'PhabricatorSearchEditController' => 'PhabricatorSearchBaseController', + 'PhabricatorSearchField' => 'Phobject', 'PhabricatorSearchHovercardController' => 'PhabricatorSearchBaseController', 'PhabricatorSearchManagementIndexWorkflow' => 'PhabricatorSearchManagementWorkflow', 'PhabricatorSearchManagementInitWorkflow' => 'PhabricatorSearchManagementWorkflow', @@ -6012,6 +6020,9 @@ phutil_register_library_map(array( 'PhabricatorSearchPreferencesSettingsPanel' => 'PhabricatorSettingsPanel', 'PhabricatorSearchResultView' => 'AphrontView', 'PhabricatorSearchSelectController' => 'PhabricatorSearchBaseController', + 'PhabricatorSearchStringListField' => 'PhabricatorSearchField', + 'PhabricatorSearchTokenizerField' => 'PhabricatorSearchField', + 'PhabricatorSearchUsersField' => 'PhabricatorSearchTokenizerField', 'PhabricatorSearchWorker' => 'PhabricatorWorker', 'PhabricatorSecurityConfigOptions' => 'PhabricatorApplicationConfigOptions', 'PhabricatorSecuritySetupCheck' => 'PhabricatorSetupCheck', diff --git a/src/applications/paste/query/PhabricatorPasteSearchEngine.php b/src/applications/paste/query/PhabricatorPasteSearchEngine.php index 45dad0799b..4b4b621895 100644 --- a/src/applications/paste/query/PhabricatorPasteSearchEngine.php +++ b/src/applications/paste/query/PhabricatorPasteSearchEngine.php @@ -11,24 +11,6 @@ final class PhabricatorPasteSearchEngine return 'PhabricatorPasteApplication'; } - public function buildSavedQueryFromRequest(AphrontRequest $request) { - $saved = new PhabricatorSavedQuery(); - $saved->setParameter( - 'authorPHIDs', - $this->readUsersFromRequest($request, 'authors')); - - $languages = $request->getStrList('languages'); - if ($request->getBool('noLanguage')) { - $languages[] = null; - } - $saved->setParameter('languages', $languages); - - $saved->setParameter('createdStart', $request->getStr('createdStart')); - $saved->setParameter('createdEnd', $request->getStr('createdEnd')); - - return $saved; - } - public function buildQueryFromSavedQuery(PhabricatorSavedQuery $saved) { $query = id(new PhabricatorPasteQuery()) ->needContent(true) @@ -49,49 +31,22 @@ final class PhabricatorPasteSearchEngine return $query; } - public function buildSearchForm( - AphrontFormView $form, - PhabricatorSavedQuery $saved_query) { - $author_phids = $saved_query->getParameter('authorPHIDs', array()); - - $languages = $saved_query->getParameter('languages', array()); - $no_language = false; - foreach ($languages as $key => $language) { - if ($language === null) { - $no_language = true; - unset($languages[$key]); - continue; - } - } - - $form - ->appendControl( - id(new AphrontFormTokenizerControl()) - ->setDatasource(new PhabricatorPeopleDatasource()) - ->setName('authors') - ->setLabel(pht('Authors')) - ->setValue($author_phids)) - ->appendChild( - id(new AphrontFormTextControl()) - ->setName('languages') - ->setLabel(pht('Languages')) - ->setValue(implode(', ', $languages))) - ->appendChild( - id(new AphrontFormCheckboxControl()) - ->addCheckbox( - 'noLanguage', - 1, - pht('Find Pastes with no specified language.'), - $no_language)); - - $this->buildDateRange( - $form, - $saved_query, - 'createdStart', - pht('Created After'), - 'createdEnd', - pht('Created Before')); - + protected function buildCustomSearchFields() { + return array( + id(new PhabricatorSearchUsersField()) + ->setAliases(array('authors')) + ->setKey('authorPHIDs') + ->setLabel(pht('Authors')), + id(new PhabricatorSearchStringListField()) + ->setKey('languages') + ->setLabel(pht('Languages')), + id(new PhabricatorSearchDateField()) + ->setKey('createdStart') + ->setLabel(pht('Created After')), + id(new PhabricatorSearchDateField()) + ->setKey('createdEnd') + ->setLabel(pht('Created Before')), + ); } protected function getURI($path) { diff --git a/src/applications/search/engine/PhabricatorApplicationSearchEngine.php b/src/applications/search/engine/PhabricatorApplicationSearchEngine.php index 1d81cdface..7891ef5174 100644 --- a/src/applications/search/engine/PhabricatorApplicationSearchEngine.php +++ b/src/applications/search/engine/PhabricatorApplicationSearchEngine.php @@ -14,7 +14,7 @@ * @task exec Paging and Executing Queries * @task render Rendering Results */ -abstract class PhabricatorApplicationSearchEngine { +abstract class PhabricatorApplicationSearchEngine extends Phobject { private $application; private $viewer; @@ -69,8 +69,20 @@ abstract class PhabricatorApplicationSearchEngine { * @param AphrontRequest The search request. * @return PhabricatorSavedQuery */ - abstract public function buildSavedQueryFromRequest( - AphrontRequest $request); + public function buildSavedQueryFromRequest(AphrontRequest $request) { + $fields = $this->buildSearchFields(); + $viewer = $this->requireViewer(); + + $saved = new PhabricatorSavedQuery(); + foreach ($fields as $field) { + $field->setViewer($viewer); + + $value = $field->readValueFromRequest($request); + $saved->setParameter($field->getKey(), $value); + } + + return $saved; + } /** * Executes the saved query. @@ -88,9 +100,42 @@ abstract class PhabricatorApplicationSearchEngine { * @param PhabricatorSavedQuery The query from which to build the form. * @return void */ - abstract public function buildSearchForm( + public function buildSearchForm( AphrontFormView $form, - PhabricatorSavedQuery $query); + PhabricatorSavedQuery $saved) { + + $fields = $this->buildSearchFields(); + $viewer = $this->requireViewer(); + + foreach ($fields as $field) { + $field->setViewer($viewer); + $field->readValueFromSavedQuery($saved); + } + + foreach ($fields as $field) { + foreach ($field->getErrors() as $error) { + $this->addError(last($error)); + } + } + + foreach ($fields as $field) { + $field->appendToForm($form); + } + } + + protected function buildSearchFields() { + $fields = array(); + + foreach ($this->buildCustomSearchFields() as $field) { + $fields[] = $field; + } + + return $fields; + } + + protected function buildCustomSearchFields() { + throw new PhutilMethodNotImplementedException(); + } public function getErrors() { return $this->errors; diff --git a/src/applications/search/field/PhabricatorSearchDateField.php b/src/applications/search/field/PhabricatorSearchDateField.php new file mode 100644 index 0000000000..e7568f7c01 --- /dev/null +++ b/src/applications/search/field/PhabricatorSearchDateField.php @@ -0,0 +1,37 @@ +getStr($key); + } + + protected function validateControlValue($value) { + if (!strlen($value)) { + return; + } + + $epoch = $this->parseDateTime($value); + if ($epoch) { + return; + } + + $this->addError( + pht('Invalid'), + pht('Date value for "%s" can not be parsed.', $this->getLabel())); + } + + protected function parseDateTime($value) { + if (!strlen($value)) { + return null; + } + + return PhabricatorTime::parseLocalTime($value, $this->getViewer()); + } + +} diff --git a/src/applications/search/field/PhabricatorSearchField.php b/src/applications/search/field/PhabricatorSearchField.php new file mode 100644 index 0000000000..75a7453b4a --- /dev/null +++ b/src/applications/search/field/PhabricatorSearchField.php @@ -0,0 +1,260 @@ +key = $key; + return $this; + } + + + /** + * Get the field's key. + * + * @return string Unique key for this field. + * @task config + */ + public function getKey() { + return $this->key; + } + + + /** + * Set a human-readable label for the field. + * + * This should be a short text string, like "Reviewers" or "Colors". + * + * @param string Short, human-readable field label. + * @return this + * task config + */ + public function setLabel($label) { + $this->label = $label; + return $this; + } + + + /** + * Get the field's human-readable label. + * + * @return string Short, human-readable field label. + * @task config + */ + public function getLabel() { + return $this->label; + } + + + /** + * Set the acting viewer. + * + * Engines do not need to do this explicitly; it will be done on their + * behalf by the caller. + * + * @param PhabricatorUser Viewer. + * @return this + * @task config + */ + public function setViewer(PhabricatorUser $viewer) { + $this->viewer = $viewer; + return $this; + } + + + /** + * Get the acting viewer. + * + * @return PhabricatorUser Viewer. + * @task config + */ + public function getViewer() { + return $this->viewer; + } + + + /** + * Provide alternate field aliases, usually more human-readable versions + * of the key. + * + * These aliases can be used when building GET requests, so you can provide + * an alias like `authors` to let users write `&authors=alincoln` instead of + * `&authorPHIDs=alincoln`. This is a little easier to use. + * + * @param list List of aliases for this field. + * @return this + * @task config + */ + public function setAliases(array $aliases) { + $this->aliases = $aliases; + return $this; + } + + + /** + * Get aliases for this field. + * + * @return list List of aliases for this field. + * @task config + */ + public function getAliases() { + return $this->aliases; + } + + +/* -( Handling Errors )---------------------------------------------------- */ + + + protected function addError($short, $long) { + $this->errors[] = array($short, $long); + return $this; + } + + public function getErrors() { + return $this->errors; + } + + protected function validateControlValue($value) { + return; + } + + protected function getShortError() { + $error = head($this->getErrors()); + if ($error) { + return head($error); + } + return null; + } + + +/* -( Reading and Writing Field Values )----------------------------------- */ + + + public function readValueFromRequest(AphrontRequest $request) { + $check = array_merge(array($this->getKey()), $this->getAliases()); + foreach ($check as $key) { + if ($this->getValueExistsInRequest($request, $key)) { + return $this->getValueFromRequest($request, $key); + } + } + return $this->getDefaultValue(); + } + + protected function getValueExistsInRequest(AphrontRequest $request, $key) { + return $request->getExists($key); + } + + abstract protected function getValueFromRequest( + AphrontRequest $request, + $key); + + public function readValueFromSavedQuery(PhabricatorSavedQuery $saved) { + $value = $saved->getParameter( + $this->getKey(), + $this->getDefaultValue()); + $this->value = $this->didReadValueFromSavedQuery($value); + $this->validateControlValue($value); + return $this; + } + + protected function didReadValueFromSavedQuery($value) { + return $value; + } + + public function getValue() { + return $this->value; + } + + protected function getValueForControl() { + return $this->value; + } + + protected function getDefaultValue() { + return null; + } + + +/* -( Rendering Controls )------------------------------------------------- */ + + + abstract protected function newControl(); + + + protected function renderControl() { + // TODO: We should `setError($this->getShortError())` here, but it looks + // terrible in the form layout. + + return $this->newControl() + ->setValue($this->getValueForControl()) + ->setName($this->getKey()) + ->setLabel($this->getLabel()); + } + + public function appendToForm(AphrontFormView $form) { + $form->appendControl($this->renderControl()); + return $this; + } + + +/* -( Utility Methods )----------------------------------------------------- */ + + + /** + * Read a list of items from the request, in either array format or string + * format: + * + * list[]=item1&list[]=item2 + * list=item1,item2 + * + * This provides flexibility when constructing URIs, especially from external + * sources. + * + * @param AphrontRequest Request to read strings from. + * @param string Key to read in the request. + * @return list List of values. + * @task utility + */ + protected function getListFromRequest( + AphrontRequest $request, + $key) { + + $list = $request->getArr($key, null); + if ($list === null) { + $list = $request->getStrList($key); + } + + if (!$list) { + return array(); + } + + return $list; + } +} diff --git a/src/applications/search/field/PhabricatorSearchStringListField.php b/src/applications/search/field/PhabricatorSearchStringListField.php new file mode 100644 index 0000000000..2db384ea90 --- /dev/null +++ b/src/applications/search/field/PhabricatorSearchStringListField.php @@ -0,0 +1,22 @@ +getStrList($key); + } + + protected function newControl() { + return new AphrontFormTextControl(); + } + + protected function getValueForControl() { + return implode(', ', parent::getValueForControl()); + } + +} diff --git a/src/applications/search/field/PhabricatorSearchTokenizerField.php b/src/applications/search/field/PhabricatorSearchTokenizerField.php new file mode 100644 index 0000000000..1feb23f8a4 --- /dev/null +++ b/src/applications/search/field/PhabricatorSearchTokenizerField.php @@ -0,0 +1,21 @@ +getListFromRequest($request, $key); + } + + protected function newControl() { + return id(new AphrontFormTokenizerControl()) + ->setDatasource($this->newDatasource()); + } + + abstract protected function newDatasource(); + +} diff --git a/src/applications/search/field/PhabricatorSearchUsersField.php b/src/applications/search/field/PhabricatorSearchUsersField.php new file mode 100644 index 0000000000..789ffb0a83 --- /dev/null +++ b/src/applications/search/field/PhabricatorSearchUsersField.php @@ -0,0 +1,55 @@ +getListFromRequest($request, $key); + $allow_types = array(); + + $phids = array(); + $names = array(); + $allow_types = array_fuse($allow_types); + $user_type = PhabricatorPeopleUserPHIDType::TYPECONST; + foreach ($list as $item) { + $type = phid_get_type($item); + if ($type == $user_type) { + $phids[] = $item; + } else if (isset($allow_types[$type])) { + $phids[] = $item; + } else { + if (PhabricatorTypeaheadDatasource::isFunctionToken($item)) { + // If this is a function, pass it through unchanged; we'll evaluate + // it later. + $phids[] = $item; + } else { + $names[] = $item; + } + } + } + + if ($names) { + $users = id(new PhabricatorPeopleQuery()) + ->setViewer($this->getViewer()) + ->withUsernames($names) + ->execute(); + foreach ($users as $user) { + $phids[] = $user->getPHID(); + } + $phids = array_unique($phids); + } + + return $phids; + } + +}