r69128 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r69127‎ | r69128 | r69129 >
Date:02:53, 7 July 2010
Author:mah
Status:resolved (Comments)
Tags:
Comment:
* Add Status::getWarningsArray() to complement Status::getErrorsArray()
* Add Status::getWikiTextArray() to allow different ways of formating a bunch of status messages (e.g. CLI output)
* Clean up messages in CliInstaller, use more i18n
* Use warning messages from Status return object in CLI installer
* Make Installer::isCompiled static so we don't have to create an object just to see that we can't use it.
* Add Installer::addInstallStepFollowing so we don't have MySQLInstaller mucking in its parent's data
Modified paths:
  • /trunk/phase3/includes/Status.php (modified) (history)
  • /trunk/phase3/includes/installer/CliInstaller.php (modified) (history)
  • /trunk/phase3/includes/installer/CliInstallerOutput.php (deleted) (history)
  • /trunk/phase3/includes/installer/Installer.php (modified) (history)
  • /trunk/phase3/includes/installer/InstallerDBType.php (modified) (history)
  • /trunk/phase3/includes/installer/MysqlInstaller.php (modified) (history)
  • /trunk/phase3/includes/installer/OracleInstaller.php (modified) (history)
  • /trunk/phase3/includes/installer/PostgresInstaller.php (modified) (history)
  • /trunk/phase3/includes/installer/SqliteInstaller.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/Status.php
