Pull legacy revision query status filters out of the main Query class

Summary:
Ref T2543. Currently, Differential uses a set of hard-coded query filters (like "open" and "closed") to query revisions by status (for example, "open" means any of "review, revision, changes planned, accepted [usually]").

In other applications, like Maniphest, we've replaced this with a low level list of the actual statuses, plus higher level convenience UI through tokenizer functions. This basically has all of the benefits of the hard-coded filters with none of the drawbacks, and is generally more flexible.

I'd like to do that in Differential, too, although we'll need to keep the legacy maps around for a while because they're used by `differential.find` and `differential.getrevision`. To prepare for this, pull all the legacy stuff out into a separate class. Then I'll modernize where I can, and we can get rid of this junk some day.

Test Plan: Grepped for `RevisionQuery::STATUS`. Ran queries via Differential UI.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T2543

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

src/__phutil_library_map__.php

@@ -471,6 +471,7 @@ phutil_register_library_map(array(
     'DifferentialJIRAIssuesCommitMessageField' => 'applications/differential/field/DifferentialJIRAIssuesCommitMessageField.php',
     'DifferentialJIRAIssuesField' => 'applications/differential/customfield/DifferentialJIRAIssuesField.php',
     'DifferentialLegacyHunk' => 'applications/differential/storage/DifferentialLegacyHunk.php',
+    'DifferentialLegacyQuery' => 'applications/differential/constants/DifferentialLegacyQuery.php',
     'DifferentialLineAdjustmentMap' => 'applications/differential/parser/DifferentialLineAdjustmentMap.php',
     'DifferentialLintField' => 'applications/differential/customfield/DifferentialLintField.php',
     'DifferentialLintStatus' => 'applications/differential/constants/DifferentialLintStatus.php',
@@ -5447,6 +5448,7 @@ phutil_register_library_map(array(
     'DifferentialJIRAIssuesCommitMessageField' => 'DifferentialCommitMessageCustomField',
     'DifferentialJIRAIssuesField' => 'DifferentialStoredCustomField',
     'DifferentialLegacyHunk' => 'DifferentialHunk',
+    'DifferentialLegacyQuery' => 'Phobject',
     'DifferentialLineAdjustmentMap' => 'Phobject',
     'DifferentialLintField' => 'DifferentialHarbormasterField',
     'DifferentialLintStatus' => 'Phobject',

src/applications/differential/conduit/DifferentialFindConduitAPIMethod.php

@@ -52,12 +52,12 @@ final class DifferentialFindConduitAPIMethod
     switch ($type) {
       case 'open':
         $query
-          ->withStatus(DifferentialRevisionQuery::STATUS_OPEN)
+          ->withStatus(DifferentialLegacyQuery::STATUS_OPEN)
           ->withAuthors($guids);
         break;
       case 'committable':
         $query
-          ->withStatus(DifferentialRevisionQuery::STATUS_ACCEPTED)
+          ->withStatus(DifferentialLegacyQuery::STATUS_ACCEPTED)
           ->withAuthors($guids);
         break;
       case 'revision-ids':

src/applications/differential/conduit/DifferentialQueryConduitAPIMethod.php

@@ -25,12 +25,7 @@ final class DifferentialQueryConduitAPIMethod
     $hash_types = ArcanistDifferentialRevisionHash::getTypes();
     $hash_const = $this->formatStringConstants($hash_types);
 
-    $status_types = array(
+    $status_types = DifferentialLegacyQuery::getAllConstants();
-      DifferentialRevisionQuery::STATUS_ANY,
-      DifferentialRevisionQuery::STATUS_OPEN,
-      DifferentialRevisionQuery::STATUS_ACCEPTED,
-      DifferentialRevisionQuery::STATUS_CLOSED,
-    );
     $status_const = $this->formatStringConstants($status_types);
 
     $order_types = array(

src/applications/differential/constants/DifferentialLegacyQuery.php

@@ -0,0 +1,86 @@
+<?php
+
+final class DifferentialLegacyQuery
+  extends Phobject {
+
+  const STATUS_ANY            = 'status-any';
+  const STATUS_OPEN           = 'status-open';
+  const STATUS_ACCEPTED       = 'status-accepted';
+  const STATUS_NEEDS_REVIEW   = 'status-needs-review';
+  const STATUS_NEEDS_REVISION = 'status-needs-revision';
+  const STATUS_CLOSED         = 'status-closed';
+  const STATUS_ABANDONED      = 'status-abandoned';
+
+  public static function getAllConstants() {
+    return array_keys(self::getMap());
+  }
+
+  public static function getQueryValues($status) {
+    if ($status === self::STATUS_ANY) {
+      return null;
+    }
+
+    $map = self::getMap();
+    if (!isset($map[$status])) {
+      throw new Exception(
+        pht(
+          'Unknown revision status filter constant "%s".',
+          $status));
+    }
+
+    $values = array();
+    foreach ($map[$status] as $status_constant) {
+      $status_object = DifferentialRevisionStatus::newForStatus(
+        $status_constant);
+
+      $legacy_key = $status_object->getLegacyKey();
+      if ($legacy_key !== null) {
+        $values[] = $legacy_key;
+      }
+    }
+
+    return $values;
+  }
+
+  private static function getMap() {
+    $all = array(
+      DifferentialRevisionStatus::NEEDS_REVIEW,
+      DifferentialRevisionStatus::NEEDS_REVISION,
+      DifferentialRevisionStatus::CHANGES_PLANNED,
+      DifferentialRevisionStatus::ACCEPTED,
+      DifferentialRevisionStatus::PUBLISHED,
+      DifferentialRevisionStatus::ABANDONED,
+    );
+
+    $open = array();
+    $closed = array();
+
+    foreach ($all as $status) {
+      $status_object = DifferentialRevisionStatus::newForStatus($status);
+      if ($status_object->isClosedStatus()) {
+        $closed[] = $status_object->getKey();
+      } else {
+        $open[] = $status_object->getKey();
+      }
+    }
+
+    return array(
+      self::STATUS_ANY => $all,
+      self::STATUS_OPEN => $open,
+      self::STATUS_ACCEPTED => array(
+        DifferentialRevisionStatus::ACCEPTED,
+      ),
+      self::STATUS_NEEDS_REVIEW => array(
+        DifferentialRevisionStatus::NEEDS_REVIEW,
+      ),
+      self::STATUS_NEEDS_REVISION => array(
+        DifferentialRevisionStatus::NEEDS_REVISION,
+      ),
+      self::STATUS_CLOSED => $closed,
+      self::STATUS_ABANDONED => array(
+        DifferentialRevisionStatus::ABANDONED,
+      ),
+    );
+  }
+
+}

src/applications/differential/constants/DifferentialRevisionStatus.php

@@ -12,6 +12,14 @@ final class DifferentialRevisionStatus extends Phobject {
   private $key;
   private $spec = array();
 
+  public function getKey() {
+    return $this->key;
+  }
+
+  public function getLegacyKey() {
+    return idx($this->spec, 'legacy');
+  }
+
   public function getIcon() {
     return idx($this->spec, 'icon');
   }
@@ -52,6 +60,18 @@ final class DifferentialRevisionStatus extends Phobject {
     return ($this->key === self::CHANGES_PLANNED);
   }
 
+  public static function newForStatus($status) {
+    $result = new self();
+
+    $map = self::getMap();
+    if (isset($map[$status])) {
+      $result->key = $status;
+      $result->spec = $map[$status];
+    }
+
+    return $result;
+  }
+
   public static function newForLegacyStatus($legacy_status) {
     $result = new self();
 
@@ -135,33 +155,4 @@ final class DifferentialRevisionStatus extends Phobject {
     );
   }
 
-  public static function getClosedStatuses() {
-    $statuses = array(
-      ArcanistDifferentialRevisionStatus::CLOSED,
-      ArcanistDifferentialRevisionStatus::ABANDONED,
-    );
-
-    if (PhabricatorEnv::getEnvConfig('differential.close-on-accept')) {
-      $statuses[] = ArcanistDifferentialRevisionStatus::ACCEPTED;
-    }
-
-    return $statuses;
-  }
-
-  public static function getOpenStatuses() {
-    return array_diff(self::getAllStatuses(), self::getClosedStatuses());
-  }
-
-  public static function getAllStatuses() {
-    return array(
-      ArcanistDifferentialRevisionStatus::NEEDS_REVIEW,
-      ArcanistDifferentialRevisionStatus::NEEDS_REVISION,
-      ArcanistDifferentialRevisionStatus::CHANGES_PLANNED,
-      ArcanistDifferentialRevisionStatus::ACCEPTED,
-      ArcanistDifferentialRevisionStatus::CLOSED,
-      ArcanistDifferentialRevisionStatus::ABANDONED,
-      ArcanistDifferentialRevisionStatus::IN_PREPARATION,
-    );
-  }
-
 }

src/applications/differential/controller/DifferentialDiffViewController.php

@@ -177,7 +177,7 @@ final class DifferentialDiffViewController extends DifferentialController {
     $revisions = id(new DifferentialRevisionQuery())
       ->setViewer($viewer)
       ->withAuthors(array($viewer->getPHID()))
-      ->withStatus(DifferentialRevisionQuery::STATUS_OPEN)
+      ->withStatus(DifferentialLegacyQuery::STATUS_OPEN)
       ->requireCapabilities(
         array(
           PhabricatorPolicyCapability::CAN_VIEW,

src/applications/differential/controller/DifferentialRevisionViewController.php

@@ -808,7 +808,7 @@ final class DifferentialRevisionViewController extends DifferentialController {
 
     $query = id(new DifferentialRevisionQuery())
       ->setViewer($this->getRequest()->getUser())
-      ->withStatus(DifferentialRevisionQuery::STATUS_OPEN)
+      ->withStatus(DifferentialLegacyQuery::STATUS_OPEN)
       ->withUpdatedEpochBetween($recent, null)
       ->setOrder(DifferentialRevisionQuery::ORDER_MODIFIED)
       ->setLimit(10)

src/applications/differential/query/DifferentialRevisionQuery.php

@@ -1,13 +1,6 @@
 <?php
 
 /**
- * Flexible query API for Differential revisions. Example:
- *
- *   // Load open revisions
- *   $revisions = id(new DifferentialRevisionQuery())
- *     ->withStatus(DifferentialRevisionQuery::STATUS_OPEN)
- *     ->execute();
- *
  * @task config   Query Configuration
  * @task exec     Query Execution
  * @task internal Internals
@@ -18,13 +11,6 @@ final class DifferentialRevisionQuery
   private $pathIDs = array();
 
   private $status             = 'status-any';
-  const STATUS_ANY            = 'status-any';
-  const STATUS_OPEN           = 'status-open';
-  const STATUS_ACCEPTED       = 'status-accepted';
-  const STATUS_NEEDS_REVIEW   = 'status-needs-review';
-  const STATUS_NEEDS_REVISION = 'status-needs-revision';
-  const STATUS_CLOSED         = 'status-closed';
-  const STATUS_ABANDONED      = 'status-abandoned';
 
   private $authors = array();
   private $draftAuthors = array();
@@ -149,7 +135,7 @@ final class DifferentialRevisionQuery
 
   /**
    * Filter results to revisions with a given status. Provide a class constant,
-   * such as `DifferentialRevisionQuery::STATUS_OPEN`.
+   * such as `DifferentialLegacyQuery::STATUS_OPEN`.
    *
    * @param const Class STATUS constant, like STATUS_OPEN.
    * @return this
@@ -711,57 +697,12 @@ final class DifferentialRevisionQuery
     // NOTE: Although the status constants are integers in PHP, the column is a
     // string column in MySQL, and MySQL won't use keys on string columns if
     // you put integers in the query.
-
+    $statuses = DifferentialLegacyQuery::getQueryValues($this->status);
-    switch ($this->status) {
+    if ($statuses !== null) {
-      case self::STATUS_ANY:
+      $where[] = qsprintf(
-        break;
+        $conn_r,
-      case self::STATUS_OPEN:
+        'r.status IN (%Ls)',
-        $where[] = qsprintf(
+        $statuses);
-          $conn_r,
-          'r.status IN (%Ls)',
-          DifferentialRevisionStatus::getOpenStatuses());
-        break;
-      case self::STATUS_NEEDS_REVIEW:
-        $where[] = qsprintf(
-          $conn_r,
-          'r.status IN (%Ls)',
-          array(
-            ArcanistDifferentialRevisionStatus::NEEDS_REVIEW,
-          ));
-        break;
-      case self::STATUS_NEEDS_REVISION:
-        $where[] = qsprintf(
-          $conn_r,
-          'r.status IN (%Ls)',
-          array(
-            ArcanistDifferentialRevisionStatus::NEEDS_REVISION,
-          ));
-        break;
-      case self::STATUS_ACCEPTED:
-        $where[] = qsprintf(
-          $conn_r,
-          'r.status IN (%Ls)',
-          array(
-            ArcanistDifferentialRevisionStatus::ACCEPTED,
-          ));
-        break;
-      case self::STATUS_CLOSED:
-        $where[] = qsprintf(
-          $conn_r,
-          'r.status IN (%Ls)',
-          DifferentialRevisionStatus::getClosedStatuses());
-        break;
-      case self::STATUS_ABANDONED:
-        $where[] = qsprintf(
-          $conn_r,
-          'r.status IN (%Ls)',
-          array(
-            ArcanistDifferentialRevisionStatus::ABANDONED,
-          ));
-        break;
-      default:
-        throw new Exception(
-          pht("Unknown revision status filter constant '%s'!", $this->status));
     }
 
     $where[] = $this->buildWhereClauseParts($conn_r);

src/applications/differential/query/DifferentialRevisionSearchEngine.php

@@ -115,7 +115,7 @@ final class DifferentialRevisionSearchEngine
 
         return $query
           ->setParameter('responsiblePHIDs', array($viewer->getPHID()))
-          ->setParameter('status', DifferentialRevisionQuery::STATUS_OPEN)
+          ->setParameter('status', DifferentialLegacyQuery::STATUS_OPEN)
           ->setParameter('bucket', $bucket_key);
       case 'authored':
         return $query
@@ -129,13 +129,13 @@ final class DifferentialRevisionSearchEngine
 
   private function getStatusOptions() {
     return array(
-      DifferentialRevisionQuery::STATUS_ANY            => pht('All'),
+      DifferentialLegacyQuery::STATUS_ANY            => pht('All'),
-      DifferentialRevisionQuery::STATUS_OPEN           => pht('Open'),
+      DifferentialLegacyQuery::STATUS_OPEN           => pht('Open'),
-      DifferentialRevisionQuery::STATUS_ACCEPTED       => pht('Accepted'),
+      DifferentialLegacyQuery::STATUS_ACCEPTED       => pht('Accepted'),
-      DifferentialRevisionQuery::STATUS_NEEDS_REVIEW   => pht('Needs Review'),
+      DifferentialLegacyQuery::STATUS_NEEDS_REVIEW   => pht('Needs Review'),
-      DifferentialRevisionQuery::STATUS_NEEDS_REVISION => pht('Needs Revision'),
+      DifferentialLegacyQuery::STATUS_NEEDS_REVISION => pht('Needs Revision'),
-      DifferentialRevisionQuery::STATUS_CLOSED         => pht('Closed'),
+      DifferentialLegacyQuery::STATUS_CLOSED         => pht('Closed'),
-      DifferentialRevisionQuery::STATUS_ABANDONED      => pht('Abandoned'),
+      DifferentialLegacyQuery::STATUS_ABANDONED      => pht('Abandoned'),
     );
   }
 
@@ -267,7 +267,7 @@ final class DifferentialRevisionSearchEngine
     $blocking_revisions = id(new DifferentialRevisionQuery())
       ->setViewer($viewer)
       ->withPHIDs($revision_phids)
-      ->withStatus(DifferentialRevisionQuery::STATUS_OPEN)
+      ->withStatus(DifferentialLegacyQuery::STATUS_OPEN)
       ->execute();
     $blocking_revisions = mpull($blocking_revisions, null, 'getPHID');
 

src/applications/diffusion/controller/DiffusionBrowseController.php

@@ -1758,7 +1758,7 @@ final class DiffusionBrowseController extends DiffusionController {
     $revisions = id(new DifferentialRevisionQuery())
       ->setViewer($viewer)
       ->withPath($repository->getID(), $path_id)
-      ->withStatus(DifferentialRevisionQuery::STATUS_OPEN)
+      ->withStatus(DifferentialLegacyQuery::STATUS_OPEN)
       ->withUpdatedEpochBetween($recent, null)
       ->setOrder(DifferentialRevisionQuery::ORDER_MODIFIED)
       ->setLimit(10)