r84475 MediaWiki - Code Review archive

You do realise if you're fixing separate issues, you can commit separately? ;)

I had been doing that, arguably overdoing it. This one kinda spiralled out of control and I couldn't find a way to easily split it up... :D I've made so many overall changes to this area in the past fortnight that it will need to be reviewed with a wholistic eye anyway.

This broke LiquidThreads, especially Special:NewMessages. Now getting:

[21-Mar-2011 22:48:59] PHP Notice: Use of Block::load is deprecated. [Called from Block::newFromDB in /www/w/includes/Block.php at line 98] for /wiki/Special:NewMessages

• GlobalFunctions.php line 3129 calls wfBacktrace()
• GlobalFunctions.php line 3097 calls wfWarn()
• Block.php line 168 calls wfDeprecated()
• Block.php line 98 calls Block::load()
• Block.php line 714 calls Block::newFromDB()
• User.php line 2727 calls Block::doAutoblock()
• User.php line 1150 calls User::spreadBlock()
• User.php line 1360 calls User::getBlockedStatus()
• User.php line 1375 calls User::isBlocked()
• Title.php line 1545 calls User::isBlockedFrom()
• Title.php line 1612 calls Title::checkUserBlock()
• Title.php line 1178 calls Title::getUserPermissionsErrorsInternal()
• Thread.php line 1568 calls Title::userCan()
• Thread.php line 1546 calls Thread::canUserCreateThreads()
• View.php line 1153 calls Thread::canUserReply()
• View.php line 1272 calls LqtV in /www/w/includes/GlobalFunctions.php on line 3134 [21-Mar-2011 22:48:59] PHP Fatal error: Call to a member function getTitle() on a non-object in /www/w/extensions/LiquidThreads/classes/Hooks.php on line 421

r84498 indeed fixes the notice. Fatal still there.

Notice: Undefined index: DisableEmail in C:\wamp\www\MW_trunk\includes\specials\SpecialBlock.php on line 542

Notice: Undefined index: DisableEmail in C:\wamp\www\MW_trunk\includes\specials\SpecialBlock.php on line 749

These were fixed in r84522. I believe everything outstanding on this revision has been dealt with; including a mysterious bug on TWN which turned out to have been Siebrand merging a blocked user with himself (r84584).

You change the decleration of userBlockedMessage() in SpecialUserlogin, but didn't update its caller at line 714

Done in r88663. I think that codepath might be unreachable, or maybe only by spoofing forms.

Block::parseTarget is supposed to return an array, I think this change made it not return an array in some cases, maybe causing bug 29032.

That's certainly not right. Fixed in r88664, don't know whether it fixes the bug you mentioned.

Whoops, meant bug 29116.

Marking FIXME; problems that are still present implicated in bug 29116

Was this resolved by your r88750?

Presumably this line (since it's the only core code-changing line in that revision):

-		} elseif( $target === null && $vagueTarget === null ){
+		} elseif( $target === null && $vagueTarget == '' ){

I'll assume so since the bug is marked FIXED. Resetting to new.

I don't know if the reported PHP Notices were only triggered if certain extensions are installed, but if not already make sure you

• test with both debug mode set to true and to false in your local trunk
• and have error_reporting set to ALL( error_reporting(E_ALL);ini_set('display_errors', 1);), and verifying that there are none before committing.

+ if ( $block->getType == Block::TYPE_RANGE ) {

Typo, gives error message.

+ $this->userBlockedMessage( $wgUser->isBlockedFromCreateAccount() );

Does this expect a Block input?

Nevermind, isBlockedFromCreateAccount() returns the right type here.

On the other hand, due to the static $accBlock var and the wfGetIP() call, calling isBlockedFromCreateAccount() only makes sense on wgUser. Maybe that should be enforced?

Are static variables in methods shared between separate instances of the same class, then? If so, that's definitely not the way to cache it :S

Yes, that's what makes them static ;-). They're accessed through the class name, not from the instance (eg. SomeClass::staticMember

See http://www.php.net/manual/en/language.variables.scope.php#53915

Oh you meant a static variable within an public method. Hm.. that's interesting. Gonna try that one out.

Converted to a member variable in r90859

+ $this->mBlockedby = $this->mBlock->getBlocker()->getName();

Won't this fatal if there is no blocker? Maybe use Block::getBy() and such.

253 	# Don't use anon only blocks on users
254 	if( $this->type == self::TYPE_USER && !$block->isHardblock() ){
255 	continue;
256 	}

Can't you just add this to the query conds?

Trying to translate the "target is a user" condition into pure SQL would be difficult, and would introduce potential inconsistencies with the behaviour of Block::parseTarget().

Fair enough, I was getting hung up on the $block->isHardblock() part, which would be easy by itself.

+	 * @deprecated since 1.18

When you deprecate methods can you please explain why and/or what the new method is? Thank you.