Replace AphrontUsageException with AphrontMalformedRequestException

Summary:
Ref T1806. Ref T7173. Context here is that I want to fix "you can not log in to this instance" being a confusing mess with an opaque error. To do this without hacks, I want to:

  - clean up some exception handling behavior (this diff);
  - modularize exception handling (next diff);
  - replace confusing, over-general exceptions with tailored ones in the Phacility cluster, using the new modular stuff.

This cleans up an awkward "AphrontUsageException" which does some weird stuff right now. In particular, it is extensible and extended in one place in Diffusion, but that extension is meaningless.

Realign this as "AphrontMalformedRequestException", which is a better description of what it is and does: raises errors before we can get as far as normal routing and site handling.

Test Plan: Hit some of these exceptions, saw the expected "abandon all hope" error page.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T1806, T7173

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

src/__phutil_library_map__.php

@@ -143,6 +143,7 @@ phutil_register_library_map(array(
     'AphrontJavelinView' => 'view/AphrontJavelinView.php',
     'AphrontKeyboardShortcutsAvailableView' => 'view/widget/AphrontKeyboardShortcutsAvailableView.php',
     'AphrontListFilterView' => 'view/layout/AphrontListFilterView.php',
+    'AphrontMalformedRequestException' => 'aphront/exception/AphrontMalformedRequestException.php',
     'AphrontMoreView' => 'view/layout/AphrontMoreView.php',
     'AphrontMultiColumnView' => 'view/layout/AphrontMultiColumnView.php',
     'AphrontMySQLDatabaseConnectionTestCase' => 'infrastructure/storage/__tests__/AphrontMySQLDatabaseConnectionTestCase.php',
@@ -170,7 +171,6 @@ phutil_register_library_map(array(
     'AphrontTokenizerTemplateView' => 'view/control/AphrontTokenizerTemplateView.php',
     'AphrontTypeaheadTemplateView' => 'view/control/AphrontTypeaheadTemplateView.php',
     'AphrontUnhandledExceptionResponse' => 'aphront/response/AphrontUnhandledExceptionResponse.php',
-    'AphrontUsageException' => 'aphront/exception/AphrontUsageException.php',
     'AphrontView' => 'view/AphrontView.php',
     'AphrontWebpageResponse' => 'aphront/response/AphrontWebpageResponse.php',
     'ArcanistConduitAPIMethod' => 'applications/arcanist/conduit/ArcanistConduitAPIMethod.php',
@@ -3771,6 +3771,7 @@ phutil_register_library_map(array(
     'AphrontJavelinView' => 'AphrontView',
     'AphrontKeyboardShortcutsAvailableView' => 'AphrontView',
     'AphrontListFilterView' => 'AphrontView',
+    'AphrontMalformedRequestException' => 'AphrontException',
     'AphrontMoreView' => 'AphrontView',
     'AphrontMultiColumnView' => 'AphrontView',
     'AphrontMySQLDatabaseConnectionTestCase' => 'PhabricatorTestCase',
@@ -3800,7 +3801,6 @@ phutil_register_library_map(array(
     'AphrontTokenizerTemplateView' => 'AphrontView',
     'AphrontTypeaheadTemplateView' => 'AphrontView',
     'AphrontUnhandledExceptionResponse' => 'AphrontStandaloneHTMLResponse',
-    'AphrontUsageException' => 'AphrontException',
     'AphrontView' => array(
       'Phobject',
       'PhutilSafeHTMLProducerInterface',
@@ -4383,7 +4383,7 @@ phutil_register_library_map(array(
     'DiffusionSearchQueryConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod',
     'DiffusionServeController' => 'DiffusionController',
     'DiffusionSetPasswordSettingsPanel' => 'PhabricatorSettingsPanel',
-    'DiffusionSetupException' => 'AphrontUsageException',
+    'DiffusionSetupException' => 'Exception',
     'DiffusionSubversionSSHWorkflow' => 'DiffusionSSHWorkflow',
     'DiffusionSubversionServeSSHWorkflow' => 'DiffusionSubversionSSHWorkflow',
     'DiffusionSubversionWireProtocol' => 'Phobject',

src/aphront/configuration/AphrontApplicationConfiguration.php

@@ -318,7 +318,7 @@ abstract class AphrontApplicationConfiguration extends Phobject {
           // This is a command line script (probably something like a unit
           // test) so it's fine that we don't have SERVER_ADDR defined.
         } else {
-          throw new AphrontUsageException(
+          throw new AphrontMalformedRequestException(
             pht('No %s', 'SERVER_ADDR'),
             pht(
               'Phabricator is configured to operate in cluster mode, but '.
@@ -330,7 +330,7 @@ abstract class AphrontApplicationConfiguration extends Phobject {
         }
       } else {
         if (!PhabricatorEnv::isClusterAddress($server_addr)) {
-          throw new AphrontUsageException(
+          throw new AphrontMalformedRequestException(
             pht('External Interface'),
             pht(
               'Phabricator is configured in cluster mode and the address '.
@@ -413,12 +413,14 @@ abstract class AphrontApplicationConfiguration extends Phobject {
     if (!$site) {
       $path = $request->getPath();
       $host = $request->getHost();
-      throw new Exception(
+      throw new AphrontMalformedRequestException(
+        pht('Site Not Found'),
         pht(
           'This request asked for "%s" on host "%s", but no site is '.
           'configured which can serve this request.',
           $path,
-          $host));
+          $host),
+        true);
     }
 
     $request->setSite($site);

src/aphront/configuration/AphrontDefaultApplicationConfiguration.php

@@ -210,22 +210,6 @@ class AphrontDefaultApplicationConfiguration
       return $response;
     }
 
-    if ($ex instanceof AphrontUsageException) {
-      $error = new PHUIInfoView();
-      $error->setTitle($ex->getTitle());
-      $error->appendChild($ex->getMessage());
-
-      $view = new PhabricatorStandardPageView();
-      $view->setRequest($this->getRequest());
-      $view->appendChild($error);
-
-      $response = new AphrontWebpageResponse();
-      $response->setContent($view->render());
-      $response->setHTTPResponseCode(500);
-
-      return $response;
-    }
-
     // Always log the unhandled exception.
     phlog($ex);
 

src/aphront/exception/AphrontMalformedRequestException.php

@@ -0,0 +1,34 @@
+<?php
+
+/**
+ * These exceptions are raised when a client submits a malformed request.
+ *
+ * These errors are caught by Aphront itself and occur too early or too
+ * fundamentally in request handling to allow the request to route to a
+ * controller or survive to normal processing.
+ *
+ * These exceptions can be made "unlogged", which will prevent them from being
+ * logged. The intent is that errors which are purely the result of client
+ * failure and of no interest to the server can be raised silently to avoid
+ * cluttering the logs with client errors that are not actionable.
+ */
+final class AphrontMalformedRequestException extends AphrontException {
+
+  private $title;
+  private $isUnlogged;
+
+  public function __construct($title, $message, $unlogged = false) {
+    $this->title = $title;
+    $this->isUnlogged = $unlogged;
+    parent::__construct($message);
+  }
+
+  public function getTitle() {
+    return $this->title;
+  }
+
+  public function getIsUnlogged() {
+    return $this->isUnlogged;
+  }
+
+}

src/aphront/exception/AphrontUsageException.php

@@ -1,21 +0,0 @@
-<?php
-
-/**
- * These exceptions represent user error, and are not logged.
- *
- * @concrete-extensible
- */
-class AphrontUsageException extends AphrontException {
-
-  private $title;
-
-  public function __construct($title, $message) {
-    $this->title = $title;
-    parent::__construct($message);
-  }
-
-  public function getTitle() {
-    return $this->title;
-  }
-
-}

src/aphront/response/AphrontUnhandledExceptionResponse.php

@@ -6,6 +6,20 @@ final class AphrontUnhandledExceptionResponse
   private $exception;
 
   public function setException(Exception $exception) {
+    // Log the exception unless it's specifically a silent malformed request
+    // exception.
+
+    $should_log = true;
+    if ($exception instanceof AphrontMalformedRequestException) {
+      if ($exception->getIsUnlogged()) {
+        $should_log = false;
+      }
+    }
+
+    if ($should_log) {
+      phlog($exception);
+    }
+
     $this->exception = $exception;
     return $this;
   }
@@ -24,7 +38,7 @@ final class AphrontUnhandledExceptionResponse
   protected function getResponseTitle() {
     $ex = $this->exception;
 
-    if ($ex instanceof AphrontUsageException) {
+    if ($ex instanceof AphrontMalformedRequestException) {
       return $ex->getTitle();
     } else {
       return pht('Unhandled Exception');
@@ -38,7 +52,7 @@ final class AphrontUnhandledExceptionResponse
   protected function getResponseBody() {
     $ex = $this->exception;
 
-    if ($ex instanceof AphrontUsageException) {
+    if ($ex instanceof AphrontMalformedRequestException) {
       $title = $ex->getTitle();
     } else {
       $title = get_class($ex);

src/applications/diffusion/exception/DiffusionSetupException.php

@@ -1,9 +1,3 @@
 <?php
 
-final class DiffusionSetupException extends AphrontUsageException {
+final class DiffusionSetupException extends Exception {}
-
-  public function __construct($message) {
-    parent::__construct(pht('Diffusion Setup Exception'), $message);
-  }
-
-}