r100863 MediaWiki - Code Review archive

• There are some minor spacing/brace "issues", i.e. in AutoProxyBlock::checkProxy(), if( in_array( $action, $wgProxyCanPerform ) || $user->isAllowed('proxyunbannable') ) return true; should really be written like this to be perfectly in line with our coding standards:

if( in_array( $action, $wgProxyCanPerform ) || $user->isAllowed( 'proxyunbannable' ) ) {
	return true;
}

• Also, in AutoProxyBlock::checkProxy(), calling the variable containing the result of wfGetIP() "$IP" is technically correct, but it's confusing, because there is already a global variable called $IP (which stands for Install Path), so I'd recommend changing that to something like $ipAddress or whatever you feel is suitable
  • 'bkip' => "$ip", is silly, you don't need the "double quotes" there (IIRC)
• AutoProxyBlock::requestForeighAPI() seems like a hacky function that really shouldn't exist
  • On that note, there's a syntax error in that function (missing semicolon): $content = Http::get($url)
• There are some long lines that could and should be split up, i.e. in AutoProxyBlock::tagProxyChange():

		if ( $wgTagProxyActions && self::isProxy( wfGetIP() ) && !$wgUser->isAllowed( 'notagproxychanges' ) ) {
			ChangeTags::addTags( 'proxy', $recentChange->mAttribs['rc_id'], $recentChange->mAttribs['rc_this_oldid'], $recentChange->mAttribs['rc_logid'] );
		}

would look better like this:

		if ( $wgTagProxyActions && self::isProxy( wfGetIP() ) && !$wgUser->isAllowed( 'notagproxychanges' ) ) {
			ChangeTags::addTags(
				'proxy',
				$recentChange->mAttribs['rc_id'],
				$recentChange->mAttribs['rc_this_oldid'],
				$recentChange->mAttribs['rc_logid']
			);
		}

Remember, space is cheap and readability is everything. :)

• AutoProxyBlock::addInternalLogEntry() is somewhat hackish and it has hardcoded English. [[Extension:CheckUser|]] used to store its logged data in a text file on the server until it was rewritten to use a database table instead; I'm thinking that you could do something similar here (store the log data on a new database table and create a special page to view the data).
• Having a version number in $wgExtensionCredits (the 'version' key) might be helpful
• The AutoProxyBlock class is very scarcely commented...some comments would make reviewers' (and maybe even end-users') life a lot easier; for example, for me it's obvious that the function AFSetVar adds a new variable called is_proxy to AbuseFilter's list of available variables that the user can use in the edit filters, but it won't be obvious for most people reading the code, as I assume that most people aren't very familiar with AbuseFilter.

See r100972.

Core function LogPage::addEntry is broken for this extension due to MediaWiki Special:Log can't display log entries by anonymous users (with user ID 0).

About "AutoProxyBlock::requestForeighAPI": core has a functions to work with foreign MediaWiki APIs?

The foreign API requests should be cached in $wgMemc and the overall isProxy() check should have a simple process cache, like a static var. For testing though, you'd want to split up isProxy() into one function with without a process cache and another which is a wrapper around the former.