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

@@ -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',

src/applications/conduit/parametertype/ConduitIntListParameterType.php

@@ -18,7 +18,7 @@ final class ConduitIntListParameterType
       }
     }
 
-    return $this->validateIntList($request, $key, $list);
+    return $list;
   }
 
   protected function getParameterTypeName() {

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

src/applications/conduit/query/ConduitResultSearchEngineExtension.php

@@ -10,7 +10,7 @@ final class ConduitResultSearchEngineExtension
   }
 
   public function getExtensionOrder() {
-    return 1000;
+    return 1500;
   }
 
   public function getExtensionName() {

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'),

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

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

src/applications/search/engineextension/PhabricatorIDsSearchEngineExtension.php

@@ -0,0 +1,55 @@
+<?php
+
+final class PhabricatorIDsSearchEngineExtension
+  extends PhabricatorSearchEngineExtension {
+
+  const EXTENSIONKEY = 'ids';
+
+  public function isExtensionEnabled() {
+    return true;
+  }
+
+  public function getExtensionName() {
+    return pht('Supports ID/PHID Queries');
+  }
+
+  public function getExtensionOrder() {
+    return 1000;
+  }
+
+  public function supportsObject($object) {
+    return true;
+  }
+
+  public function getSearchFields($object) {
+    return array(
+      id(new PhabricatorIDsSearchField())
+        ->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']);
+    }
+
+  }
+
+}

src/applications/search/field/PhabricatorIDsSearchField.php

@@ -0,0 +1,30 @@
+<?php
+
+final class PhabricatorIDsSearchField
+  extends PhabricatorSearchField {
+
+  protected function getDefaultValue() {
+    return array();
+  }
+
+  protected function getValueFromRequest(AphrontRequest $request, $key) {
+    return $request->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();
+  }
+
+}

src/applications/search/field/PhabricatorPHIDsSearchField.php

@@ -0,0 +1,30 @@
+<?php
+
+final class PhabricatorPHIDsSearchField
+  extends PhabricatorSearchField {
+
+  protected function getDefaultValue() {
+    return array();
+  }
+
+  protected function getValueFromRequest(AphrontRequest $request, $key) {
+    return $request->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();
+  }
+
+}

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;
   }