——@@ -179,19 +179,15 @@
180180 }
181181 }
182182 if ( count( $this->errors ) == 1 ) {
183 - $params = array_map( 'wfEscapeWikiText', $this->cleanParams( $this->errors[0]['params'] ) );
184 - $s = wfMsgReal( $this->errors[0]['message'], $params, true, false, false );
 183+ $s = $this->getWikiTextForError( $this->errors[0], $this->errors[0] );
185184 if ( $shortContext ) {
186185 $s = wfMsgNoTrans( $shortContext, $s );
187186 } elseif ( $longContext ) {
188187 $s = wfMsgNoTrans( $longContext, "* $s\n" );
189188 }
190189 } else {
191 - $s = '';
192 - foreach ( $this->errors as $error ) {
193 - $params = array_map( 'wfEscapeWikiText', $this->cleanParams( $error['params'] ) );
194 - $s .= '* ' . wfMsgReal( $error['message'], $params, true, false, false ) . "\n";
195 - }
 190+ $s = '* '. implode("\n* ",
 191+ $this->getWikiTextArray( $this->errors ) ) . "\n";
196192 if ( $longContext ) {
197193 $s = wfMsgNoTrans( $longContext, $s );
198194 } elseif ( $shortContext ) {
——@@ -202,6 +198,41 @@
203199 }
204200
205201 /**
 202+ * Return the wiki text for a single error.
 203+ * @param $error Mixed With an array & two values keyed by
 204+ * 'message' and 'params', use those keys-value pairs.
 205+ * Otherwise, if its an array, just use the first value as the
 206+ * message and the remaining items as the params.
 207+ *
 208+ * @return String
 209+ */
 210+ protected function getWikiTextForError( $error ) {
 211+ if ( is_array( $error ) ) {
 212+ if ( isset( $error['message'] ) && isset( $error['params'] ) ) {
 213+ return wfMsgReal( $error['message'],
 214+ array_map( 'wfEscapeWikiText', $this->cleanParams( $error['params'] ) ),
 215+ true, false, false );
 216+ } else {
 217+ $message = array_shift($error);
 218+ return wfMsgReal( $message,
 219+ array_map( 'wfEscapeWikiText', $this->cleanParams( $error ) ),
 220+ true, false, false );
 221+ }
 222+ } else {
 223+ return wfMsgReal( $error, array(), true, false, false);
 224+ }
 225+ }
 226+
 227+ /**
 228+ * Return an array with the wikitext for each item in the array.
 229+ * @param $errors Array
 230+ * @return Array
 231+ */
 232+ function getWikiTextArray( $errors ) {
 233+ return array_map( array( $this, 'getWikiTextForError' ), $errors );
 234+ }
 235+
 236+ /**
206237 * Merge another status object into this one
207238 *
208239 * @param $other Other Status object
——@@ -223,17 +254,37 @@
224255 * @return Array
225256 */
226257 function getErrorsArray() {
 258+ return $this->getStatArray( "error" );
 259+ }
 260+
 261+ /**
 262+ * Get the list of warnings (but not errors)
 263+ *
 264+ * @return Array
 265+ */
 266+ function getWarningsArray() {
 267+ return $this->getStatArray( "warning" );
 268+ }
 269+
 270+ /**
 271+ * Returns a list of status messages of the given type
 272+ * @param $type String
 273+ *
 274+ * @return Array
 275+ */
 276+ protected function getStatArray( $type ) {
227277 $result = array();
228278 foreach ( $this->errors as $error ) {
229 - if ( $error['type'] == 'error' )
230 - if( $error['params'] )
 279+ if ( $error['type'] === $type ) {
 280+ if( $error['params'] ) {
231281 $result[] = array_merge( array( $error['message'] ), $error['params'] );
232 - else
 282+ } else {
233283 $result[] = $error['message'];
 284+ }
 285+ }
234286 }
235287 return $result;
236288 }
237 -
238289 /**
239290 * Returns true if the specified message is present as a warning or error
240291 *
Index: trunk/phase3/includes/installer/CliInstallerOutput.php
——@@ -1,79 +0,0 @@
2 -<?php
3 -
4 -/**
5 - * Output class modelled on OutputPage.
6 - *
7 - * I've opted to use a distinct class rather than derive from OutputPage here in
8 - * the interests of separation of concerns: if we used a subclass, there would be
9 - * quite a lot of things you could do in OutputPage that would break the installer,
10 - * that wouldn't be immediately obvious.
11 - */
12 -class CliInstallerOutput {
13 -
14 - function __construct( $parent ) {
15 - $this->parent = $parent;
16 - }
17 -
18 - function addHTML( $html ) {
19 - $this->contents .= $html;
20 - }
21 -
22 - function addWikiText( $text ) {
23 - $this->addHTML( $this->parent->parse( $text ) );
24 - }
25 -
26 - function addHTMLNoFlush( $html ) {
27 - $this->contents .= $html;
28 - }
29 -
30 - function addWarning( $msg ) {
31 - $this->warnings .= "<p>$msg</p>\n";
32 - }
33 -
34 - function addWarningMsg( $msg /*, ... */ ) {
35 - $params = func_get_args();
36 - array_shift( $params );
37 - $this->addWarning( wfMsg( $msg, $params ) );
38 - }
39 -
40 - function redirect( $url ) {
41 - if ( $this->headerDone ) {
42 - throw new MWException( __METHOD__ . ' called after sending headers' );
43 - }
44 - $this->redirectTarget = $url;
45 - }
46 -
47 - function output() {
48 - $this->flush();
49 - }
50 -
51 - function useShortHeader( $use = true ) {
52 - }
53 -
54 - function flush() {
55 - echo html_entity_decode( strip_tags( $this->contents ), ENT_QUOTES );
56 - flush();
57 - $this->contents = '';
58 - }
59 -
60 - function getDir() {
61 - global $wgLang;
62 - if( !is_object( $wgLang ) || !$wgLang->isRtl() )
63 - return 'ltr';
64 - else
65 - return 'rtl';
66 - }
67 -
68 - function getLanguageCode() {
69 - global $wgLang;
70 - if( !is_object( $wgLang ) )
71 - return 'en';
72 - else
73 - return $wgLang->getCode();
74 - }
75 -
76 - function outputWarnings() {
77 - $this->addHTML( $this->warnings );
78 - $this->warnings = '';
79 - }
80 -}
Index: trunk/phase3/includes/installer/Installer.php
——@@ -233,21 +233,9 @@
234234 foreach ( $this->defaultVarNames as $var ) {
235235 $this->settings[$var] = $GLOBALS[$var];
236236 }
237 -
238 - $this->parserTitle = Title::newFromText( 'Installer' );
239 - $this->parserOptions = new ParserOptions;
240 - $this->parserOptions->setEditSection( false );
241 - }
242 -
243 - /*
244 - * Set up our database objects. They need to inject some of their
245 - * own configuration into our global context. Usually this'll just be
246 - * things like the default $wgDBname.
247 - */
248 - function setupDatabaseObjects() {
249237 foreach ( $this->dbTypes as $type ) {
250238 $installer = $this->getDBInstaller( $type );
251 - if ( !$installer->isCompiled() ) {
 239+ if ( !$installer ) {
252240 continue;
253241 }
254242 $defaults = $installer->getGlobalDefaults();
——@@ -259,6 +247,10 @@
260248 }
261249 }
262250 }
 251+
 252+ $this->parserTitle = Title::newFromText( 'Installer' );
 253+ $this->parserOptions = new ParserOptions;
 254+ $this->parserOptions->setEditSection( false );
