User:Jack Phoenix/Code reviews/TextScroller

Based on: 2015-07-24 version

General[edit]

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+

Ew, short opening tag
Why does this extend UnlistedSpecialPage when it's a parser hook? Lose the extends UnlistedSpecialPage part altogether.
Don't use global $wgParser in setParserFunction; instead have it take a &$parser argument and use that
You'll want to output the CSS and JS (via ResourceLoader) in parserFunction, the callback function for setParserFunction, like this:
- $parser->getOutput()->addModules( 'ext.textscroller.scripts' );
- $parser->getOutput()->addModuleStyles( 'ext.textscroller.styles' );
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 parserFunction(); currently its signature looks like this: parserFunction($parser, $arrowText, $grayText, $scrollText) which means that supplying less than three arguments to the {{#txtscrl:}} parser function would trigger an E_NOTICE and an E_WARNING for each missing param; fix it by supplying defaults: public static function parserFunction( $parser, $arrowText = '', $grayText = '', $scrollText = '' ) {

Shouldn't exist, should be converted to JSON i18n (it's present in MW 1.23+)
- run /maintenance/generateJsonI18n.php --phpfile /extensions/wikihow/textscroller/TextScroller.i18n.php, then remove this file and change $wgExtensionMessagesFiles['TextScroller'] = dirname(__FILE__) . '/TextScroller.i18n.php'; to $wgMessagesDirs['TextScroller'] = __DIR__ . '/i18n'; in TextScroller.php

if ( ! defined( 'MEDIAWIKI' ) ) check can now be safely removed as long as you don't have register_globals enabled; MW 1.25+ will no longer run if that deprecated PHP setting is enabled
dirname(__FILE__) → __DIR__, PHP 5.3+ has been required for a while and therefore it's totally OK to use PHP 5.3+ functions
Clean up the if (defined('MW_SUPPORTS_PARSERFIRSTCALLINIT')) { stuff; that define check is for MW 1.12 (!), all you need here is $wgHooks['ParserFirstCallInit'][] = 'TextScroller::setParserFunction';
Extension version + MW.org infopage URL into $wgExtensionCredits
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