User "Jack Phoenix" posted a comment on MediaWiki.r100863.
Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/100863#c24912
Commit summary:
Adding beta-version of extension 'AutoProxyBlock' to repo
Comment:
*There are some minor spacing/brace "issues", i.e. in
<code>AutoProxyBlock::checkProxy()</code>, <code>if( in_array( $action,
$wgProxyCanPerform ) || $user->isAllowed('proxyunbannable') ) return
true;</code> should really be written like this to be perfectly in line with
our [[Manual:Coding conventions|coding standards]]:
<pre><nowiki>
if( in_array( $action, $wgProxyCanPerform ) || $user->isAllowed(
'proxyunbannable' ) ) {
return true;
}
</nowiki></pre>
*Also, in <code>AutoProxyBlock::checkProxy()</code>, calling the variable
containing the result of wfGetIP() "$IP" is technically correct, but it's
confusing, because [[Manual:$IP|there is already a global variable called
<code>$IP</code>]] (which stands for '''I'''nstall '''P'''ath), so I'd
recommend changing that to something like $ipAddress or whatever you feel is
suitable
**<code>'bkip' => "$ip",</code> is silly, you don't need the "double quotes"
there (IIRC)
*<code>AutoProxyBlock::requestForeighAPI()</code> seems like a hacky function
that really shouldn't exist
**On that note, there's a syntax error in that function (missing semicolon):
<code>$content = Http::get($url)</code>
*There are some long lines that could and should be split up, i.e. in
<code>AutoProxyBlock::tagProxyChange()</code>:
<pre><nowiki>
if ( $wgTagProxyActions && self::isProxy( wfGetIP() ) &&
!$wgUser->isAllowed( 'notagproxychanges' ) ) {
ChangeTags::addTags( 'proxy',
$recentChange->mAttribs['rc_id'], $recentChange->mAttribs['rc_this_oldid'],
$recentChange->mAttribs['rc_logid'] );
}
</nowiki></pre>
would look better like this:
<pre><nowiki>
if ( $wgTagProxyActions && self::isProxy( wfGetIP() ) &&
!$wgUser->isAllowed( 'notagproxychanges' ) ) {
ChangeTags::addTags(
'proxy',
$recentChange->mAttribs['rc_id'],
$recentChange->mAttribs['rc_this_oldid'],
$recentChange->mAttribs['rc_logid']
);
}
</nowiki></pre>
Remember, space is cheap and readability is everything. :)
*<code>AutoProxyBlock::addInternalLogEntry()</code> 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 [[Manual:$wgExtensionCredits|$wgExtensionCredits]]
(the <code>'version'</code> key) might be helpful
*The <code>AutoProxyBlock</code> 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 <code>AFSetVar</code> adds a
new variable called <code>is_proxy</code> to
[[Extension:AbuseFilter|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.
_______________________________________________
MediaWiki-CodeReview mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
