r94350 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r94349‎ | r94350 | r94351 >
Date:14:32, 12 August 2011
Author:robin
Status:ok (Comments)
Tags:needs-php-test 
Comment:
Fix r91886 thanks to johnduhart: check if it is an IP *before* stripping subpages, otherwise IP range blocking does not work
Modified paths:
  • /trunk/phase3/includes/Block.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/Block.php
——@@ -1057,6 +1057,19 @@
10581058 return array( null, null );
10591059 }
10601060
 1061+ if ( IP::isValid( $target ) ) {
 1062+ # We can still create a User if it's an IP address, but we need to turn
 1063+ # off validation checking (which would exclude IP addresses)
 1064+ return array(
 1065+ User::newFromName( IP::sanitizeIP( $target ), false ),
 1066+ Block::TYPE_IP
 1067+ );
 1068+
 1069+ } elseif ( IP::isValidBlock( $target ) ) {
 1070+ # Can't create a User from an IP range
 1071+ return array( IP::sanitizeRange( $target ), Block::TYPE_RANGE );
 1072+ }
 1073+
10611074 # Consider the possibility that this is not a username at all
10621075 # but actually an old subpage (bug #29797)
10631076 if( strpos( $target, '/' ) !== false ){
——@@ -1072,18 +1085,6 @@
10731086 # since hash characters are not valid in usernames or titles generally.
10741087 return array( $userObj, Block::TYPE_USER );
10751088
1076 - } elseif ( IP::isValid( $target ) ) {
1077 - # We can still create a User if it's an IP address, but we need to turn
1078 - # off validation checking (which would exclude IP addresses)
1079 - return array(
1080 - User::newFromName( IP::sanitizeIP( $target ), false ),
1081 - Block::TYPE_IP
1082 - );
1083 -
1084 - } elseif ( IP::isValidBlock( $target ) ) {
1085 - # Can't create a User from an IP range
1086 - return array( IP::sanitizeRange( $target ), Block::TYPE_RANGE );
1087 -
10881089 } elseif ( preg_match( '/^#\d+$/', $target ) ) {
10891090 # Autoblock reference in the form "#12345"
10901091 return array( substr( $target, 1 ), Block::TYPE_AUTO );

Follow-up revisions

RevisionCommit summaryAuthorDate
r94351merge r94350robin14:33, 12 August 2011
r94420Revert r94351, which merged r94350 from trunk to 1.18: code was unreviewed an...catrope17:00, 13 August 2011
r964701.17wmf1: MFT r92962, r93062, r93093, r93385, r93468, r93473, r94350, r94502,...catrope19:07, 7 September 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r91886(bug 29797) Error: "Tried to load block with invalid type" when subpages are ...robin16:53, 11 July 2011

Comments

#Comment by Catrope (talk | contribs)   16:58, 13 August 2011

Causes a test failure: 

	
Error:

BlockTest::testInitializerFunctionsReturnCorrectBlock
Argument 1 passed to Block::equals() must be an instance of Block, null given, called in /home/ci/cruisecontrol-bin-2.8.3/projects/mw/source/tests/phpunit/includes/BlockTest.php on line 68 and defined

/home/ci/cruisecontrol-bin-2.8.3/projects/mw/source/includes/Block.php:136
/home/ci/cruisecontrol-bin-2.8.3/projects/mw/source/tests/phpunit/includes/BlockTest.php:68
/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

The backtrace is a bit of a red herring. The unit test triggering the error is this: 

	/**
	 * CheckUser since being changed to use Block::newFromTarget started failing
	 * because the new function didn't accept empty strings like Block::load()
	 * had. Regression [https://bugzilla.wikimedia.org/show_bug.cgi?id=29116 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 ) );
	}

So what happens is that newFromTarget() returns null (which is the real issue), and equals() barfs on it (which is what causes the error).

#Comment by 😂 (talk | contribs)   17:02, 13 August 2011

This probably means we need additional test cases as well.

#Comment by Catrope (talk | contribs)   17:04, 13 August 2011

Yes, you'd need new test cases to cover the bug fix.

#Comment by Catrope (talk | contribs)   19:01, 13 August 2011

...and now I can't reproduce the test failure on trunk. CruiseControl has also been reproducing it intermittently. This is really weird.

#Comment by SPQRobin (talk | contribs)   18:57, 16 August 2011

I don't have phpunit installed to test it, but maybe it is a solution to put the IP code back, and replace if( strpos( $target, '/' ) !== false ) with if( strpos( $target, '/' ) !== false && !( IP::isValid( $target ) || IP::isValidBlock( $target ) ) ) ?

#Comment by Johnduhart (talk | contribs)   02:32, 29 August 2011

Can we remove fixme? It seems fine now

#Comment by Catrope (talk | contribs)   18:31, 5 September 2011

Marking OK. The block test failure is very intermittent and does not seem to be caused by this revision.

Status & tagging log