Allow Controllers to return a wider range of "response-like" objects

Summary:
Ref T1806. Ref T5752. Currently, `handleRequest()` needs to return an `AphrontResponse`, but sometimes it's really convenient to return some other object, like a Dialog, and let that convert into a response elsewhere.

Formalize this and clean up some of the existing hacks for it so there's less custom/magical code in Phabricator-specific classes and more general code in Aphront classes.

More broadly, I want to clean up T5752 before pursuing T9132, since I'm generally happy with how `SearchEngine` works except for how it interacts with side navs / application menus. I want to fix that first so a new Editor (which will have a lot in common with SearchEngine in terms of how controllers interact with it) doesn't make the problem twice as bad.

Test Plan:
  - Loaded a bunch of normal pages.
  - Loaded dialogs.
  - Loaded proxy responses (submitted empty comments in Maniphest).

Reviewers: chad

Reviewed By: chad

Subscribers: joshuaspence

Maniphest Tasks: T1806, T5752

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

src/__phutil_library_map__.php

@@ -158,6 +158,7 @@ phutil_register_library_map(array(
     'AphrontRequest' => 'aphront/AphrontRequest.php',
     'AphrontRequestTestCase' => 'aphront/__tests__/AphrontRequestTestCase.php',
     'AphrontResponse' => 'aphront/response/AphrontResponse.php',
+    'AphrontResponseProducerInterface' => 'aphront/interface/AphrontResponseProducerInterface.php',
     'AphrontRoutingMap' => 'aphront/site/AphrontRoutingMap.php',
     'AphrontRoutingResult' => 'aphront/site/AphrontRoutingResult.php',
     'AphrontSideNavFilterView' => 'view/layout/AphrontSideNavFilterView.php',
@@ -3732,7 +3733,10 @@ phutil_register_library_map(array(
     'AphrontCursorPagerView' => 'AphrontView',
     'AphrontDefaultApplicationConfiguration' => 'AphrontApplicationConfiguration',
     'AphrontDialogResponse' => 'AphrontResponse',
-    'AphrontDialogView' => 'AphrontView',
+    'AphrontDialogView' => array(
+      'AphrontView',
+      'AphrontResponseProducerInterface',
+    ),
     'AphrontException' => 'Exception',
     'AphrontFileResponse' => 'AphrontResponse',
     'AphrontFormCheckboxControl' => 'AphrontFormControl',
@@ -3775,7 +3779,10 @@ phutil_register_library_map(array(
     'AphrontPageView' => 'AphrontView',
     'AphrontPlainTextResponse' => 'AphrontResponse',
     'AphrontProgressBarView' => 'AphrontBarView',
-    'AphrontProxyResponse' => 'AphrontResponse',
+    'AphrontProxyResponse' => array(
+      'AphrontResponse',
+      'AphrontResponseProducerInterface',
+    ),
     'AphrontRedirectResponse' => 'AphrontResponse',
     'AphrontRedirectResponseTestCase' => 'PhabricatorTestCase',
     'AphrontReloadResponse' => 'AphrontRedirectResponse',

src/aphront/AphrontController.php

@@ -24,10 +24,6 @@ abstract class AphrontController extends Phobject {
     return;
   }
 
-  public function didProcessRequest($response) {
-    return $response;
-  }
-
   public function handleRequest(AphrontRequest $request) {
     if (method_exists($this, 'processRequest')) {
       return $this->processRequest();

src/aphront/configuration/AphrontApplicationConfiguration.php

@@ -1,7 +1,8 @@
 <?php
 
 /**
- * @task  routing URI Routing
+ * @task routing URI Routing
+ * @task response Response Handling
  */
 abstract class AphrontApplicationConfiguration extends Phobject {
 
@@ -234,6 +235,7 @@ abstract class AphrontApplicationConfiguration extends Phobject {
       if (!$response) {
         $controller->willProcessRequest($uri_data);
         $response = $controller->handleRequest($request);
+        $this->validateControllerResponse($controller, $response);
       }
     } catch (Exception $ex) {
       $original_exception = $ex;
@@ -241,8 +243,8 @@ abstract class AphrontApplicationConfiguration extends Phobject {
     }
 
     try {
-      $response = $controller->didProcessRequest($response);
-      $response = $this->willSendResponse($response, $controller);
+      $response = $this->produceResponse($request, $response);
+      $response = $controller->willSendResponse($response);
       $response->setRequest($request);
 
       $unexpected_output = PhabricatorStartup::endOutputCapture();
@@ -424,4 +426,148 @@ abstract class AphrontApplicationConfiguration extends Phobject {
     return $site;
   }
 
+
+/* -(  Response Handling  )-------------------------------------------------- */
+
+
+  /**
+   * Tests if a response is of a valid type.
+   *
+   * @param wild Supposedly valid response.
+   * @return bool True if the object is of a valid type.
+   * @task response
+   */
+  private function isValidResponseObject($response) {
+    if ($response instanceof AphrontResponse) {
+      return true;
+    }
+
+    if ($response instanceof AphrontResponseProducerInterface) {
+      return true;
+    }
+
+    return false;
+  }
+
+
+  /**
+   * Verifies that the return value from an @{class:AphrontController} is
+   * of an allowed type.
+   *
+   * @param AphrontController Controller which returned the response.
+   * @param wild Supposedly valid response.
+   * @return void
+   * @task response
+   */
+  private function validateControllerResponse(
+    AphrontController $controller,
+    $response) {
+
+    if ($this->isValidResponseObject($response)) {
+      return;
+    }
+
+    throw new Exception(
+      pht(
+        'Controller "%s" returned an invalid response from call to "%s". '.
+        'This method must return an object of class "%s", or an object '.
+        'which implements the "%s" interface.',
+        get_class($controller),
+        'handleRequest()',
+        'AphrontResponse',
+        'AphrontResponseProducerInterface'));
+  }
+
+
+  /**
+   * Verifies that the erturn value from an
+   * @{class:AphrontResponseProducerInterface} is of an allowed type.
+   *
+   * @param AphrontResponseProducerInterface Object which produced
+   *   this response.
+   * @param wild Supposedly valid response.
+   * @return void
+   * @task response
+   */
+  private function validateProducerResponse(
+    AphrontResponseProducerInterface $producer,
+    $response) {
+
+    if ($this->isValidResponseObject($response)) {
+      return;
+    }
+
+    throw new Exception(
+      pht(
+        'Producer "%s" returned an invalid response from call to "%s". '.
+        'This method must return an object of class "%s", or an object '.
+        'which implements the "%s" interface.',
+        get_class($producer),
+        'produceAphrontResponse()',
+        'AphrontResponse',
+        'AphrontResponseProducerInterface'));
+  }
+
+
+  /**
+   * Resolves a response object into an @{class:AphrontResponse}.
+   *
+   * Controllers are permitted to return actual responses of class
+   * @{class:AphrontResponse}, or other objects which implement
+   * @{interface:AphrontResponseProducerInterface} and can produce a response.
+   *
+   * If a controller returns a response producer, invoke it now and produce
+   * the real response.
+   *
+   * @param AphrontRequest Request being handled.
+   * @param AphrontResponse|AphrontResponseProducerInterface Response, or
+   *   response producer.
+   * @return AphrontResponse Response after any required production.
+   * @task response
+   */
+  private function produceResponse(AphrontRequest $request, $response) {
+    $original = $response;
+
+    // Detect cycles on the exact same objects. It's still possible to produce
+    // infinite responses as long as they're all unique, but we can only
+    // reasonably detect cycles, not guarantee that response production halts.
+
+    $seen = array();
+    while (true) {
+      // NOTE: It is permissible for an object to be both a response and a
+      // response producer. If so, being a producer is "stronger". This is
+      // used by AphrontProxyResponse.
+
+      // If this response is a valid response, hand over the request first.
+      if ($response instanceof AphrontResponse) {
+        $response->setRequest($request);
+      }
+
+      // If this isn't a producer, we're all done.
+      if (!($response instanceof AphrontResponseProducerInterface)) {
+        break;
+      }
+
+      $hash = spl_object_hash($response);
+      if (isset($seen[$hash])) {
+        throw new Exception(
+          pht(
+            'Failure while producing response for object of class "%s": '.
+            'encountered production cycle (identical object, of class "%s", '.
+            'was produced twice).',
+            get_class($original),
+            get_class($response)));
+      }
+
+      $seen[$hash] = true;
+
+      $new_response = $response->produceAphrontResponse();
+      $this->validateProducerResponse($response, $new_response);
+      $response = $new_response;
+    }
+
+    return $response;
+  }
+
+
 }

src/aphront/configuration/AphrontDefaultApplicationConfiguration.php

@@ -273,10 +273,6 @@ class AphrontDefaultApplicationConfiguration
     return $response;
   }
 
-  public function willSendResponse(AphrontResponse $response) {
-    return $response;
-  }
-
   public function build404Controller() {
     return array(new Phabricator404Controller(), array());
   }

src/aphront/interface/AphrontResponseProducerInterface.php

@@ -0,0 +1,24 @@
+<?php
+
+/**
+ * An object can implement this interface to allow it to be returned directly
+ * from an @{class:AphrontController}.
+ *
+ * Normally, controllers must return an @{class:AphrontResponse}. Sometimes,
+ * this is not convenient or requires an awkward API. If it's preferable to
+ * return some other type of object which is equivalent to or describes a
+ * valid response, that object can implement this interface and produce a
+ * response later.
+ */
+interface AphrontResponseProducerInterface {
+
+
+  /**
+   * Produce the equivalent @{class:AphrontResponse} for this object.
+   *
+   * @return AphrontResponse Equivalent response.
+   */
+  public function produceAphrontResponse();
+
+
+}

src/aphront/response/AphrontProxyResponse.php

@@ -8,7 +8,9 @@
  * then constructing a real @{class:AphrontAjaxResponse} in
  * @{method:reduceProxyResponse}.
  */
-abstract class AphrontProxyResponse extends AphrontResponse {
+abstract class AphrontProxyResponse
+  extends AphrontResponse
+  implements AphrontResponseProducerInterface {
 
   private $proxy;
 
@@ -71,4 +73,12 @@ abstract class AphrontProxyResponse extends AphrontResponse {
         'reduceProxyResponse()'));
   }
 
+
+/* -(  AphrontResponseProducerInterface  )----------------------------------- */
+
+
+  public function produceAphrontResponse() {
+    return $this->reduceProxyResponse();
+  }
+
 }

src/applications/base/controller/PhabricatorController.php

@@ -370,28 +370,8 @@ abstract class PhabricatorController extends AphrontController {
     return $this->buildPageResponse($page);
   }
 
-  public function didProcessRequest($response) {
-    // If a bare DialogView is returned, wrap it in a DialogResponse.
-    if ($response instanceof AphrontDialogView) {
-      $response = id(new AphrontDialogResponse())->setDialog($response);
-    }
-
+  public function willSendResponse(AphrontResponse $response) {
     $request = $this->getRequest();
-    $response->setRequest($request);
-
-    $seen = array();
-    while ($response instanceof AphrontProxyResponse) {
-      $hash = spl_object_hash($response);
-      if (isset($seen[$hash])) {
-        $seen[] = get_class($response);
-        throw new Exception(
-          pht('Cycle while reducing proxy responses: %s',
-          implode(' -> ', $seen)));
-      }
-      $seen[$hash] = get_class($response);
-
-      $response = $response->reduceProxyResponse();
-    }
 
     if ($response instanceof AphrontDialogResponse) {
       if (!$request->isAjax() && !$request->isQuicksand()) {

src/view/AphrontDialogView.php

@@ -1,6 +1,8 @@
 <?php
 
-final class AphrontDialogView extends AphrontView {
+final class AphrontDialogView
+  extends AphrontView
+  implements AphrontResponseProducerInterface {
 
   private $title;
   private $shortTitle;
@@ -371,4 +373,13 @@ final class AphrontDialogView extends AphrontView {
     }
   }
 
+
+/* -(  AphrontResponseProducerInterface  )----------------------------------- */
+
+
+  public function produceAphrontResponse() {
+    return id(new AphrontDialogResponse())
+      ->setDialog($this);
+  }
+
 }