r83909 MediaWiki - Code Review archive

Have extensions been checked for the Ipblocklist -> BlockList change?

Everything in svn should be correct in terms of PHP code. There are still quite a lot of references in messages (98 in /extensions/Crosswiki/Crosswiki.i18n.php, for instance), which should resolve but should nonetheless be updated in due course; and a few random occurences in comments and non-code files.

Some errors on TWN:

PHP Warning:  in_array() expects parameter 2 to be array, null given in /www/w/includes/specials/SpecialBlockList.php on line 166
PHP Warning:  in_array() expects parameter 2 to be array, null given in /www/w/includes/specials/SpecialBlockList.php on line 169
PHP Warning:  in_array() expects parameter 2 to be array, null given in /www/w/includes/specials/SpecialBlockList.php on line 172
PHP Notice:  Undefined property: BlockListPager::$mPage in /www/w/includes/specials/SpecialBlockList.php on line 422
PHP Fatal error:  Call to a member function getTitle() on a non-object in /www/w/includes/specials/SpecialBlockList.php on line 422

Should be fixed by r83966.

The URL parameters looks very uncommons: http://translatewiki.net/wiki/Special:BlockList?wpTarget=&wpOptions[]=userblocks&wpOptions[]=tempblocks&wpOptions[]=addressblocks

It seems it works but it this intended?

That's the way MultiSelect fields work, and as you say the URL links work fine. I do notice, however, that the format is not recognised by the internal link syntax, I've fixed that in r84022.

Please update the other Messages*.php-Files, because there are no local aliases for Special:BlockList in all languages unequal to en. Or it is bad to reuse the local aliases? Thanks.

Not being a speaker of all 243 languages, I don't know whether the old aliases are appropriate for the new name; whether they're a literal transliteration of "Ipblocklist" (in which case they need to be updated, not just renamed) or a proper translation of "page which lists blocks". Hence why we have translatewiki.

Old aliases should always keep working.

Can TWN handle the renaming of the 'ipblocklist' messages?

+ $formatted = wfMessage( 'autoblockid', $row->ipb_id );

How does that work? Should it return HTML?

+ static $headers = null;

Why does getFieldNames() use a static var? Is it called often or something?

line 123 	if ( !$wgUser->isAllowed( 'hideuser' ) ){

This check is already there in the Pager class.

+	protected static function getIpFragment( $ip ){
+		global $wgBlockCIDRLimit;
+		if( IP::isIPv4( $ip ) ){
+			$hexAddress = IP::toHex( $ip );
+			return substr( $hexAddress, 0,  wfBaseconvert( $wgBlockCIDRLimit['IPv4'], 10, 16 ) );
+		} elseif( IP::isIPv6( $ip ) ) {
+			$hexAddress = substr( IP::toHex( $ip ), 2 );
+			return 'v6-' . substr( $hexAddress, 0,  wfBaseconvert( $wgBlockCIDRLimit['IPv6'], 10, 16 ) );

I don't think this works like you think. Try:

$hexAddress = IP::toHex( '122.114.14.5' );
var_dump( $hexAddress  );
var_dump( wfBaseconvert( 16, 10, 16 ) );
var_dump( $hexAddress, 0, wfBaseconvert( 16, 10, 16 ) );

Which gives...

string(8) "7A720E05"
string(2) "10"
string(8) "7A720E05"

If the min prefix is 16 bits, and each hex char is 4 bits, you'd want floor(16/4) = 4. Maybe use ($wgBlockCIDRLimit['IPv4']/4) in place of " wfBaseconvert( $wgBlockCIDRLimit['IPv4'], 10, 16 )" and likewise for v6?

Gah, for the test code, I meant:

$hexAddress = IP::toHex( '122.114.14.5' );
var_dump( $hexAddress  );
var_dump( wfBaseconvert( 16, 10, 16 ) );
var_dump( substr( $hexAddress, 0, wfBaseconvert( 16, 10, 16 ) ) );

Looks like getIpFragment() was fixed in r84358.

+ $formatted = wfMessage( 'autoblockid', $row->ipb_id );

How does that work? Should it return HTML?

The Message class has a __toString() magic method.

+ static $headers = null;

Why does getFieldNames() use a static var? Is it called often or something?

Actually no, it's not. Doesn't do any harm, per se, though.

I'd very much prefer if you just call parse() or what not on the message to be more explicit :)

Did this one myself (r91918).

This is a serious performance regression. Removing the LinkBatch and ipb_by_text code results in 3 DB queries being done for each block, with a noticeable performance impact in production.

Handled by r100254.