Skip to content

Commit 00bb0c9

Browse files
author
epriestley
committedSep 6, 2016
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
1 parent 3099601 commit 00bb0c9

File tree

4 files changed

+80
-46
lines changed

4 files changed

+80
-46
lines changed
 

‎src/aphront/configuration/AphrontApplicationConfiguration.php

+50-32
Original file line numberDiff line numberDiff line change
@@ -73,13 +73,26 @@ public static function runHTTPRequest(AphrontHTTPSink $sink) {
7373

7474
$response = PhabricatorSetupCheck::willPreflightRequest();
7575
if ($response) {
76-
PhabricatorStartup::endOutputCapture();
77-
$sink->writeResponse($response);
78-
return;
76+
return self::writeResponse($sink, $response);
7977
}
8078

8179
PhabricatorStartup::beginStartupPhase('env.init');
82-
PhabricatorEnv::initializeWebEnvironment();
80+
81+
try {
82+
PhabricatorEnv::initializeWebEnvironment();
83+
$database_exception = null;
84+
} catch (AphrontInvalidCredentialsQueryException $ex) {
85+
$database_exception = $ex;
86+
} catch (AphrontConnectionQueryException $ex) {
87+
$database_exception = $ex;
88+
}
89+
90+
if ($database_exception) {
91+
$issue = PhabricatorSetupIssue::newDatabaseConnectionIssue(
92+
$database_exception);
93+
$response = PhabricatorSetupCheck::newIssueResponse($issue);
94+
return self::writeResponse($sink, $response);
95+
}
8396

8497
$multimeter->setSampleRate(
8598
PhabricatorEnv::getEnvConfig('debug.sample-rate'));
@@ -111,9 +124,7 @@ public static function runHTTPRequest(AphrontHTTPSink $sink) {
111124

112125
$response = PhabricatorSetupCheck::willProcessRequest();
113126
if ($response) {
114-
PhabricatorStartup::endOutputCapture();
115-
$sink->writeResponse($response);
116-
return;
127+
return self::writeResponse($sink, $response);
117128
}
118129

119130
$host = AphrontRequest::getHTTPHeader('Host');
@@ -256,31 +267,7 @@ public function processRequest(
256267
$response = $controller->willSendResponse($response);
257268
$response->setRequest($request);
258269

259-
$unexpected_output = PhabricatorStartup::endOutputCapture();
260-
if ($unexpected_output) {
261-
$unexpected_output = pht(
262-
"Unexpected output:\n\n%s",
263-
$unexpected_output);
264-
265-
phlog($unexpected_output);
266-
267-
if ($response instanceof AphrontWebpageResponse) {
268-
echo phutil_tag(
269-
'div',
270-
array(
271-
'style' =>
272-
'background: #eeddff;'.
273-
'white-space: pre-wrap;'.
274-
'z-index: 200000;'.
275-
'position: relative;'.
276-
'padding: 8px;'.
277-
'font-family: monospace',
278-
),
279-
$unexpected_output);
280-
}
281-
}
282-
283-
$sink->writeResponse($response);
270+
self::writeResponse($sink, $response);
284271
} catch (Exception $ex) {
285272
if ($original_exception) {
286273
throw $original_exception;
@@ -291,6 +278,37 @@ public function processRequest(
291278
return $response;
292279
}
293280

281+
private static function writeResponse(
282+
AphrontHTTPSink $sink,
283+
AphrontResponse $response) {
284+
285+
$unexpected_output = PhabricatorStartup::endOutputCapture();
286+
if ($unexpected_output) {
287+
$unexpected_output = pht(
288+
"Unexpected output:\n\n%s",
289+
$unexpected_output);
290+
291+
phlog($unexpected_output);
292+
293+
if ($response instanceof AphrontWebpageResponse) {
294+
echo phutil_tag(
295+
'div',
296+
array(
297+
'style' =>
298+
'background: #eeddff;'.
299+
'white-space: pre-wrap;'.
300+
'z-index: 200000;'.
301+
'position: relative;'.
302+
'padding: 8px;'.
303+
'font-family: monospace',
304+
),
305+
$unexpected_output);
306+
}
307+
}
308+
309+
$sink->writeResponse($response);
310+
}
311+
294312

295313
/* -( URI Routing )-------------------------------------------------------- */
296314

‎src/applications/config/check/PhabricatorDatabaseSetupCheck.php

+9-13
Original file line numberDiff line numberDiff line change
@@ -23,21 +23,17 @@ protected function executeChecks() {
2323

2424
try {
2525
queryfx($conn_raw, 'SELECT 1');
26+
$database_exception = null;
27+
} catch (AphrontInvalidCredentialsQueryException $ex) {
28+
$database_exception = $ex;
2629
} catch (AphrontConnectionQueryException $ex) {
27-
$message = pht(
28-
"Unable to connect to MySQL!\n\n".
29-
"%s\n\n".
30-
"Make sure Phabricator and MySQL are correctly configured.",
31-
$ex->getMessage());
30+
$database_exception = $ex;
31+
}
3232

33-
$this->newIssue('mysql.connect')
34-
->setName(pht('Can Not Connect to MySQL'))
35-
->setMessage($message)
36-
->setIsFatal(true)
37-
->addRelatedPhabricatorConfig('mysql.host')
38-
->addRelatedPhabricatorConfig('mysql.port')
39-
->addRelatedPhabricatorConfig('mysql.user')
40-
->addRelatedPhabricatorConfig('mysql.pass');
33+
if ($database_exception) {
34+
$issue = PhabricatorSetupIssue::newDatabaseConnectionIssue(
35+
$database_exception);
36+
$this->addIssue($issue);
4137
return;
4238
}
4339

‎src/applications/config/check/PhabricatorSetupCheck.php

+1-1
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ final public static function willPreflightRequest() {
147147
return null;
148148
}
149149

150-
private static function newIssueResponse(PhabricatorSetupIssue $issue) {
150+
public static function newIssueResponse(PhabricatorSetupIssue $issue) {
151151
$view = id(new PhabricatorSetupIssueView())
152152
->setIssue($issue);
153153

‎src/applications/config/issue/PhabricatorSetupIssue.php

+20
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,26 @@ final class PhabricatorSetupIssue extends Phobject {
2020
private $originalPHPConfigValues = array();
2121
private $links;
2222

23+
public static function newDatabaseConnectionIssue(
24+
AphrontQueryException $ex) {
25+
26+
$message = pht(
27+
"Unable to connect to MySQL!\n\n".
28+
"%s\n\n".
29+
"Make sure Phabricator and MySQL are correctly configured.",
30+
$ex->getMessage());
31+
32+
return id(new self())
33+
->setIssueKey('mysql.connect')
34+
->setName(pht('Can Not Connect to MySQL'))
35+
->setMessage($message)
36+
->setIsFatal(true)
37+
->addRelatedPhabricatorConfig('mysql.host')
38+
->addRelatedPhabricatorConfig('mysql.port')
39+
->addRelatedPhabricatorConfig('mysql.user')
40+
->addRelatedPhabricatorConfig('mysql.pass');
41+
}
42+
2343
public function addCommand($command) {
2444
$this->commands[] = $command;
2545
return $this;

0 commit comments

Comments
 (0)
Failed to load comments.