263255 }
264256
265257 /**
——@@ -286,9 +278,15 @@
287279 if ( !$type ) {
288280 $type = $this->getVar( 'wgDBtype' );
289281 }
 282+ $type = strtolower($type);
 283+
290284 if ( !isset( $this->dbInstallers[$type] ) ) {
291285 $class = ucfirst( $type ). 'Installer';
292 - $this->dbInstallers[$type] = new $class( $this );
 286+ if ($class::isCompiled()) {
 287+ $this->dbInstallers[$type] = new $class( $this );
 288+ } else {
 289+ $this->dbInstallers[$type] = false;
 290+ }
293291 }
294292 return $this->dbInstallers[$type];
295293 }
——@@ -410,7 +408,7 @@
411409 foreach ( $this->dbTypes as $name ) {
412410 $db = $this->getDBInstaller( $name );
413411 $readableName = wfMsg( 'config-type-' . $name );
414 - if ( $db->isCompiled() ) {
 412+ if ( $db ) {
415413 $compiledDBs[] = $name;
416414 $goodNames[] = $readableName;
417415 }
——@@ -901,8 +899,13 @@
902900 }
903901
904902 public function installDatabase() {
905 - $installer = $this->getDBInstaller( $this->getVar( 'wgDBtype' ) );
906 - $status = $installer->setupDatabase();
 903+ $type = $this->getVar( 'wgDBtype' );
 904+ $installer = $this->getDBInstaller( $type );
 905+ if(!$installer) {
 906+ $status = Status::newFatal( "config-no-db", $type );
 907+ } else {
 908+ $status = $installer->setupDatabase();
 909+ }
907910 return $status;
908911 }
909912
——@@ -1046,4 +1049,18 @@
10471050 $GLOBALS['wgShowSQLErrors'] = true;
10481051 $GLOBALS['wgShowDBErrorBacktrace'] = true;
10491052 }
 1053+
 1054+ /**
 1055+ * Add an installation step following the given step.
 1056+ * @param $findStep String the step to find. Use NULL to put the step at the beginning.
 1057+ * @param $callback array
 1058+ */
 1059+ function addInstallStepFollowing( $findStep, $callback ) {
 1060+ $where = 0;
 1061+ if( $findStep !== null ) $where = array_search( $findStep, $this->installSteps );
 1062+
 1063+ array_splice( $this->installSteps, $where, 0, $callback );
 1064+ }
 1065+
 1066+
