From 536fad8f908e1f2f20b6454985e905e1e5fdde07 Mon Sep 17 00:00:00 2001
From: Brion Vibber <brion@users.mediawiki.org>
Date: Tue, 24 May 2011 21:04:50 +0000
Subject: [PATCH] * (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.
---
 includes/Block.php                   | 10 +++++----
 tests/phpunit/includes/BlockTest.php | 33 ++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/includes/Block.php b/includes/Block.php
index d1e498a3dc..d336dcc859 100644
--- 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 278dd92767..908a0a8892 100644
--- 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 )
+		);
+	}
 }
 
-- 
2.20.1