User:Catrope/Extension review/Incubator
Appearance
INCUBATOR REVIEW /trunk/extensions/WikimediaIncubator @ r88357
Most important issues:
- Will break on the deployment branch because Linker::link() isn't static there yet, and because it's being called in a broken way
- Will break for language codes longer than three letters or containing non-letter characters (e.g. nds-nl, zh-cn)
- Uses a broken way of adding a hidden input field to the user creation form, and will break other extensions using the same broken method (ConfirmAccount and ConfirmEdit)
- Some messages are added to the HTML unescaped. This is needed sometimes, but should be avoided when not needed.
- Not using proper query building everywhere.
- Some Incubator-specific things like project prefixes are configurable, but the config is ignored in favor of hardcoding about half the time,
- Code duplication in some places
General:
- Coding style not followed everywhere. I'll run stylize
CreateAccountTestWiki.php:
AutoTestWiki functions are called statically through $wgHooks, so they should be declared as public static(2) In AutoTestWiki::onUserCreateForm(): /[a-z][a-z][a-z]?/ doesn't match language codes like nds-nl. Use Language::isValidCode()introduced IncubatorTest::validateLanguageCode() that can be updated when needed. SPQRobin 21:24, 20 May 2011 (UTC)- (3) To be nice to other extensions, AutoTestWiki::onUserCreateForm() should append to the header instead of overwriting it
- UsercreateTemplate has a convenience function for this called addInputItem(). For example usage see TitleBlacklistHooks::addOverrideCheckbox()
- TODO This does not work with 'hidden'. I thought I'd use select & text instead, but select is not possible. I should find a solution for this... SPQRobin 21:24, 20 May 2011 (UTC)
- UsercreateTemplate has a convenience function for this called addInputItem(). For example usage see TitleBlacklistHooks::addOverrideCheckbox()
AutoTestWiki::onAddNewAccount() should do the same validation as onUserCreateForm()
IncubatorTest.php:
IncubatorTest::onGetPreferences()$wmincPrefProject and $wmincPrefNone aren't used anywhere and aren't declared in the extension setup file(2) Max length 3 for language code preference doesn't make sense: there are longer language codes, e.g. nds-nl$wmincLangCodeLength introducedThe callback for the language code validation is wrongly capitalized (CodeValidation vs. codeValidation), although PHP doesn't seem to care— Fixed & made name more descriptive SPQRobin 21:24, 20 May 2011 (UTC)
All functions in IncubatorTest are called statically, so they should be declared as staticIncubatorTest::isNormalPrefix() isn't very nicely written: it should take the prefix as a parameter.— Fixed & made name more descriptive SPQRobin 21:24, 20 May 2011 (UTC)You can also use return is_array(...); instead of if ( in_array(...) ) { return true; } else { return false; }Did return (bool) in_array(...); SPQRobin 21:24, 20 May 2011 (UTC)
IncubatorTest::displayPrefix(): don't use == true, just use if ( self::isNormalPrefix() )(7) IncubatorTest::displayPrefixedTitle() should take a Title object call $title->getPrefixedText() to get a namespace:title string instead of reinventing the wheel— solved it differently using Title SPQRobin 21:24, 20 May 2011 (UTC)IncubatorTest::editPageCheckPrefix()(6) 'inc' is hardcoded, use $wmincProjectSite['short'] instead(6) Set of namespaces is hardcoded, should probably be configurable— $wmincTestWikiNamespaces introduced(2,6) Regex is hardcoded but inferred from stuff in $wmincProjects. Also uses three-letter language codesIndentation in the if ( is_array(..) ) block is weirdif ( !$wgTitle->exists() ) block and its elseif block are not indentedUse wfUrlencode() instead of urlencode()
efLoadViewUserLangLink()should also be a static function in a classmakeKnownLinkObj() is deprecated, use link()
SpecialViewUserLang.php:
SpecialViewUserLang::execute(): instead of doing if ( $target ) { ... } else { ... } you can use $wgRequest->getText( 'target', $subpage );SpecialViewUserLang::showInfo()Don't use == true(1) Linker::link() is the new style in trunk, but this doesn't work on the live site yet. Use $sk->link() for now(1) The parameter to link() must be a Title object rather than a string(7) The logic for building the string fed to link() is duplicated from IncubatorTest::displayPrefix()(4) In the code paths where $testwiki is not set to the return value of link(), it should be escaped with htmlspecialchars()I did htmlspecialchars( $wmincProjectSite['name'] ); if that is what you mean. SPQRobin 21:24, 20 May 2011 (UTC)(4) All of the wfMsg() calls inside the $wgOut->addHTML() call should be changed to call wfMsgHtml() instead so those messages are escaped— Not done for 'wminc-userdoesnotexist' because then it is double espaced. SPQRobin 21:26, 20 May 2011 (UTC)
TestWikiRC.php:
- TestWikiRC::onRcQuery()
(7) The logic for building $fullprefix is duplicated from IncubatorTest::displayPrefix()I know, but I had left it for a possible better solution :) Anyway, now it uses displayPrefix(). SPQRobin 21:24, 20 May 2011 (UTC)Use || instead of OR(5) Use $dbr->buildLike() to build the NOT LIKE query — I know it should, but I do not know how to do it in combination with the OR query. SPQRobin 21:24, 20 May 2011 (UTC)OR not needed actually, so it's fixed now. SPQRobin 18:59, 21 May 2011 (UTC)rc_title NOT LIKE 'foo' OR 'bar' doesn't seem to work as intended. You seem to want rc_title NOT LIKE 'foo' AND rc_title NOT LIKE 'bar' — It does work actually.. SPQRobin 21:24, 20 May 2011 (UTC)OR not even needed actually. SPQRobin 18:59, 21 May 2011 (UTC)(5) Don't build your own IN(...) list with makeList(), just use $conds['rc_namespace'] = $namespaces;That's a lot easier indeed.. :) SPQRobin 21:24, 20 May 2011 (UTC)(6) Hardcoded set of namespaces appears again$wmincTestWikiNamespaces introduced
- TestWikiRC::onRcForm()
(2) Max length for language codes set to 3 again$wmincLangCodeLength introduced- rc-testwiki-project and rc-testwiki-code seem to be loaded into $opts by onRcQuery(), then taken out by onRcForm(), but the latter doesn't use their values. What's going on here? — I don't know how it *should* be done according to MediaWiki coding standards, but it does work... SPQRobin 19:18, 21 May 2011 (UTC)
Done in rev:88490. SPQRobin 21:36, 20 May 2011 (UTC)
RC fix done in rev:88529. Only thing left is finding a solution for the 'hidden' in CreateAccountTestWiki.php (and possibly your last remark about onRcForm()) but I don't think that should hold it back from deployment :). SPQRobin 18:59, 21 May 2011 (UTC)
In rev:88925, I fixed a bug in ViewUserLang, as well as changed a few functions that I think were necessary before deployment (otherwise I would have waited with committing them). SPQRobin 20:10, 28 May 2011 (UTC)