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 mediawiki-coderev...@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview