Fix some Releeph jankiness in interacting with the redesign

Summary: Releeph does some really old sketchy stuff in result rendering. Modernize it, remove the holdover/oldschool interface (these were the last two implementors) and make errors a little softer.

Test Plan: Viewed Releeph products and branches.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

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

src/__phutil_library_map__.php

@@ -1397,7 +1397,6 @@ phutil_register_library_map(array(
     'PhabricatorApplicationSearchEngine' => 'applications/search/engine/PhabricatorApplicationSearchEngine.php',
     'PhabricatorApplicationSearchEngineTestCase' => 'applications/search/engine/__tests__/PhabricatorApplicationSearchEngineTestCase.php',
     'PhabricatorApplicationSearchResultView' => 'applications/search/view/PhabricatorApplicationSearchResultView.php',
-    'PhabricatorApplicationSearchResultsControllerInterface' => 'applications/search/interface/PhabricatorApplicationSearchResultsControllerInterface.php',
     'PhabricatorApplicationStatusView' => 'applications/meta/view/PhabricatorApplicationStatusView.php',
     'PhabricatorApplicationTestCase' => 'applications/base/__tests__/PhabricatorApplicationTestCase.php',
     'PhabricatorApplicationTransaction' => 'applications/transactions/storage/PhabricatorApplicationTransaction.php',
@@ -7256,10 +7255,7 @@ phutil_register_library_map(array(
     'ReleephBranchTemplate' => 'Phobject',
     'ReleephBranchTransaction' => 'PhabricatorApplicationTransaction',
     'ReleephBranchTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
-    'ReleephBranchViewController' => array(
+    'ReleephBranchViewController' => 'ReleephBranchController',
-      'ReleephBranchController',
-      'PhabricatorApplicationSearchResultsControllerInterface',
-    ),
     'ReleephCommitFinder' => 'Phobject',
     'ReleephCommitFinderException' => 'Exception',
     'ReleephCommitMessageFieldSpecification' => 'ReleephFieldSpecification',
@@ -7293,10 +7289,7 @@ phutil_register_library_map(array(
     'ReleephProductSearchEngine' => 'PhabricatorApplicationSearchEngine',
     'ReleephProductTransaction' => 'PhabricatorApplicationTransaction',
     'ReleephProductTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
-    'ReleephProductViewController' => array(
+    'ReleephProductViewController' => 'ReleephProductController',
-      'ReleephProductController',
-      'PhabricatorApplicationSearchResultsControllerInterface',
-    ),
     'ReleephProject' => array(
       'ReleephDAO',
       'PhabricatorApplicationTransactionInterface',

src/applications/releeph/controller/branch/ReleephBranchViewController.php

@@ -1,7 +1,6 @@
 <?php
 
-final class ReleephBranchViewController extends ReleephBranchController
+final class ReleephBranchViewController extends ReleephBranchController {
-  implements PhabricatorApplicationSearchResultsControllerInterface {
 
   private $queryKey;
   private $branchID;
@@ -37,50 +36,6 @@ final class ReleephBranchViewController extends ReleephBranchController
     return $this->delegateToController($controller);
   }
 
-  public function renderResultsList(
-    array $requests,
-    PhabricatorSavedQuery $query) {
-
-    assert_instances_of($requests, 'ReleephRequest');
-    $viewer = $this->getRequest()->getUser();
-
-    // TODO: This is generally a bit sketchy, but we don't do this kind of
-    // thing elsewhere at the moment. For the moment it shouldn't be hugely
-    // costly, and we can batch things later. Generally, this commits fewer
-    // sins than the old code did.
-
-    $engine = id(new PhabricatorMarkupEngine())
-      ->setViewer($viewer);
-
-    $list = array();
-    foreach ($requests as $pull) {
-      $field_list = PhabricatorCustomField::getObjectFields(
-        $pull,
-        PhabricatorCustomField::ROLE_VIEW);
-
-      $field_list
-        ->setViewer($viewer)
-        ->readFieldsFromStorage($pull);
-
-      foreach ($field_list->getFields() as $field) {
-        if ($field->shouldMarkup()) {
-          $field->setMarkupEngine($engine);
-        }
-      }
-
-      $list[] = id(new ReleephRequestView())
-        ->setUser($viewer)
-        ->setCustomFields($field_list)
-        ->setPullRequest($pull)
-        ->setIsListView(true);
-    }
-
-    // This is quite sketchy, but the list has not actually rendered yet, so
-    // this still allows us to batch the markup rendering.
-    $engine->process();
-
-    return $list;
-  }
 
   public function buildSideNavView($for_app = false) {
     $user = $this->getRequest()->getUser();

src/applications/releeph/controller/product/ReleephProductViewController.php

@@ -1,7 +1,6 @@
 <?php
 
-final class ReleephProductViewController extends ReleephProductController
+final class ReleephProductViewController extends ReleephProductController {
-  implements PhabricatorApplicationSearchResultsControllerInterface {
 
   private $productID;
   private $queryKey;
@@ -39,91 +38,6 @@ final class ReleephProductViewController extends ReleephProductController
     return $this->delegateToController($controller);
   }
 
-  public function renderResultsList(
-    array $branches,
-    PhabricatorSavedQuery $saved) {
-    assert_instances_of($branches, 'ReleephBranch');
-
-    $viewer = $this->getRequest()->getUser();
-
-    $products = mpull($branches, 'getProduct');
-    $repo_phids = mpull($products, 'getRepositoryPHID');
-
-    $repos = id(new PhabricatorRepositoryQuery())
-      ->setViewer($viewer)
-      ->withPHIDs($repo_phids)
-      ->execute();
-    $repos = mpull($repos, null, 'getPHID');
-
-    $requests = array();
-    if ($branches) {
-      $requests = id(new ReleephRequestQuery())
-        ->setViewer($viewer)
-        ->withBranchIDs(mpull($branches, 'getID'))
-        ->withStatus(ReleephRequestQuery::STATUS_OPEN)
-        ->execute();
-      $requests = mgroup($requests, 'getBranchID');
-    }
-
-    $list = id(new PHUIObjectItemListView())
-      ->setUser($viewer);
-    foreach ($branches as $branch) {
-      $diffusion_href = null;
-      $repo = idx($repos, $branch->getProduct()->getRepositoryPHID());
-      if ($repo) {
-        $drequest = DiffusionRequest::newFromDictionary(
-          array(
-            'user' => $viewer,
-            'repository' => $repo,
-          ));
-
-        $diffusion_href = $drequest->generateURI(
-          array(
-            'action' => 'branch',
-            'branch' => $branch->getName(),
-          ));
-      }
-
-      $branch_link = $branch->getName();
-      if ($diffusion_href) {
-        $branch_link = phutil_tag(
-          'a',
-          array(
-            'href' => $diffusion_href,
-          ),
-          $branch_link);
-      }
-
-      $item = id(new PHUIObjectItemView())
-        ->setHeader($branch->getDisplayName())
-        ->setHref($this->getApplicationURI('branch/'.$branch->getID().'/'))
-        ->addAttribute($branch_link);
-
-      if (!$branch->getIsActive()) {
-        $item->setDisabled(true);
-      }
-
-      $commit = $branch->getCutPointCommit();
-      if ($commit) {
-        $item->addIcon(
-          'none',
-          phabricator_datetime($commit->getEpoch(), $viewer));
-      }
-
-      $open_count = count(idx($requests, $branch->getID(), array()));
-      if ($open_count) {
-        $item->setStatusIcon('fa-code-fork orange');
-        $item->addIcon(
-          'fa-code-fork',
-          pht('%d Open Pull Request(s)', new PhutilNumber($open_count)));
-      }
-
-      $list->addItem($item);
-    }
-
-    return $list;
-  }
-
   public function buildSideNavView($for_app = false) {
     $viewer = $this->getRequest()->getUser();
     $product = $this->getProduct();

src/applications/releeph/query/ReleephBranchSearchEngine.php

@@ -99,4 +99,96 @@ final class ReleephBranchSearchEngine
     );
   }
 
+  protected function renderResultList(
+    array $branches,
+    PhabricatorSavedQuery $query,
+    array $handles) {
+
+
+    assert_instances_of($branches, 'ReleephBranch');
+
+    $viewer = $this->getRequest()->getUser();
+
+    $products = mpull($branches, 'getProduct');
+    $repo_phids = mpull($products, 'getRepositoryPHID');
+
+    if ($repo_phids) {
+      $repos = id(new PhabricatorRepositoryQuery())
+        ->setViewer($viewer)
+        ->withPHIDs($repo_phids)
+        ->execute();
+      $repos = mpull($repos, null, 'getPHID');
+    } else {
+      $repos = array();
+    }
+
+    $requests = array();
+    if ($branches) {
+      $requests = id(new ReleephRequestQuery())
+        ->setViewer($viewer)
+        ->withBranchIDs(mpull($branches, 'getID'))
+        ->withStatus(ReleephRequestQuery::STATUS_OPEN)
+        ->execute();
+      $requests = mgroup($requests, 'getBranchID');
+    }
+
+    $list = id(new PHUIObjectItemListView())
+      ->setUser($viewer);
+    foreach ($branches as $branch) {
+      $diffusion_href = null;
+      $repo = idx($repos, $branch->getProduct()->getRepositoryPHID());
+      if ($repo) {
+        $drequest = DiffusionRequest::newFromDictionary(
+          array(
+            'user' => $viewer,
+            'repository' => $repo,
+          ));
+
+        $diffusion_href = $drequest->generateURI(
+          array(
+            'action' => 'branch',
+            'branch' => $branch->getName(),
+          ));
+      }
+
+      $branch_link = $branch->getName();
+      if ($diffusion_href) {
+        $branch_link = phutil_tag(
+          'a',
+          array(
+            'href' => $diffusion_href,
+          ),
+          $branch_link);
+      }
+
+      $item = id(new PHUIObjectItemView())
+        ->setHeader($branch->getDisplayName())
+        ->setHref($this->getApplicationURI('branch/'.$branch->getID().'/'))
+        ->addAttribute($branch_link);
+
+      if (!$branch->getIsActive()) {
+        $item->setDisabled(true);
+      }
+
+      $commit = $branch->getCutPointCommit();
+      if ($commit) {
+        $item->addIcon(
+          'none',
+          phabricator_datetime($commit->getEpoch(), $viewer));
+      }
+
+      $open_count = count(idx($requests, $branch->getID(), array()));
+      if ($open_count) {
+        $item->setStatusIcon('fa-code-fork orange');
+        $item->addIcon(
+          'fa-code-fork',
+          pht('%d Open Pull Request(s)', new PhutilNumber($open_count)));
+      }
+
+      $list->addItem($item);
+    }
+
+    return id(new PhabricatorApplicationSearchResultView())
+      ->setObjectList($list);
+  }
 }

src/applications/releeph/query/ReleephRequestSearchEngine.php

@@ -172,4 +172,50 @@ final class ReleephRequestSearchEngine
     }
   }
 
+  protected function renderResultList(
+    array $requests,
+    PhabricatorSavedQuery $query,
+    array $handles) {
+
+    assert_instances_of($requests, 'ReleephRequest');
+    $viewer = $this->requireViewer();
+
+    // TODO: This is generally a bit sketchy, but we don't do this kind of
+    // thing elsewhere at the moment. For the moment it shouldn't be hugely
+    // costly, and we can batch things later. Generally, this commits fewer
+    // sins than the old code did.
+
+    $engine = id(new PhabricatorMarkupEngine())
+      ->setViewer($viewer);
+
+    $list = array();
+    foreach ($requests as $pull) {
+      $field_list = PhabricatorCustomField::getObjectFields(
+        $pull,
+        PhabricatorCustomField::ROLE_VIEW);
+
+      $field_list
+        ->setViewer($viewer)
+        ->readFieldsFromStorage($pull);
+
+      foreach ($field_list->getFields() as $field) {
+        if ($field->shouldMarkup()) {
+          $field->setMarkupEngine($engine);
+        }
+      }
+
+      $list[] = id(new ReleephRequestView())
+        ->setUser($viewer)
+        ->setCustomFields($field_list)
+        ->setPullRequest($pull)
+        ->setIsListView(true);
+    }
+
+    // This is quite sketchy, but the list has not actually rendered yet, so
+    // this still allows us to batch the markup rendering.
+    $engine->process();
+
+    return id(new PhabricatorApplicationSearchResultView())
+      ->setContent($list);
+  }
 }

src/applications/search/controller/PhabricatorApplicationSearchController.php

@@ -217,18 +217,16 @@ final class PhabricatorApplicationSearchController
 
         $objects = $engine->executeQuery($query, $pager);
 
-        // TODO: To support Dashboard panels, rendering is moving into
-        // SearchEngines. Move it all the way in and then get rid of this.
-
-        $interface = 'PhabricatorApplicationSearchResultsControllerInterface';
-        if ($parent instanceof $interface) {
-          $list = $parent->renderResultsList($objects, $saved_query);
-        } else {
         $engine->setRequest($request);
+        $list = $engine->renderResults($objects, $saved_query);
 
-          $list = $engine->renderResults(
+        if (!($list instanceof PhabricatorApplicationSearchResultView)) {
-            $objects,
+          throw new Exception(
-            $saved_query);
+            pht(
+              'SearchEngines must render a "%s" object, but this engine '.
+              '(of class "%s") rendered something else.',
+              'PhabricatorApplicationSearchResultView',
+              get_class($engine)));
         }
 
         if ($list->getActions()) {

src/applications/search/engine/PhabricatorApplicationSearchEngine.php

@@ -1038,12 +1038,10 @@ abstract class PhabricatorApplicationSearchEngine extends Phobject {
     return array();
   }
 
-  protected function renderResultList(
+  abstract protected function renderResultList(
     array $objects,
     PhabricatorSavedQuery $query,
-    array $handles) {
+    array $handles);
-    throw new Exception(pht('Not supported here yet!'));
-  }
 
 
 /* -(  Application Search  )------------------------------------------------- */

src/applications/search/interface/PhabricatorApplicationSearchResultsControllerInterface.php

@@ -1,9 +0,0 @@
-<?php
-
-interface PhabricatorApplicationSearchResultsControllerInterface {
-
-  public function renderResultsList(
-    array $items,
-    PhabricatorSavedQuery $query);
-
-}