User:Catrope/Extension review/Incubator

INCUBATOR REVIEW /trunk/extensions/WikimediaIncubator @ r88357

Most important issues:
 * 1) Will break on the deployment branch because Linker::link isn't static there yet, and because it's being called in a broken way
 * 2) Will break for language codes longer than three letters or containing non-letter characters (e.g. nds-nl, zh-cn)
 * It is the current policy not to allow non-ISO 639 codes. I will change it if needed. (Btw, Language::isValidCode seems to return the value '11' for some reason). SPQRobin 16:37, 20 May 2011 (UTC)
 * OK, that's fine. Just make sure it's documented then. --Catrope 17:17, 20 May 2011 (UTC)
 * 1) 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)
 * 2) Some messages are added to the HTML unescaped. This is needed sometimes, but should be avoided when not needed.
 * 3) Not using proper query building everywhere.
 * 4) Some Incubator-specific things like project prefixes are configurable, but the config is ignored in favor of hardcoding about half the time,
 * 5) 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)
 * 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 introduced
 * The 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 static
 * IncubatorTest::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 codes
 * Indentation in the if ( is_array(..) ) block is weird
 * if ( !$wgTitle->exists ) block and its elseif block are not indented
 * Use wfUrlencode instead of urlencode
 * efLoadViewUserLangLink
 * should also be a static function in a class
 * makeKnownLinkObj 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)
 * What's up with the double percent signs? — I thought that was the correct way to get "any string". It works at least.. SPQRobin 21:24, 20 May 2011 (UTC) — Not used any longer anyway now that it uses buildLike. 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 88490. SPQRobin 21:36, 20 May 2011 (UTC)

RC fix done in 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 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)