Jump to content

User:Catrope/Extension review/WikiLove

From mediawiki.org

WIKILOVE REVIEW /trunk/extensions/WikiLove @ r88424

See also User:Krinkle/Extension_review/WikiLove for more points.

General

  • Coding style not followed in JS. PHP seems mostly fine and a stylize.php run will take care of that, but AFAIK JS stylization is not automated

WikiLove.php

  • "Not a final schema" thing should be removed and the schema hook invocation uncommented.
  • API module names are typically all lowercase (action=wikilove as opposed to action=wikiLove).
  • Config vars should be nearer the top of the file (before hook and autoloader stuff) and have clear docs for each variable

jquery.elastic.js

  • Please replace this with an unminified version and add a comment saying where you got this plugin from.
  • If this is generally useful (seems to be), it should be moved to core.
  • curratedHeight in setHeightAndOverflow() is leaked to the global scope

WikiLoveLog.sql

  • The `backticks` aren't needed
  • The wl_ prefix is already used by the watchlist table, I think wll_ would be better
  • Fields are completely undocumented
  • No indexes besides primary key. What kind of queries will be run on this table? At the very least an index on timestamp seems useful.

WikiLove.hooks.php:3

  • Don't use inline scripts. Instead, export the variable through the MakeGlobalVariablesScript hook
    • Do you even need it? You can get an edit token through AJAX

WikiLove.api.php:

  • 'invalidtitle' is being thrown when the specified title is not a user or user talk page. This is misleading, because the title isn't invalid.
  • Title::setFragment() is deprecated for public use
  • What's the wl_email field for if it's hard-set to 0?
  • Don't use dieUsage() for a warning. If you want a warning, set a warning. If you want it to die, just remove all the query exception handling and the API will take care of it

defaultTypes.js:

  • Shouldn't this stuff be in localizable messages?

wikiLove.css:

  • Actually, !ie works in IE7 as well AFAIK

wikiLove.js:

  • mw.loader.load() is asynchronous so the config stuff may not arrive in time. Use $.getScript( 'script url', function() { code executed when script loaded } ); instead
  • getEmailable(): a getter that doesn't return anything but just sets a value is weird. It's just one value, so you could kill the function and just put it in openDialog(), right?
  • The <img src="blah"> tags are all built without escaping the src attribute. Escape it using mw.html.escape() or build the tag with jQuery
    • Same for $buttonInside.append( '<div class="wlLinkText">' + $.wikiLove.types[typeId].name + '</div>' );
    • Same for all the messages, e.g. .append( '<h3>' + mw.msg( 'wikilove-select-type' ) + '</h3>' )
    • Same for $( '<option>' + subtype.option + '</option>' )
    • Same for $( '#wlPreview' ).append( '<div class="wlError">' + data.error.info + '<div>' );
  • Re $addDetails: rather than calling .append() a million times, you can put stuff in a long single-quoted string with newlines escaped with backslahes, and use <html:msg> and $.localize() to do messages. ArticleFeedback uses this strategy, focusing on putting as much of the skeleton as possible in a single string and filling the rest (like src attributes) in dynamically with jQuery
  • showError() should use .text() to prevent HTML injection through messages
  • msg.replace() stuff in prepareMsg() is broken, replaces only the first occurrence
  • if ( foo ) { var bar = foo; } else { var bar = 'baz'; } can be written as var bar = foo || 'baz';
  • $.wikiLove.init() call should be done on document ready
  • $button, $buttonInside and $img are being exported to the global scope (JSHint caught this)
  • JSHint also wants you to use !== null instead of != null
Retrieved from "https://www.mediawiki.org/w/index.php?title=User:Catrope/Extension_review/WikiLove&oldid=404609"