From 20ce1a905f3a51aff1e9f976db7977862625d99d Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 3 Sep 2015 10:04:17 -0700 Subject: [PATCH] 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 | 6 ++-- .../AphrontApplicationConfiguration.php | 10 +++--- ...AphrontDefaultApplicationConfiguration.php | 16 --------- .../AphrontMalformedRequestException.php | 34 +++++++++++++++++++ .../exception/AphrontUsageException.php | 21 ------------ .../AphrontUnhandledExceptionResponse.php | 18 ++++++++-- .../exception/DiffusionSetupException.php | 8 +---- 7 files changed, 60 insertions(+), 53 deletions(-) create mode 100644 src/aphront/exception/AphrontMalformedRequestException.php delete mode 100644 src/aphront/exception/AphrontUsageException.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 184bd19229..9bde309819 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/aphront/configuration/AphrontApplicationConfiguration.php b/src/aphront/configuration/AphrontApplicationConfiguration.php index c8b092290e..dad223a63f 100644 --- a/src/aphront/configuration/AphrontApplicationConfiguration.php +++ b/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); diff --git a/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php b/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php index 4d5f24eea1..c51aac3749 100644 --- a/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php +++ b/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); diff --git a/src/aphront/exception/AphrontMalformedRequestException.php b/src/aphront/exception/AphrontMalformedRequestException.php new file mode 100644 index 0000000000..a4f265f473 --- /dev/null +++ b/src/aphront/exception/AphrontMalformedRequestException.php @@ -0,0 +1,34 @@ +title = $title; + $this->isUnlogged = $unlogged; + parent::__construct($message); + } + + public function getTitle() { + return $this->title; + } + + public function getIsUnlogged() { + return $this->isUnlogged; + } + +} diff --git a/src/aphront/exception/AphrontUsageException.php b/src/aphront/exception/AphrontUsageException.php deleted file mode 100644 index d242b56dea..0000000000 --- a/src/aphront/exception/AphrontUsageException.php +++ /dev/null @@ -1,21 +0,0 @@ -title = $title; - parent::__construct($message); - } - - public function getTitle() { - return $this->title; - } - -} diff --git a/src/aphront/response/AphrontUnhandledExceptionResponse.php b/src/aphront/response/AphrontUnhandledExceptionResponse.php index c741c35898..efd9d70ead 100644 --- a/src/aphront/response/AphrontUnhandledExceptionResponse.php +++ b/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); diff --git a/src/applications/diffusion/exception/DiffusionSetupException.php b/src/applications/diffusion/exception/DiffusionSetupException.php index 0ce5994b7e..6efaefaf94 100644 --- a/src/applications/diffusion/exception/DiffusionSetupException.php +++ b/src/applications/diffusion/exception/DiffusionSetupException.php @@ -1,9 +1,3 @@