r90219 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r90218‎ | r90219 | r90220 >
Date:17:58, 16 June 2011
Author:brion
Status:ok (Comments)
Tags:
Comment:
* (bug 29426) Fix wrong use of Block::load's second parameter in BlockTest

It was accidentally passing a username where it should have passed a user ID, causing PostgreSQL's stricter comparisons to fail, while MySQL's allowed it to run without complaint but returned bad results.
Of course that bug got hidden by the test.... testing the wrong thing... :)

Now correctly loads using the user id instead of name, checks the proper return values, and actually compares the right object.
Modified paths:
  • /trunk/phase3/tests/phpunit/includes/BlockTest.php (modified) (history)

Diff [purge]

Index: trunk/phase3/tests/phpunit/includes/BlockTest.php
——@@ -91,9 +91,14 @@
9292 * @dataProvider dataBug29116
9393 */
9494 function testBug29116LoadWithEmptyIp( $vagueTarget ) {
 95+ $uid = User::idFromName( 'UTBlockee' );
 96+ $this->assertTrue( ($uid > 0), 'Must be able to look up the target user during tests' );
 97+
9598 $block = new Block();
96 - $block->load( $vagueTarget, 'UTBlockee' );
97 - $this->assertTrue( $this->block->equals( Block::newFromTarget('UTBlockee', $vagueTarget) ), "Block->load() returns the same block as the one that was made when given empty ip param " . var_export( $vagueTarget, true ) );
 99+ $ok = $block->load( $vagueTarget, $uid );
 100+ $this->assertTrue( $ok, "Block->load() with empty IP and user ID '$uid' should return a block" );
 101+
 102+ $this->assertTrue( $this->block->equals( $block ), "Block->load() returns the same block as the one that was made when given empty ip param " . var_export( $vagueTarget, true ) );
98103 }
99104
100105 /**

Sign-offs

UserFlagDate
Hasharinspected18:15, 16 June 2011

Comments

#Comment by Hashar (talk | contribs)   18:14, 16 June 2011

Maybe block::load() should throw an exception if the second parameter ($user) is not an integer?

I don't think PHP allow type hinting in functions or we could have wrotten something like: 

 function /*block -> */ load( string $address = '', int $user = 0 ) { .. }
                              ^^^^^^                ^^^

Status & tagging log