r90137 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r90136‎ | r90137 | r90138 >
Date:19:05, 15 June 2011
Author:brion
Status:ok (Comments)
Tags:
Comment:
Fix for partial regression in r84534: Block::newFromId was no longer handling the case where there was no matching row correctly. Now returns null.

This bug was showing up in test results like this:

BlockTest::testInitializerFunctionsReturnCorrectBlock
Trying to get property of non-object

/home/ci/cruisecontrol-bin-2.8.3/projects/mw/source/includes/Block.php:340
/home/ci/cruisecontrol-bin-2.8.3/projects/mw/source/includes/Block.php:365
/home/ci/cruisecontrol-bin-2.8.3/projects/mw/source/includes/Block.php:118
/home/ci/cruisecontrol-bin-2.8.3/projects/mw/source/tests/phpunit/includes/BlockTest.php:60
/home/ci/cruisecontrol-bin-2.8.3/projects/mw/source/tests/phpunit/MediaWikiTestCase.php:60
/home/ci/cruisecontrol-bin-2.8.3/projects/mw/source/tests/phpunit/MediaWikiPHPUnitCommand.php:20
/home/ci/cruisecontrol-bin-2.8.3/projects/mw/source/tests/phpunit/phpunit.php:60

However, that only triggers because the actual test is failing -- it's expecting to get a return back. This can only be reproduce when using the suite.xml configuration file, and not when running the BlockTest standalone.
Modified paths:
  • /trunk/phase3/includes/Block.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/Block.php
——@@ -105,7 +105,7 @@
106106 * Load a blocked user from their block id.
107107 *
108108 * @param $id Integer: Block id to search for
109 - * @return Block object
 109+ * @return Block object or null
110110 */
111111 public static function newFromID( $id ) {
112112 $dbr = wfGetDB( DB_SLAVE );
——@@ -115,7 +115,11 @@
116116 array( 'ipb_id' => $id ),
117117 __METHOD__
118118 );
119 - return Block::newFromRow( $res );
 119+ if ( $res ) {
 120+ return Block::newFromRow( $res );
 121+ } else {
 122+ return null;
 123+ }
120124 }
121125
122126 /**

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r84534(hopefully) last bit of heavy lifting in Block.php: now that we've internalis...happy-melon17:18, 22 March 2011

Comments

#Comment by 😂 (talk | contribs)   19:38, 15 June 2011

The failures in BlockTest are because the class gets instantiated multiple times (and thus the $this->block->insert() fails and the $this->block doesn't remain valid)

#Comment by Brion VIBBER (talk | contribs)   20:23, 15 June 2011

The block->insert() seems to work fine -- it just returns 'null' as the block id. Depending on what else has happened on the DB connection, the lookup for the null id either returns the last inserted row or nothing.

#Comment by 😂 (talk | contribs)   20:26, 15 June 2011

That's because Block::insert() is not correct here anyway (it doesn't set $this->mId for one, and nextSequenceValue() fails for Mysql, as we've discussed before).

You can prove the multiple instantiations--and thus multiple setUp()s--by removing the IGNORE from Block::insert() and run the test with --filter. It'll explode due to the duplicate block insertions.

#Comment by Brion VIBBER (talk | contribs)   20:38, 15 June 2011

Yeah I see it's doing both of those things now. I think I've got it almost resolved by trying to delete the old block first so we're always testing a fresh one.

Status & tagging log