User:Jack Phoenix/Code reviews/TextScroller

Based on: 2015-07-24 version

General

 * Run stylize.php
 * Manually or programmatically clean up EOL whitespace and inconsistent indentation (indentation should always use tabs unless otherwise specified in the coding conventions, but IIRC that applies only to certain languages like Python or Ruby, not to PHP/JS/CSS)
 * Bonus: extension.json file for MW 1.25+

TextScroller.body.php

 * Ew, short opening tag
 * Why does this extend UnlistedSpecialPage when it's a parser hook? Lose the  part altogether.
 * Don't use  in  ; instead have it take a &$parser argument and use that
 * You'll want to output the CSS and JS (via ResourceLoader) in, the callback function for  , like this:
 * Shouldn't be using EasyTemplate because EasyTemplate is wikiHow-specific (and it used to be also available on Wikia's codebase, where wikiHow's version originates from); if you insist using a template class, go with either QuickTemplate (in core since before the dawn of time) or Mustache (in core since...MW 1.25? Or 1.26+, but also available on wikiHow's codebase); IMO this can be done relatively cleanly here with a heredoc block and some local variables, though, so you don't really need a template of any kind.
 * There's a bug here in ; currently its signature looks like this:   which means that supplying less than three arguments to the   parser function would trigger an E_NOTICE and an E_WARNING for each missing param; fix it by supplying defaults:
 * Shouldn't be using EasyTemplate because EasyTemplate is wikiHow-specific (and it used to be also available on Wikia's codebase, where wikiHow's version originates from); if you insist using a template class, go with either QuickTemplate (in core since before the dawn of time) or Mustache (in core since...MW 1.25? Or 1.26+, but also available on wikiHow's codebase); IMO this can be done relatively cleanly here with a heredoc block and some local variables, though, so you don't really need a template of any kind.
 * There's a bug here in ; currently its signature looks like this:   which means that supplying less than three arguments to the   parser function would trigger an E_NOTICE and an E_WARNING for each missing param; fix it by supplying defaults:

textscroller.css

 * See ; just spacing stuff in this file

TextScroller.i18n.php

 * Shouldn't exist, should be converted to JSON i18n (it's present in MW 1.23+)
 * run, then remove this file and change   to   in TextScroller.php

textscroller.js

 * Coding style (spacing etc.)
 * is no more, can't use it

TextScroller.php

 * check can now be safely removed as long as you don't have  enabled; MW 1.25+ will no longer run if that deprecated PHP setting is enabled
 * &rarr;, PHP 5.3+ has been required for a while and therefore it's totally OK to use PHP 5.3+ functions
 * Clean up the  stuff; that define check is for MW 1.12 (!), all you need here is
 * Extension version + MW.org infopage URL into
 * Register ResourceLoader modules here; suggested to register a separate module for the CSS file and another for the JS file, as mentioned earlier
 * I18n people will insist on making the extension description (shown on Special:Version) translatable; while I personally disagree with that practise, it's somewhat of an estabilished standard these days

textscroller.tmpl.php

 * Shouldn't exist as-is because EasyTemplate is not in core