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+
TextScroller.body.php edit
- 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
insetParserFunction
; 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 forsetParserFunction
, 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 = '' ) {
textscroller.css edit
- See #General; just spacing stuff in this file
TextScroller.i18n.php edit
- 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
- run
textscroller.js edit
- Coding style (spacing etc.)
$.browser
is no more, can't use it
TextScroller.php edit
if ( ! defined( 'MEDIAWIKI' ) )
check can now be safely removed as long as you don't haveregister_globals
enabled; MW 1.25+ will no longer run if that deprecated PHP setting is enableddirname(__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
textscroller.tmpl.php edit
- Shouldn't exist as-is because EasyTemplate is not in core