r88936 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r88935‎ | r88936 | r88937 >
Date:21:49, 26 May 2011
Author:mah
Status:reverted (Comments)
Tags:
Comment:
This needs to be forward-ported to trunk.

Fix for Bug #28172 - wfGetDB called when it shouldn't be

Avoid an ominous error (“Mediawiki tried to access the database via
wfGetDB(). This is not allowed.”) by passing db handles to user
methods that would otherwise have to use wfGetDB().
Modified paths:
  • /branches/REL1_17/phase3/includes/User.php (modified) (history)
  • /branches/REL1_17/phase3/includes/installer/Installer.php (modified) (history)

Diff [purge]

Index: branches/REL1_17/phase3/includes/User.php
——@@ -2161,8 +2161,8 @@
21622162 * This takes immediate effect.
21632163 * @param $group \string Name of the group to add
21642164 */
2165 - function addGroup( $group ) {
2166 - $dbw = wfGetDB( DB_MASTER );
 2165+ function addGroup( $group, $dbw = null ) {
 2166+ if( $dbw === null ) $dbw = wfGetDB( DB_MASTER );
21672167 if( $this->getId() ) {
21682168 $dbw->insert( 'user_groups',
21692169 array(
——@@ -2563,7 +2563,7 @@
25642564 ), __METHOD__
25652565 );
25662566
2567 - $this->saveOptions();
 2567+ $this->saveOptions( $dbw );
25682568
25692569 wfRunHooks( 'UserSaveSettings', array( $this ) );
25702570 $this->clearSharedCache();
——@@ -2640,9 +2640,9 @@
26412641 /**
26422642 * Add this existing user object to the database
26432643 */
2644 - function addToDatabase() {
 2644+ function addToDatabase( $dbw = null ) {
26452645 $this->load();
2646 - $dbw = wfGetDB( DB_MASTER );
 2646+ if( $dbw === null ) $dbw = wfGetDB( DB_MASTER );
26472647 $seqVal = $dbw->nextSequenceValue( 'user_user_id_seq' );
26482648 $dbw->insert( 'user',
26492649 array(
——@@ -2665,7 +2665,7 @@
26662666 // Clear instance cache other than user table data, which is already accurate
26672667 $this->clearInstanceCache();
26682668
2669 - $this->saveOptions();
 2669+ $this->saveOptions( $dbw );
26702670 }
26712671
26722672 /**
——@@ -3683,13 +3683,13 @@
36843684 wfRunHooks( 'UserLoadOptions', array( $this, &$this->mOptions ) );
36853685 }
36863686
3687 - protected function saveOptions() {
 3687+ protected function saveOptions( $dbw = null ) {
36883688 global $wgAllowPrefChange;
36893689
36903690 $extuser = ExternalUser::newFromUser( $this );
36913691
36923692 $this->loadOptions();
3693 - $dbw = wfGetDB( DB_MASTER );
 3693+ if( $dbw === null ) $dbw = wfGetDB( DB_MASTER );
36943694
36953695 $insert_rows = array();
36963696
Index: branches/REL1_17/phase3/includes/installer/Installer.php
——@@ -1370,6 +1370,12 @@
13711371 protected function createSysop() {
13721372 $name = $this->getVar( '_AdminName' );
13731373 $user = User::newFromName( $name );
 1374+ $status = $this->getDBInstaller()->getConnection();
 1375+ if( $status->isOK() ) {
 1376+ $db = $status->value;
 1377+ } else {
 1378+ return Status::newFatal( 'config-admin-error-user', $name );
 1379+ }
13741380
13751381 if ( !$user ) {
13761382 // We should've validated this earlier anyway!
——@@ -1377,7 +1383,7 @@
13781384 }
13791385
13801386 if ( $user->idForName() == 0 ) {
1381 - $user->addToDatabase();
 1387+ $user->addToDatabase( $db );
13821388
13831389 try {
13841390 $user->setPassword( $this->getVar( '_AdminPassword' ) );
——@@ -1385,12 +1391,12 @@
13861392 return Status::newFatal( 'config-admin-error-password', $name, $pwe->getMessage() );
13871393 }
13881394
1389 - $user->addGroup( 'sysop' );
1390 - $user->addGroup( 'bureaucrat' );
 1395+ $user->addGroup( 'sysop', $db );
 1396+ $user->addGroup( 'bureaucrat', $db );
13911397 if( $this->getVar( '_AdminEmail' ) ) {
13921398 $user->setEmail( $this->getVar( '_AdminEmail' ) );
13931399 }
1394 - $user->saveSettings();
 1400+ $user->saveSettings( $db );
13951401
13961402 // Update user count
13971403 $ssUpdate = new SiteStatsUpdate( 0, 0, 0, 0, 1 );

Follow-up revisions

RevisionCommit summaryAuthorDate
r89374Finish fix for bug #28172 (“wfGetDB called when it shouldn't be”)....mah00:53, 3 June 2011
r89376Fix for bug #28172 (“wfGetDB called when it shouldn't be”)....mah01:06, 3 June 2011
r89738Revert r89374, r88936 backports.mah20:04, 8 June 2011
r89739Revert r89374, r88936mah20:13, 8 June 2011
r101880Bug 28172 - wfGetDB called when it shouldn't beoverlordq21:00, 3 November 2011

Comments

#Comment by Tim Starling (talk | contribs)   04:34, 8 June 2011

I think this should be reverted. Regular operations on the new database should be done after DatabaseInstaller::enableLB() is called. It certainly shouldn't be necessary to add database connection parameters to every function.

Maybe the bug that you are trying to fix here is due to PostgresInstaller::createTables() not calling enableLB() when it is done. If that's the case, then it should have other symptoms apart from failure of createSysop().

#Comment by MarkAHershberger (talk | contribs)   04:41, 8 June 2011

Thanks for looking at this. I wasn't sure about the change, but it was the fix that someone more experienced than me suggested. I'll try enableLB() tomorrow.

Status & tagging log