User:Catrope/Extension review/Incubator: Difference between revisions

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)

3. 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)
4. Some messages are added to the HTML unescaped. This is needed sometimes, but should be avoided when not needed.
5. Not using proper query building everywhere.
6. Some Incubator-specific things like project prefixes are configurable, but the config is ignored in favor of hardcoding about half the time,
7. 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 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)