Modernize Phriction ordering/paging

Summary: Ref T7803. Fixes T7809. Move Phriction away from getReversePaging() / getPagingColumn().

Test Plan: Paged "All Documents", "Updated", and viewed document hierarchy.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: ite-klass, epriestley

Maniphest Tasks: T7809, T7803

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

src/applications/phriction/query/PhrictionDocumentQuery.php

@@ -17,7 +17,6 @@ final class PhrictionDocumentQuery
   const STATUS_OPEN     = 'status-open';
   const STATUS_NONSTUB  = 'status-nonstub';
 
-  private $order        = 'order-created';
   const ORDER_CREATED   = 'order-created';
   const ORDER_UPDATED   = 'order-updated';
   const ORDER_HIERARCHY = 'order-hierarchy';
@@ -63,7 +62,20 @@ final class PhrictionDocumentQuery
   }
 
   public function setOrder($order) {
-    $this->order = $order;
+    switch ($order) {
+      case self::ORDER_CREATED:
+        $this->setOrderVector(array('id'));
+        break;
+      case self::ORDER_UPDATED:
+        $this->setOrderVector(array('updated'));
+        break;
+      case self::ORDER_HIERARCHY:
+        $this->setOrderVector(array('depth', 'title', 'updated'));
+        break;
+      default:
+        throw new Exception(pht('Unknown order "%s".', $order));
+    }
+
     return $this;
   }
 
@@ -71,19 +83,13 @@ final class PhrictionDocumentQuery
     $table = new PhrictionDocument();
     $conn_r = $table->establishConnection('r');
 
-    if ($this->order == self::ORDER_HIERARCHY) {
-      $order_clause = $this->buildHierarchicalOrderClause($conn_r);
-    } else {
-      $order_clause = $this->buildOrderClause($conn_r);
-    }
-
     $rows = queryfx_all(
       $conn_r,
       'SELECT d.* FROM %T d %Q %Q %Q %Q',
       $table->getTableName(),
       $this->buildJoinClause($conn_r),
       $this->buildWhereClause($conn_r),
-      $order_clause,
+      $this->buildOrderClause($conn_r),
       $this->buildLimitClause($conn_r));
 
     $documents = $table->loadAllFromArray($rows);
@@ -186,13 +192,15 @@ final class PhrictionDocumentQuery
 
   private function buildJoinClause(AphrontDatabaseConnection $conn) {
     $join = '';
-    if ($this->order == self::ORDER_HIERARCHY) {
+
+    if ($this->getOrderVector()->containsKey('updated')) {
       $content_dao = new PhrictionContent();
       $join = qsprintf(
         $conn,
         'JOIN %T c ON d.contentID = c.id',
         $content_dao->getTableName());
     }
+
     return $join;
   }
   protected function buildWhereClause(AphrontDatabaseConnection $conn) {
@@ -271,47 +279,58 @@ final class PhrictionDocumentQuery
     return $this->formatWhereClause($where);
   }
 
-  private function buildHierarchicalOrderClause(
+  public function getOrderableColumns() {
-    AphrontDatabaseConnection $conn_r) {
+    return parent::getOrderableColumns() + array(
+      'depth' => array(
+        'table' => 'd',
+        'column' => 'depth',
+        'reverse' => true,
+        'type' => 'int',
+      ),
+      'title' => array(
+        'table' => 'c',
+        'column' => 'title',
+        'reverse' => true,
+        'type' => 'string',
+      ),
+      'updated' => array(
+        'table' => 'd',
+        'column' => 'contentID',
+        'type' => 'int',
+        'unique' => true,
+      ),
+    );
+  }
 
-    if ($this->getBeforeID()) {
+  protected function getPagingValueMap($cursor, array $keys) {
-      return qsprintf(
+    $document = $this->loadCursorObject($cursor);
-        $conn_r,
+
-        'ORDER BY d.depth, c.title, %Q %Q',
+    $map = array(
-        $this->getPagingColumn(),
+      'id' => $document->getID(),
-        $this->getReversePaging() ? 'DESC' : 'ASC');
+      'depth' => $document->getDepth(),
-    } else {
+      'updated' => $document->getContentID(),
-      return qsprintf(
+    );
-        $conn_r,
+
-        'ORDER BY d.depth, c.title, %Q %Q',
+    foreach ($keys as $key) {
-        $this->getPagingColumn(),
+      switch ($key) {
-        $this->getReversePaging() ? 'ASC' : 'DESC');
+        case 'title':
+          $map[$key] = $document->getContent()->getTitle();
+          break;
       }
     }
 
-  protected function getPagingColumn() {
+    return $map;
-    switch ($this->order) {
+  }
-      case self::ORDER_CREATED:
+
-      case self::ORDER_HIERARCHY:
+  protected function willExecuteCursorQuery(
-        return 'd.id';
+    PhabricatorCursorPagedPolicyAwareQuery $query) {
-      case self::ORDER_UPDATED:
+    $vector = $this->getOrderVector();
-        return 'd.contentID';
+
-      default:
+    if ($vector->containsKey('title')) {
-        throw new Exception("Unknown order '{$this->order}'!");
+      $query->needContent(true);
     }
   }
 
-  protected function getPagingValue($result) {
-    switch ($this->order) {
-      case self::ORDER_CREATED:
-      case self::ORDER_HIERARCHY:
-        return $result->getID();
-      case self::ORDER_UPDATED:
-        return $result->getContentID();
-      default:
-        throw new Exception("Unknown order '{$this->order}'!");
-    }
-  }
 
   public function getQueryApplicationClass() {
     return 'PhabricatorPhrictionApplication';

src/applications/phriction/query/PhrictionSearchEngine.php

@@ -14,8 +14,8 @@ final class PhrictionSearchEngine
   public function buildSavedQueryFromRequest(AphrontRequest $request) {
     $saved = new PhabricatorSavedQuery();
 
-    $saved->setParameter('status', $request->getArr('status'));
+    $saved->setParameter('status', $request->getStr('status'));
-    $saved->setParameter('order', $request->getArr('order'));
+    $saved->setParameter('order', $request->getStr('order'));
 
     return $saved;
   }

src/infrastructure/query/order/PhabricatorQueryOrderVector.php

@@ -92,6 +92,10 @@ final class PhabricatorQueryOrderVector
     return implode(', ', $scalars);
   }
 
+  public function containsKey($key) {
+    return isset($this->items[$key]);
+  }
+
 
 /* -(  Iterator Interface  )------------------------------------------------- */
 

src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php

@@ -249,6 +249,8 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
       ->setViewer($this->getPagingViewer())
       ->withIDs(array((int)$cursor));
 
+    $this->willExecuteCursorQuery($query);
+
     $object = $query->executeOne();
     if (!$object) {
       throw new Exception(
@@ -260,6 +262,11 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
     return $object;
   }
 
+  protected function willExecuteCursorQuery(
+    PhabricatorCursorPagedPolicyAwareQuery $query) {
+    return;
+  }
+
 
   /**
    * Simplifies the task of constructing a paging clause across multiple
@@ -446,7 +453,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery
   /**
    * @task order
    */
-  private function getOrderVector() {
+  protected function getOrderVector() {
     if (!$this->orderVector) {
       $vector = $this->getDefaultOrderVector();
       $vector = PhabricatorQueryOrderVector::newFromVector($vector);