r80892 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r80891‎ | r80892 | r80893 >
Date:18:36, 24 January 2011
Author:demon
Status:ok (Comments)
Tags:
Comment:
* Cleanup massive duplication across Database constructors. Default implementation fairly sane. Now they all share the same if( $server ) logic to allow constructing the class without forcing open a connection (MySQL has done this since at least r15094)
* Get rid of intermediate installTables() callback
* Actually cache the result of DbInstaller::getConnection() like the documentation says
Modified paths:
  • /trunk/phase3/includes/db/DatabaseIbm_db2.php (modified) (history)
  • /trunk/phase3/includes/db/DatabaseMssql.php (modified) (history)
  • /trunk/phase3/includes/db/DatabaseOracle.php (modified) (history)
  • /trunk/phase3/includes/db/DatabasePostgres.php (modified) (history)
  • /trunk/phase3/includes/db/DatabaseSqlite.php (modified) (history)
  • /trunk/phase3/includes/installer/DatabaseInstaller.php (modified) (history)
  • /trunk/phase3/includes/installer/Installer.php (modified) (history)
  • /trunk/phase3/includes/installer/MysqlInstaller.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/db/DatabaseOracle.php
——@@ -182,7 +182,7 @@
183183 {
184184 $tablePrefix = $tablePrefix == 'get from global' ? $tablePrefix : strtoupper( $tablePrefix );
185185 parent::__construct( $server, $user, $password, $dbName, $flags, $tablePrefix );
186 - wfRunHooks( 'DatabaseOraclePostInit', array( &$this ) );
 186+ wfRunHooks( 'DatabaseOraclePostInit', array( $this ) );
187187 }
188188
189189 function getType() {
Index: trunk/phase3/includes/db/DatabasePostgres.php
——@@ -100,13 +100,6 @@
101101 var $numeric_version = null;
102102 var $mAffectedRows = null;
103103
104 - function __construct( $server = false, $user = false, $password = false, $dbName = false,
105 - $flags = 0 )
106 - {
107 - $this->mFlags = $flags;
108 - $this->open( $server, $user, $password, $dbName );
109 - }
110 -
111104 function getType() {
112105 return 'postgres';
113106 }
Index: trunk/phase3/includes/db/DatabaseIbm_db2.php
——@@ -264,14 +264,6 @@
265265 $dbName = false, $flags = 0,
266266 $schema = self::USE_GLOBAL )
267267 {
268 -
269 - global $wgOut, $wgDBmwschema;
270 - # Can't get a reference if it hasn't been set yet
271 - if ( !isset( $wgOut ) ) {
272 - $wgOut = null;
273 - }
274 - $this->mFlags = DBO_TRX | $flags;
275 -
276268 if ( $schema == self::USE_GLOBAL ) {
277269 $this->mSchema = $wgDBmwschema;
278270 } else {
——@@ -286,7 +278,7 @@
287279 $this->setDB2Option( 'rowcount', 'DB2_ROWCOUNT_PREFETCH_ON',
288280 self::STMT_OPTION );
289281
290 - $this->open( $server, $user, $password, $dbName );
 282+ parent::__construct( $server, $user, $password, $dbName, $flags );
291283 }
292284
293285 /**
Index: trunk/phase3/includes/db/DatabaseMssql.php
——@@ -17,13 +17,6 @@
1818 var $mLastResult = NULL;
1919 var $mAffectedRows = NULL;
2020
21 - function __construct( $server = false, $user = false, $password = false, $dbName = false,
22 - $flags = 0 )
23 - {
24 - $this->mFlags = $flags;
25 - $this->open( $server, $user, $password, $dbName );
26 - }
27 -
2821 function cascadingDeletes() {
2922 return true;
3023 }
Index: trunk/phase3/includes/db/DatabaseSqlite.php
——@@ -24,12 +24,12 @@
2525 * Parameters $server, $user and $password are not used.
2626 */
2727 function __construct( $server = false, $user = false, $password = false, $dbName = false, $flags = 0 ) {
28 - global $wgSharedDB;
29 - $this->mFlags = $flags;
3028 $this->mName = $dbName;
31 -
32 - if( $server ) {
33 - if ( $this->open( $server, $user, $password, $dbName ) && $wgSharedDB ) {
 29+ parent::__construct( $server, $user, $password, $dbName, $flags );
 30+ // parent doesn't open when $server is false, but we can work with $dbName
 31+ if( !$server && $dbName ) {
 32+ global $wgSharedDB;
 33+ if( $this->open( $server, $user, $password, $dbName ) && $wgSharedDB ) {
3434 $this->attachDatabase( $wgSharedDB );
3535 }
3636 }
Index: trunk/phase3/includes/installer/Installer.php
——@@ -556,23 +556,6 @@
557557 }
558558
559559 /**
560 - * TODO: document
561 - *
562 - * @param $installer DatabaseInstaller
563 - *
564 - * @return Status
565 - */
566 - public function installTables( DatabaseInstaller &$installer ) {
567 - $status = $installer->createTables();
568 -
569 - if( $status->isOK() ) {
570 - LBFactory::enableBackend();
571 - }
572 -
573 - return $status;
574 - }
575 -
576 - /**
577560 * Exports all wg* variables stored by the installer into global scope.
578561 */
579562 public function exportVars() {
——@@ -1222,7 +1205,7 @@
12231206 protected function getInstallSteps( DatabaseInstaller &$installer ) {
12241207 $coreInstallSteps = array(
12251208 array( 'name' => 'database', 'callback' => array( $installer, 'setupDatabase' ) ),
1226 - array( 'name' => 'tables', 'callback' => array( $this, 'installTables' ) ),
 1209+ array( 'name' => 'tables', 'callback' => array( $installer, 'createTables' ) ),
12271210 array( 'name' => 'interwiki', 'callback' => array( $installer, 'populateInterwikiTable' ) ),
12281211 array( 'name' => 'secretkey', 'callback' => array( $this, 'generateSecretKey' ) ),
12291212 array( 'name' => 'upgradekey', 'callback' => array( $this, 'generateUpgradeKey' ) ),
Index: trunk/phase3/includes/installer/SqliteInstaller.php
——@@ -93,17 +93,21 @@
9494 global $wgSQLiteDataDir;
9595
9696 $status = Status::newGood();
97 - $dir = $this->getVar( 'wgSQLiteDataDir' );
98 - $dbName = $this->getVar( 'wgDBname' );
 97+ if( is_null( $this->db ) ) {
 98+ $dir = $this->getVar( 'wgSQLiteDataDir' );
 99+ $dbName = $this->getVar( 'wgDBname' );
99100
100 - try {
101 - # FIXME: need more sensible constructor parameters, e.g. single associative array
102 - # Setting globals kind of sucks
103 - $wgSQLiteDataDir = $dir;
104 - $this->db = new DatabaseSqlite( false, false, false, $dbName );
 101+ try {
 102+ # FIXME: need more sensible constructor parameters, e.g. single associative array
 103+ # Setting globals kind of sucks
 104+ $wgSQLiteDataDir = $dir;
 105+ $this->db = new DatabaseSqlite( false, false, false, $dbName );
 106+ $status->value = $this->db;
 107+ } catch ( DBConnectionError $e ) {
 108+ $status->fatal( 'config-sqlite-connection-error', $e->getMessage() );
 109+ }
 110+ } else {
105111 $status->value = $this->db;
106 - } catch ( DBConnectionError $e ) {
107 - $status->fatal( 'config-sqlite-connection-error', $e->getMessage() );
108112 }
109113 return $status;
110114 }
Index: trunk/phase3/includes/installer/DatabaseInstaller.php
——@@ -28,7 +28,7 @@
2929 *
3030 * @var DatabaseBase
3131 */
32 - public $db;
 32+ public $db = null;
3333
3434 /**
3535 * Internal variables for installation.
——@@ -140,6 +140,10 @@
141141 } else {
142142 $this->db->commit( __METHOD__ );
143143 }
 144+ // Resume normal operations
 145+ if( $status->isOk() ) {
 146+ LBFactory::enableBackend();
 147+ }
144148 return $status;
145149 }
146150
Index: trunk/phase3/includes/installer/MysqlInstaller.php
——@@ -113,19 +113,23 @@
114114
115115 public function getConnection() {
116116 $status = Status::newGood();
117 - try {
118 - $this->db = new DatabaseMysql(
119 - $this->getVar( 'wgDBserver' ),
120 - $this->getVar( '_InstallUser' ),
121 - $this->getVar( '_InstallPassword' ),
122 - false,
123 - false,
124 - 0,
125 - $this->getVar( 'wgDBprefix' )
126 - );
 117+ if( is_null( $this->db ) ) {
 118+ try {
 119+ $this->db = new DatabaseMysql(
 120+ $this->getVar( 'wgDBserver' ),
 121+ $this->getVar( '_InstallUser' ),
 122+ $this->getVar( '_InstallPassword' ),
 123+ false,
 124+ false,
 125+ 0,
 126+ $this->getVar( 'wgDBprefix' )
 127+ );
 128+ $status->value = $this->db;
 129+ } catch ( DBConnectionError $e ) {
 130+ $status->fatal( 'config-connection-error', $e->getMessage() );
 131+ }
 132+ } else {
127133 $status->value = $this->db;
128 - } catch ( DBConnectionError $e ) {
129 - $status->fatal( 'config-connection-error', $e->getMessage() );
130134 }
131135 return $status;
132136 }
Index: trunk/phase3/includes/installer/PostgresInstaller.php
——@@ -100,16 +100,19 @@
101101
102102 function getConnection($database = 'template1') {
103103 $status = Status::newGood();
104 -
105 - try {
106 - $this->db = new DatabasePostgres(
107 - $this->getVar( 'wgDBserver' ),
108 - $this->getVar( '_InstallUser' ),
109 - $this->getVar( '_InstallPassword' ),
110 - $database );
 104+ if( is_null( $this->db ) ) {
 105+ try {
 106+ $this->db = new DatabasePostgres(
 107+ $this->getVar( 'wgDBserver' ),
 108+ $this->getVar( '_InstallUser' ),
 109+ $this->getVar( '_InstallPassword' ),
 110+ $database );
 111+ $status->value = $this->db;
 112+ } catch ( DBConnectionError $e ) {
 113+ $status->fatal( 'config-connection-error', $e->getMessage() );
 114+ }
 115+ } else {
111116 $status->value = $this->db;
112 - } catch ( DBConnectionError $e ) {
113 - $status->fatal( 'config-connection-error', $e->getMessage() );
114117 }
115118 return $status;
116119 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r80957* Fixed a bug causing the installer to ignore the "engine" and "charset" sett...tstarling07:37, 25 January 2011
r81143Restore global wrongly removed in r80892platonides18:10, 28 January 2011
r84532Cleanup r80892, r84485: Check user also in sqlite for consistency.demon15:59, 22 March 2011
r84548MFT r80892, r81143, partial r81896, r84532demon20:10, 22 March 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r15094Revert to r15092; massive breakage, unable to connect to MySQL at allbrion16:11, 27 June 2006

Comments

#Comment by Platonides (talk | contribs)   18:11, 28 January 2011

You removed the forcing of DBO_TRX flag in DatabaseIbm_db2. Had been there since the beginning (r45755). I don't know if it was really needed or not.

#Comment by 😂 (talk | contribs)   19:03, 28 January 2011

lol db2.

Status & tagging log