User "Catrope" posted a comment on MediaWiki.r95572.
Full URL:
https://secure.wikimedia.org/wikipedia/mediawiki/wiki/Special:Code/MediaWiki/95572#c21691
Commit summary:
Adds ResourceLoader support to AbuseFilter
Rewrote javascript to use jQuery
Added API modules to replace sajax_* calls
Solves bug 29714
Comment:
<pre>
+$wgAPIModules['checkfiltersyntax'] = 'ApiCheckFilterSyntax';
</pre>
I would feel safer if API module names like these were prefixed to make naming
conflicts less likely. Something like 'abusefilterchecksyntax' or
'checkabusefiltersyntax' (such that 'abusefilter' as a whole is mentioned).
The way information is passed in static member variables of
<code>AbuseFilter</code> and <code>AbuseFilterViewExamine</code> is
undocumented and kind of scary, but offhand I wouldn't know a better way. Maybe
you can avoid passing the information instead; it looks like filterBoxName
doesn't need to be passed around if you can make it so that the box always has
the same ID.
<pre>
+ data = data.checkfiltersyntax;
...
+ if ( data.status == 'ok' ) {
</pre>
You need to check whether the first assignment didn't set <code>data</code> to
<code>undefined</code>; this may happen if the API module throws an error.
<pre>
+ $filterBox.text(
data.query.abusefilters[0].pattern );
</pre>
Same here, check that these elements are present in these objects or you'll get
a ReferenceError and your entire script dies.
<pre>
+ var action = this.id.substr( 31 );
</pre>
Please add a comment explaining where the number 31 came from.
<pre>
+ }, function( data ) {
+ $( '#mw-abusefilter-warn-preview' ).html( data )
</pre>
Second line should be indented one tab more.
<pre>
+ insertAtCaret: function(myValue){
</pre>
Does <code>$.textSelection</code>'s encapsulateSelection functionality not do
what you want?
<pre>
+ $( '#mw-abusefilter-expr-result' )
+ .html( mw.html.escape( data.evalfilterexpression.result
) );
</pre>
Huh? Can't you just use <code>.text()</code> here?
<pre>
+ $vars = json_decode( $params['vars'], true );
</pre>
Use <code>FormatJson::decode()</code>. This wraps around
<code>json_decode()</code> except when the JSON module isn't present or a
version with a known bug is present, if which case it falls back to a PHP
implementation.
<pre>
+ // Same as below
...
+ // Oh god this is so bad but this message uses GENDER
</pre>
It seems you refactored this and changed the order, so these comments are
confusing at first, then amusing :)
Looks very good otherwise, thanks!
_______________________________________________
MediaWiki-CodeReview mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
