User:Catrope/Extension review/WikiLove

WIKILOVE REVIEW /trunk/extensions/WikiLove @ r88424

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 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( ' ' + $.wikiLove.types[typeId].name + ' ' );
 * Same for all the messages, e.g. .append( ' ' + mw.msg( 'wikilove-select-type' ) + ' ' )
 * Same for $( ' ' + subtype.option + ' ' )
 * Same for $( '#wlPreview' ).append( ' ' + data.error.info + ' ' );
 * 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  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