r36211 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r36210‎ | r36211 | r36212 >
Date:00:15, 12 June 2008
Author:brion
Status:old
Tags:
Comment:
* (bug 14497) Throw visible errors in installer scripts when SQL files fail due to database permission or other error

Database::sourceFile() was suppressing the standard error reporting, and returning any errors as a string value.
While nice behavior in theory, several years ago, this doesn't work well in practice; most callers assume either that any error will be fatal and halt the script (no error checking) or that an error will return false and any boolean true is success. This caused false positive success, as error conditions returned as strings evaluated to true.
Changed to letting the database object's existing erorr reporting behavior control whether it throws an exception or suppresses the error and returns it as a string -- default behavior will be to throw an exception, making 'can't run CREATE TABLE' errors actually visible to the user trying to install an extension.
For good measure, failure to open the input file also throws an exception to ensure that it will produce a nice visible error instead of a hidden WTF.
Modified paths:
  • /trunk/phase3/RELEASE-NOTES (modified) (history)
  • /trunk/phase3/includes/Database.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/Database.php
——@@ -2116,7 +2116,7 @@
21172117
21182118 /**
21192119 * Read and execute SQL commands from a file.
2120 - * Returns true on success, error string on failure
 2120+ * Returns true on success, error string or exception on failure (depending on object's error ignore settings)
21212121 * @param string $filename File name to open
21222122 * @param callback $lineCallback Optional function called before reading each line
21232123 * @param callback $resultCallback Optional function called for each MySQL result
——@@ -2124,7 +2124,7 @@
21252125 function sourceFile( $filename, $lineCallback = false, $resultCallback = false ) {
21262126 $fp = fopen( $filename, 'r' );
21272127 if ( false === $fp ) {
2128 - return "Could not open \"{$filename}\".\n";
 2128+ throw new MWException( "Could not open \"{$filename}\".\n" );
21292129 }
21302130 $error = $this->sourceStream( $fp, $lineCallback, $resultCallback );
21312131 fclose( $fp );
——@@ -2133,7 +2133,7 @@
21342134
21352135 /**
21362136 * Read and execute commands from an open file handle
2137 - * Returns true on success, error string on failure
 2137+ * Returns true on success, error string or exception on failure (depending on object's error ignore settings)
21382138 * @param string $fp File handle
21392139 * @param callback $lineCallback Optional function called before reading each line
21402140 * @param callback $resultCallback Optional function called for each MySQL result
——@@ -2176,7 +2176,7 @@
21772177 if ( $done ) {
21782178 $cmd = str_replace(';;', ";", $cmd);
21792179 $cmd = $this->replaceVars( $cmd );
2180 - $res = $this->query( $cmd, __METHOD__, true );
 2180+ $res = $this->query( $cmd, __METHOD__ );
21812181 if ( $resultCallback ) {
21822182 call_user_func( $resultCallback, $res );
21832183 }
Index: trunk/phase3/RELEASE-NOTES
——@@ -362,6 +362,8 @@
363363 * We no longer just give up on a missing upload base directory; it's now
364364 created automatically if we have sufficient permissions!
365365 * (bug 14479) MediaWiki:upload-maxfilesize should have a div id wrapper
 366+* (bug 14497) Throw visible errors in installer scripts when SQL files
 367+ fail due to database permission or other error
366368
367369
368370 === API changes in 1.13 ===

Follow-up revisions

RevisionCommit summaryAuthorDate
r65672Actually check sourceFile for failure, showing the error message in the install....platonides21:49, 29 April 2010

Status & tagging log