10501067 }
Index: trunk/phase3/includes/installer/SqliteInstaller.php
——@@ -10,8 +10,8 @@
1111 return 'sqlite';
1212 }
1313
14 - function isCompiled() {
15 - return $this->checkExtension( 'pdo_sqlite' );
 14+ static function isCompiled() {
 15+ return self::checkExtension( 'pdo_sqlite' );
1616 }
1717
1818 function getGlobalDefaults() {
Index: trunk/phase3/includes/installer/MysqlInstaller.php
——@@ -39,19 +39,24 @@
4040 return;
4141 }
4242
 43+ if ( $this->parent->getVar( 'wgDBtype' ) !== $this->getName() ) {
 44+ return;
 45+ }
 46+
4347 # Add our user callback to installSteps, right before the tables are created.
44 - $where_tables = array_search( "tables", $this->parent->installSteps );
 48+
 49+ debug_print_backtrace();
4550 $callback = array(
4651 array(
4752 'name' => 'user',
4853 'callback' => array( &$this, 'setupUser' ),
4954 )
5055 );
51 - array_splice( $this->parent->installSteps, $where_tables, 0, $callback );
 56+ $this->parent->addInstallStepFollowing( "tables", $callback );
5257 }
53 -
54 - function isCompiled() {
55 - return $this->checkExtension( 'mysql' );
 58+
 59+ static function isCompiled() {
 60+ return self::checkExtension( 'mysql' );
5661 }
5762
5863 function getGlobalDefaults() {
Index: trunk/phase3/includes/installer/OracleInstaller.php
——@@ -19,8 +19,8 @@
2020 return 'oracle';
2121 }
2222
23 - function isCompiled() {
24 - return $this->checkExtension( 'oci8' );
 23+ static function isCompiled() {
 24+ return self::checkExtension( 'oci8' );
2525 }
2626
2727 function getConnectForm() {
Index: trunk/phase3/includes/installer/PostgresInstaller.php
——@@ -25,8 +25,8 @@
2626 return 'postgres';
2727 }
2828
29 - function isCompiled() {
30 - return $this->checkExtension( 'pgsql' );
 29+ static function isCompiled() {
 30+ return self::checkExtension( 'pgsql' );
3131 }
3232
3333 function getConnectForm() {
Index: trunk/phase3/includes/installer/CliInstaller.php
——@@ -68,17 +68,24 @@
6969 * Main entry point.
7070 */
7171 function execute( ) {
72 - foreach( $this->getInstallSteps() as $step ) {
73 - $this->showMessage("Installing $step... ");
 72+ foreach( $this->getInstallSteps() as $stepObj ) {
 73+ $step = is_array( $stepObj ) ? $stepObj['name'] : $stepObj;
 74+ $this->showMessage( wfMsg( "config-install-$step") .
 75+ wfMsg( 'ellipsis' ) . wfMsg( 'word-separator' ) );
7476 $func = 'install' . ucfirst( $step );
7577 $status = $this->{$func}();
 78+ $warnings = $status->getWarningsArray();
7679 if ( !$status->isOk() ) {
7780 $this->showStatusMessage( $status );
 81+ echo "\n";
7882 exit;
79 - } elseif ( !$status->isGood() ) {
80 - $this->showStatusMessage( $status );
 83+ } elseif ( count($warnings) !== 0 ) {
 84+ foreach ( $status->getWikiTextArray( $warnings ) as $w ) {
 85+ $this->showMessage( $w . wfMsg( 'ellipsis') .
 86+ wfMsg( 'word-separator' ) );
 87+ }
8188 }
82 - $this->showMessage("done\n");
 89+ $this->showMessage( wfMsg( 'config-install-step-done' ) ."\n");
8390 }
8491 }
8592
Index: trunk/phase3/includes/installer/InstallerDBType.php
——@@ -24,7 +24,7 @@
2525 /**
2626 * @return true if the client library is compiled in
2727 */
28 - abstract function isCompiled();
 28+ abstract static function isCompiled();
2929
3030 /**
3131 * Get an array of MW configuration globals that will be configured by this class.
——@@ -126,7 +126,7 @@
127127 * Convenience function
128128 * Check if a named extension is present
129129 */
130 - function checkExtension( $name ) {
 130+ static function checkExtension( $name ) {
131131 wfSuppressWarnings();
132132 $compiled = wfDl( $name );
133133 wfRestoreWarnings();

Follow-up revisions

RevisionCommit summaryAuthorDate
r69136new-installer: Remove debug_print_backtrace() call added in r69128avar10:42, 7 July 2010
r69137new-installer: Remove duplicate code added in r69128avar10:42, 7 July 2010
r69141* use call_user_func instead of $class::isCompiled() for php 5.2 (re r69128)...mah13:39, 7 July 2010
r69142Partial revert r69128: go back to making isCompiled() an instance method rath...demon13:52, 7 July 2010
r69147new-installer: Revert DB driver check breakage in r69128...avar15:05, 7 July 2010
r69228Cleanup r69128, getStatArray() -> getStatusArray(). It's two characters, c'mondemon09:26, 10 July 2010

Comments

#Comment by Ævar Arnfjörð Bjarmason (talk | contribs)   11:01, 7 July 2010

This broke the non-MySQL installer again. The whole point of the changes in r69108 is to move the database object construction out of __construct and do it after we've read the session data. Otherwise the if ( $this->parent->getVar( 'wgDBtype' ) !== $this->getName() ) check in MysqlInstaller will always be true (the commit message for r69108 explains all this, b.t.w.).

With this change the SQLite installer is broken again: 

[07-Jul-2010 10:46:30] PHP Fatal error:  Call to undefined method SqliteInstaller::setupUser() in /home/avar/g/phase3/includes/installer/Installer.php on line 914
#Comment by 😂 (talk | contribs)   13:13, 7 July 2010 
+	abstract static function isCompiled();

No such thing as an abstract static function.

#Comment by 😂 (talk | contribs)   13:58, 7 July 2010

r69142 should resolve most of this?

#Comment by Ævar Arnfjörð Bjarmason (talk | contribs)   15:10, 7 July 2010

Yes, everything's OK now. Marking this as resolved.

#Comment by Bryan (talk | contribs)   18:49, 7 July 2010

Do you really need to abbreviate Status into Stat?

#Comment by 😂 (talk | contribs)   09:27, 10 July 2010

Fixed in r69228.

Status & tagging log