Modernize DifferentialRevisionSearchEngine

Summary:
Ref T10939. Ref T4144. This moves the revision SearchEngine to modern code so I can add some kind of bucketing layer on top of it.

This seems to have worked pretty cleanly. One thing is that I removed the ability to search for "pending drafts":

  - This was added in D1927 from a bootcamp task, was an indirect solution to a questionable problem, and almost certainly would not meet the bar today.
  - Later, in D3324, we added the icons to the list. I think this is a better solution in general. In particular, it specifically addressed the query being kind of junky.
  - At the time, Differential had a prebuilt "Drafts" filter. This was removed in D6347 with the move to ApplicationSearch, which simplified the large number of prebuilt filters. Although we got a lot of feedback about that, none requested that the drafts filter be restored.

Test Plan: Searched for responsible users, subscribers, orders, projects, repositories.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4144, T10939

Differential Revision: https://secure.phabricator.com/D15921

src/applications/differential/query/DifferentialRevisionQuery.php

@@ -91,18 +91,6 @@ final class DifferentialRevisionQuery
     return $this;
   }
 
-  /**
-   * Filter results to revisions with comments authored by the given PHIDs.
-   *
-   * @param array List of PHIDs of authors
-   * @return this
-   * @task config
-   */
-  public function withDraftRepliesByAuthors(array $author_phids) {
-    $this->draftAuthors = $author_phids;
-    return $this;
-  }
-
   /**
    * Filter results to revisions which CC one of the listed people. Calling this
    * function will clear anything set by previous calls to @{method:withCCs}.
@@ -239,27 +227,6 @@ final class DifferentialRevisionQuery
   }
 
 
-  /**
-   * Set result ordering. Provide a class constant, such as
-   * `DifferentialRevisionQuery::ORDER_CREATED`.
-   *
-   * @task config
-   */
-  public function setOrder($order_constant) {
-    switch ($order_constant) {
-      case self::ORDER_CREATED:
-        $this->setOrderVector(array('id'));
-        break;
-      case self::ORDER_MODIFIED:
-        $this->setOrderVector(array('updated', 'id'));
-        break;
-      default:
-        throw new Exception(pht('Unknown order "%s".', $order_constant));
-    }
-
-    return $this;
-  }
-
 
   /**
    * Set whether or not the query will load and attach relationships.
@@ -371,6 +338,11 @@ final class DifferentialRevisionQuery
 /* -(  Query Execution  )---------------------------------------------------- */
 
 
+  public function newResultObject() {
+    return new DifferentialRevision();
+  }
+
+
   /**
    * Execute the query as configured, returning matching
    * @{class:DifferentialRevision} objects.
@@ -379,11 +351,9 @@ final class DifferentialRevisionQuery
    * @task exec
    */
   protected function loadPage() {
-    $table = new DifferentialRevision();
-    $conn_r = $table->establishConnection('r');
-
     $data = $this->loadData();
 
+    $table = $this->newResultObject();
     return $table->loadAllFromArray($data);
   }
 
