From a1338bd2d8897d4f763cd4e94287ebb39b92d201 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 6 Sep 2016 12:52:03 -0700 Subject: [PATCH] (stable) Raise setup warnings immediately when failing to load configuration from the database Summary: Ref T11589. Previously, when we failed to load database configuration we just continued anyway, in order to get to setup checks so we could raise a better error. There was a small chance that this could lead to pages running in a broken state, where ONLY that connection failed and everything else worked. This was accidentally fixed by narrowing the exceptions we continue on in D16489. However, this "fix" meant that users no longer got helpful setup instructions. Instead: - Keep throwing these exceptions: it's bad to continue if we've failed to connect to the database. - However, catch them and turn them into setup errors. - Share all the setup code so these errors and setup check errors work the same way. Test Plan: - Intentionally broke `mysql.host` and `mysql.pass`. - Loaded pages. - Got good setup errors. - Hit normal setup errors too. - Put everything back. - Swapped into cluster mode. - Intentionally broke cluster mode, saw failover to readonly. Reviewers: chad Reviewed By: chad Maniphest Tasks: T11589 Differential Revision: https://secure.phabricator.com/D16501 --- .../AphrontApplicationConfiguration.php | 82 +++++++++++-------- .../check/PhabricatorDatabaseSetupCheck.php | 22 ++--- .../config/check/PhabricatorSetupCheck.php | 2 +- .../config/issue/PhabricatorSetupIssue.php | 20 +++++ 4 files changed, 80 insertions(+), 46 deletions(-) diff --git a/src/aphront/configuration/AphrontApplicationConfiguration.php b/src/aphront/configuration/AphrontApplicationConfiguration.php index 7c5c571b64..5eeba52221 100644 --- a/src/aphront/configuration/AphrontApplicationConfiguration.php +++ b/src/aphront/configuration/AphrontApplicationConfiguration.php @@ -73,13 +73,26 @@ abstract class AphrontApplicationConfiguration extends Phobject { $response = PhabricatorSetupCheck::willPreflightRequest(); if ($response) { - PhabricatorStartup::endOutputCapture(); - $sink->writeResponse($response); - return; + return self::writeResponse($sink, $response); } PhabricatorStartup::beginStartupPhase('env.init'); - PhabricatorEnv::initializeWebEnvironment(); + + try { + PhabricatorEnv::initializeWebEnvironment(); + $database_exception = null; + } catch (AphrontInvalidCredentialsQueryException $ex) { + $database_exception = $ex; + } catch (AphrontConnectionQueryException $ex) { + $database_exception = $ex; + } + + if ($database_exception) { + $issue = PhabricatorSetupIssue::newDatabaseConnectionIssue( + $database_exception); + $response = PhabricatorSetupCheck::newIssueResponse($issue); + return self::writeResponse($sink, $response); + } $multimeter->setSampleRate( PhabricatorEnv::getEnvConfig('debug.sample-rate')); @@ -111,9 +124,7 @@ abstract class AphrontApplicationConfiguration extends Phobject { $response = PhabricatorSetupCheck::willProcessRequest(); if ($response) { - PhabricatorStartup::endOutputCapture(); - $sink->writeResponse($response); - return; + return self::writeResponse($sink, $response); } $host = AphrontRequest::getHTTPHeader('Host'); @@ -256,31 +267,7 @@ abstract class AphrontApplicationConfiguration extends Phobject { $response = $controller->willSendResponse($response); $response->setRequest($request); - $unexpected_output = PhabricatorStartup::endOutputCapture(); - if ($unexpected_output) { - $unexpected_output = pht( - "Unexpected output:\n\n%s", - $unexpected_output); - - phlog($unexpected_output); - - if ($response instanceof AphrontWebpageResponse) { - echo phutil_tag( - 'div', - array( - 'style' => - 'background: #eeddff;'. - 'white-space: pre-wrap;'. - 'z-index: 200000;'. - 'position: relative;'. - 'padding: 8px;'. - 'font-family: monospace', - ), - $unexpected_output); - } - } - - $sink->writeResponse($response); + self::writeResponse($sink, $response); } catch (Exception $ex) { if ($original_exception) { throw $original_exception; @@ -291,6 +278,37 @@ abstract class AphrontApplicationConfiguration extends Phobject { return $response; } + private static function writeResponse( + AphrontHTTPSink $sink, + AphrontResponse $response) { + + $unexpected_output = PhabricatorStartup::endOutputCapture(); + if ($unexpected_output) { + $unexpected_output = pht( + "Unexpected output:\n\n%s", + $unexpected_output); + + phlog($unexpected_output); + + if ($response instanceof AphrontWebpageResponse) { + echo phutil_tag( + 'div', + array( + 'style' => + 'background: #eeddff;'. + 'white-space: pre-wrap;'. + 'z-index: 200000;'. + 'position: relative;'. + 'padding: 8px;'. + 'font-family: monospace', + ), + $unexpected_output); + } + } + + $sink->writeResponse($response); + } + /* -( URI Routing )-------------------------------------------------------- */ diff --git a/src/applications/config/check/PhabricatorDatabaseSetupCheck.php b/src/applications/config/check/PhabricatorDatabaseSetupCheck.php index b99cd37adc..fd318a5fa1 100644 --- a/src/applications/config/check/PhabricatorDatabaseSetupCheck.php +++ b/src/applications/config/check/PhabricatorDatabaseSetupCheck.php @@ -23,21 +23,17 @@ final class PhabricatorDatabaseSetupCheck extends PhabricatorSetupCheck { try { queryfx($conn_raw, 'SELECT 1'); + $database_exception = null; + } catch (AphrontInvalidCredentialsQueryException $ex) { + $database_exception = $ex; } catch (AphrontConnectionQueryException $ex) { - $message = pht( - "Unable to connect to MySQL!\n\n". - "%s\n\n". - "Make sure Phabricator and MySQL are correctly configured.", - $ex->getMessage()); + $database_exception = $ex; + } - $this->newIssue('mysql.connect') - ->setName(pht('Can Not Connect to MySQL')) - ->setMessage($message) - ->setIsFatal(true) - ->addRelatedPhabricatorConfig('mysql.host') - ->addRelatedPhabricatorConfig('mysql.port') - ->addRelatedPhabricatorConfig('mysql.user') - ->addRelatedPhabricatorConfig('mysql.pass'); + if ($database_exception) { + $issue = PhabricatorSetupIssue::newDatabaseConnectionIssue( + $database_exception); + $this->addIssue($issue); return; } diff --git a/src/applications/config/check/PhabricatorSetupCheck.php b/src/applications/config/check/PhabricatorSetupCheck.php index b1f79bdd47..ad779ed63f 100644 --- a/src/applications/config/check/PhabricatorSetupCheck.php +++ b/src/applications/config/check/PhabricatorSetupCheck.php @@ -147,7 +147,7 @@ abstract class PhabricatorSetupCheck extends Phobject { return null; } - private static function newIssueResponse(PhabricatorSetupIssue $issue) { + public static function newIssueResponse(PhabricatorSetupIssue $issue) { $view = id(new PhabricatorSetupIssueView()) ->setIssue($issue); diff --git a/src/applications/config/issue/PhabricatorSetupIssue.php b/src/applications/config/issue/PhabricatorSetupIssue.php index 1c2ff0f99b..53bab14a85 100644 --- a/src/applications/config/issue/PhabricatorSetupIssue.php +++ b/src/applications/config/issue/PhabricatorSetupIssue.php @@ -20,6 +20,26 @@ final class PhabricatorSetupIssue extends Phobject { private $originalPHPConfigValues = array(); private $links; + public static function newDatabaseConnectionIssue( + AphrontQueryException $ex) { + + $message = pht( + "Unable to connect to MySQL!\n\n". + "%s\n\n". + "Make sure Phabricator and MySQL are correctly configured.", + $ex->getMessage()); + + return id(new self()) + ->setIssueKey('mysql.connect') + ->setName(pht('Can Not Connect to MySQL')) + ->setMessage($message) + ->setIsFatal(true) + ->addRelatedPhabricatorConfig('mysql.host') + ->addRelatedPhabricatorConfig('mysql.port') + ->addRelatedPhabricatorConfig('mysql.user') + ->addRelatedPhabricatorConfig('mysql.pass'); + } + public function addCommand($command) { $this->commands[] = $command; return $this;