* (bug 29116) Fix regression breaking CheckUser extension

Fixes regression from r84475 and friends which made Block->load() and its new front-end Block::newFromTarget() fail when an empty string was passed in as the IP / $vagueTarget parameter to indicate skipping IP-based lookups.
Added phpunit test cases to confirm that both Block->load() and Block::newFromTarget() work when given null (already ok), '' (as done from CheckUser), or false (not seen, but perfectly legit sounding).
Adjusted comparisons to work as expected.

diff --git a/includes/Block.php b/includes/Block.php
index d1e498a..d336dcc 100644 (file)
--- a/includes/Block.php
+++ b/includes/Block.php
@@ -184,7 +184,7 @@ class Block {
         *     2) A rangeblock encompasing the given IP (smallest first)
         *     3) An autoblock on the given IP
         * @param $vagueTarget User|String also search for blocks affecting this target.  Doesn't
-        *     make any sense to use TYPE_AUTO / TYPE_ID here
+        *     make any sense to use TYPE_AUTO / TYPE_ID here. Leave blank to skip IP lookups.
         * @return Bool whether a relevant block was found
         */
        protected function newLoad( $vagueTarget = null ) {
@@ -198,7 +198,8 @@ class Block {
                        $conds = array( 'ipb_address' => array() );
                }
 
-               if( $vagueTarget !== null ){
+               # Be aware that the != '' check is explicit, since empty values will be passed by some callers.
+               if( $vagueTarget != ''){
                        list( $target, $type ) = self::parseTarget( $vagueTarget );
                        switch( $type ) {
                                case self::TYPE_USER:
@@ -962,7 +963,7 @@ class Block {
         *     1.2.3.4 will not select 1.2.0.0/16 or even 1.2.3.4/32)
         * @param $vagueTarget String|User|Int as above, but we will search for *any* block which
         *     affects that target (so for an IP address, get ranges containing that IP; and also
-        *     get any relevant autoblocks)
+        *     get any relevant autoblocks). Leave empty or blank to skip IP-based lookups.
         * @param $fromMaster Bool whether to use the DB_MASTER database
         * @return Block|null (null if no relevant block could be found).  The target and type
         *     of the returned Block will refer to the actual block which was found, which might
@@ -979,8 +980,9 @@ class Block {
                if( $type == Block::TYPE_ID || $type == Block::TYPE_AUTO ){
                        return Block::newFromID( $target );
 
-               } elseif( $target === null && $vagueTarget === null ){
+               } elseif( $target === null && $vagueTarget == '' ){
                        # We're not going to find anything useful here
+                       # Be aware that the == '' check is explicit, since empty values will be passed by some callers.
                        return null;
 
                } elseif( in_array( $type, array( Block::TYPE_USER, Block::TYPE_IP, Block::TYPE_RANGE, null ) ) ) {

diff --git a/tests/phpunit/includes/BlockTest.php b/tests/phpunit/includes/BlockTest.php
index 278dd92..908a0a8 100644 (file)
--- a/tests/phpunit/includes/BlockTest.php
+++ b/tests/phpunit/includes/BlockTest.php
@@ -48,5 +48,38 @@ class BlockTest extends MediaWikiLangTestCase {
                
        }
 
+       /**
+        * This is the method previously used to load block info in CheckUser etc
+        * passing an empty value (empty string, null, etc) as the ip parameter bypasses IP lookup checks.
+        *
+        * This stopped working with r84475 and friends: regression being fixed for bug 29116.
+        *
+        * @dataProvider dataBug29116
+        */
+       function testBug29116LoadWithEmptyIp( $vagueTarget ) {
+               $block = new Block();
+               $block->load( $vagueTarget, 'UTBlockee' );
+               $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 ) );
+       }
+
+       /**
+        * CheckUser since being changed to use Block::newFromTarget started failing
+        * because the new function didn't accept empty strings like Block::load()
+        * had. Regression bug 29116.
+        *
+        * @dataProvider dataBug29116
+        */
+       function testBug29116NewFromTargetWithEmptyIp( $vagueTarget ) {
+               $block = Block::newFromTarget('UTBlockee', $vagueTarget);
+               $this->assertTrue( $this->block->equals( $block ), "newFromTarget() returns the same block as the one that was made when given empty vagueTarget param " . var_export( $vagueTarget, true ) );
+       }
+
+       function dataBug29116() {
+               return array(
+                       array( null ),
+                       array( '' ),
+                       array( false )
+               );
+       }
 }