@@ -519,7 +489,7 @@ final class DifferentialRevisionQuery
   }
 
   private function loadData() {
-    $table = new DifferentialRevision();
+    $table = $this->newResultObject();
     $conn_r = $table->establishConnection('r');
 
     $selects = array();
@@ -605,7 +575,7 @@ final class DifferentialRevisionQuery
 
     $joins = $this->buildJoinsClause($conn_r);
     $where = $this->buildWhereClause($conn_r);
-    $group_by = $this->buildGroupByClause($conn_r);
+    $group_by = $this->buildGroupClause($conn_r);
     $having = $this->buildHavingClause($conn_r);
 
     $this->buildingGlobalOrder = false;
@@ -849,19 +819,37 @@ final class DifferentialRevisionQuery
   /**
    * @task internal
    */
-  private function buildGroupByClause($conn_r) {
+  protected function shouldGroupQueryResultRows() {
+
     $join_triggers = array_merge(
       $this->pathIDs,
       $this->ccs,
       $this->reviewers);
 
-    $needs_distinct = (count($join_triggers) > 1);
+    if (count($join_triggers) > 1) {
-
+      return true;
-    if ($needs_distinct) {
-      return 'GROUP BY r.id';
-    } else {
-      return '';
     }
+
+    return parent::shouldGroupQueryResultRows();
+  }
+
+  public function getBuiltinOrders() {
+    $orders = parent::getBuiltinOrders() + array(
+      'updated' => array(
+        'vector' => array('updated', 'id'),
+        'name' => pht('Date Updated (Latest First)'),
+        'aliases' => array(self::ORDER_MODIFIED),
+      ),
+      'outdated' => array(
+        'vector' => array('-updated', '-id'),
+        'name' => pht('Date Updated (Oldest First)'),
+       ),
+    );
+
+    // Alias the "newest" builtin to the historical key for it.
+    $orders['newest']['aliases'][] = self::ORDER_CREATED;
+
+    return $orders;
   }
 
   protected function getDefaultOrderVector() {

src/applications/differential/query/DifferentialRevisionSearchEngine.php

@@ -25,189 +25,68 @@ final class DifferentialRevisionSearchEngine
     return parent::getPageSize($saved);
   }
 
-  public function buildSavedQueryFromRequest(AphrontRequest $request) {
-    $saved = new PhabricatorSavedQuery();
 
-    $saved->setParameter(
+  protected function buildQueryFromParameters(array $map) {
-      'responsiblePHIDs',
+    $query = $this->newQuery();
-      $this->readUsersFromRequest($request, 'responsibles'));
 
-    $saved->setParameter(
+    if ($map['responsiblePHIDs']) {
-      'authorPHIDs',
+      $query->withResponsibleUsers($map['responsiblePHIDs']);
-      $this->readUsersFromRequest($request, 'authors'));
-
-    $saved->setParameter(
-      'reviewerPHIDs',
-      $this->readUsersFromRequest(
-        $request,
-        'reviewers',
-        array(
-          PhabricatorProjectProjectPHIDType::TYPECONST,
-        )));
-
-    $saved->setParameter(
-      'subscriberPHIDs',
-      $this->readSubscribersFromRequest($request, 'subscribers'));
-
-    $saved->setParameter(
-      'repositoryPHIDs',
-      $request->getArr('repositories'));
-
-    $saved->setParameter(
-      'projects',
-      $this->readProjectsFromRequest($request, 'projects'));
-
-    $saved->setParameter(
-      'draft',
-      $request->getBool('draft'));
-
-    $saved->setParameter(
-      'order',
-      $request->getStr('order'));
-
-    $saved->setParameter(
-      'status',
-      $request->getStr('status'));
-
-    return $saved;
-  }
-
-  public function buildQueryFromSavedQuery(PhabricatorSavedQuery $saved) {
-    $query = id(new DifferentialRevisionQuery())
-      ->needFlags(true)
-      ->needDrafts(true)
-      ->needRelationships(true);
-
-    $user_datasource = id(new PhabricatorPeopleUserFunctionDatasource())
-      ->setViewer($this->requireViewer());
-
-    $responsible_phids = $saved->getParameter('responsiblePHIDs', array());
-    $responsible_phids = $user_datasource->evaluateTokens($responsible_phids);
-    if ($responsible_phids) {
-      $query->withResponsibleUsers($responsible_phids);
     }
 
-    $this->setQueryProjects($query, $saved);
+    if ($map['authorPHIDs']) {
-
+      $query->withAuthors($map['authorPHIDs']);
-    $author_phids = $saved->getParameter('authorPHIDs', array());
-    $author_phids = $user_datasource->evaluateTokens($author_phids);
-    if ($author_phids) {
-      $query->withAuthors($author_phids);
     }
 
-    $reviewer_phids = $saved->getParameter('reviewerPHIDs', array());
+    if ($map['reviewerPHIDs']) {
-    if ($reviewer_phids) {
+      $query->withReviewers($map['reviewerPHIDs']);
-      $query->withReviewers($reviewer_phids);
     }
 
-    $sub_datasource = id(new PhabricatorMetaMTAMailableFunctionDatasource())
+    if ($map['repositoryPHIDs']) {
-      ->setViewer($this->requireViewer());
+      $query->withRepositoryPHIDs($map['repositoryPHIDs']);
-    $subscriber_phids = $saved->getParameter('subscriberPHIDs', array());
-    $subscriber_phids = $sub_datasource->evaluateTokens($subscriber_phids);
-    if ($subscriber_phids) {
-      $query->withCCs($subscriber_phids);
     }
 
-    $repository_phids = $saved->getParameter('repositoryPHIDs', array());
+    if ($map['status']) {
-    if ($repository_phids) {
+      $query->withStatus($map['status']);
-      $query->withRepositoryPHIDs($repository_phids);
-    }
-
-    $draft = $saved->getParameter('draft', false);
-    if ($draft && $this->requireViewer()->isLoggedIn()) {
-      $query->withDraftRepliesByAuthors(
-        array($this->requireViewer()->getPHID()));
-    }
-
-    $status = $saved->getParameter('status');
-    if (idx($this->getStatusOptions(), $status)) {
-      $query->withStatus($status);
-    }
-
-    $order = $saved->getParameter('order');
-    if (idx($this->getOrderOptions(), $order)) {
-      $query->setOrder($order);
-    } else {
-      $query->setOrder(DifferentialRevisionQuery::ORDER_CREATED);
     }
 
     return $query;
   }
 
-  public function buildSearchForm(
+  protected function buildCustomSearchFields() {
-    AphrontFormView $form,
+    return array(
-    PhabricatorSavedQuery $saved) {
+      id(new PhabricatorUsersSearchField())
-
+        ->setLabel(pht('Responsible Users'))
-    $responsible_phids = $saved->getParameter('responsiblePHIDs', array());
+        ->setKey('responsiblePHIDs')
-    $author_phids = $saved->getParameter('authorPHIDs', array());
+        ->setAliases(array('responsiblePHID', 'responsibles', 'responsible'))
-    $reviewer_phids = $saved->getParameter('reviewerPHIDs', array());
+        ->setDescription(
-    $subscriber_phids = $saved->getParameter('subscriberPHIDs', array());
+          pht('Find revisions that a given user is responsible for.')),
-    $repository_phids = $saved->getParameter('repositoryPHIDs', array());
+      id(new PhabricatorUsersSearchField())
-    $only_draft = $saved->getParameter('draft', false);
+        ->setLabel(pht('Authors'))
-    $projects = $saved->getParameter('projects', array());
+        ->setKey('authorPHIDs')
-
+        ->setAliases(array('author', 'authors', 'authorPHID'))
-    $form
+        ->setDescription(
-      ->appendControl(
+          pht('Find revisions with specific authors.')),
-        id(new AphrontFormTokenizerControl())
+      id(new PhabricatorSearchDatasourceField())
-          ->setLabel(pht('Responsible Users'))
+        ->setLabel(pht('Reviewers'))
-          ->setName('responsibles')
+        ->setKey('reviewerPHIDs')
-          ->setDatasource(new PhabricatorPeopleUserFunctionDatasource())
+        ->setAliases(array('reviewer', 'reviewers', 'reviewerPHID'))
-          ->setValue($responsible_phids))
+        ->setDatasource(new DiffusionAuditorDatasource())
-      ->appendControl(
+        ->setDescription(
-        id(new AphrontFormTokenizerControl())
+          pht('Find revisions with specific reviewers.')),
-          ->setLabel(pht('Authors'))
+      id(new PhabricatorSearchDatasourceField())
-          ->setName('authors')
+        ->setLabel(pht('Repositories'))
-          ->setDatasource(new PhabricatorPeopleUserFunctionDatasource())
+        ->setKey('repositoryPHIDs')
-          ->setValue($author_phids))
+        ->setAliases(array('repository', 'repositories', 'repositoryPHID'))
-      ->appendControl(
+        ->setDatasource(new DiffusionRepositoryDatasource())
-        id(new AphrontFormTokenizerControl())
+        ->setDescription(
-          ->setLabel(pht('Reviewers'))
+          pht('Find revisions from specific repositories.')),
-          ->setName('reviewers')
+      id(new PhabricatorSearchSelectField())
-          ->setDatasource(new PhabricatorProjectOrUserDatasource())
+        ->setLabel(pht('Status'))
-          ->setValue($reviewer_phids))
+        ->setKey('status')
-      ->appendControl(
+        ->setOptions($this->getStatusOptions())
-        id(new AphrontFormTokenizerControl())
+        ->setDescription(
-          ->setLabel(pht('Subscribers'))
+          pht('Find revisions with particular statuses.')),
-          ->setName('subscribers')
+    );
-          ->setDatasource(new PhabricatorMetaMTAMailableFunctionDatasource())
-          ->setValue($subscriber_phids))
-      ->appendControl(
-        id(new AphrontFormTokenizerControl())
-          ->setLabel(pht('Repositories'))
-          ->setName('repositories')
-          ->setDatasource(new DiffusionRepositoryDatasource())
-          ->setValue($repository_phids))
-      ->appendControl(
-        id(new AphrontFormTokenizerControl())
-          ->setLabel(pht('Tags'))
-          ->setName('projects')
-          ->setDatasource(new PhabricatorProjectLogicalDatasource())
-          ->setValue($projects))
-      ->appendChild(
-        id(new AphrontFormSelectControl())
-          ->setLabel(pht('Status'))
-          ->setName('status')
-          ->setOptions($this->getStatusOptions())
-          ->setValue($saved->getParameter('status')));
-
-    if ($this->requireViewer()->isLoggedIn()) {
-      $form
-        ->appendChild(
-          id(new AphrontFormCheckboxControl())
-            ->addCheckbox(
-              'draft',
-              1,
-              pht('Show only revisions with a draft comment.'),
-              $only_draft));
-    }
-
-    $form
-      ->appendChild(
-        id(new AphrontFormSelectControl())
-          ->setLabel(pht('Order'))
-          ->setName('order')
-          ->setOptions($this->getOrderOptions())
-          ->setValue($saved->getParameter('order')));
   }
 
   protected function getURI($path) {

src/applications/maniphest/query/ManiphestTaskSearchEngine.php

@@ -148,8 +148,7 @@ final class ManiphestTaskSearchEngine
   }
 
   protected function buildQueryFromParameters(array $map) {
-    $query = id(new ManiphestTaskQuery())
+    $query = $this->newQuery();
-      ->needProjectPHIDs(true);
 
     if ($map['assignedPHIDs']) {
       $query->withOwners($map['assignedPHIDs']);