User "Krinkle" changed the status of MediaWiki.r98729. Old Status: fixme New Status: new
User "Krinkle" also posted a comment on MediaWiki.r98729. Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/98729#c23772 Commit summary: [RL2] Merge gadget manager into SpecialGadgets * Rewrite SpecialGadgets to use the new repository backend. While at it, made it use context variables instead of globals.-- Comment: <blockquote><em>getDefinitionTitleFromID() and makeTitleSafe() can return null, but this is not checked. In the latter case (which occurs twice), that can lead to a fatal. Marking fixme.</em></blockquote> getDefinitionTitleFromID 's documentation covers for it returning in instance of Title or null. Fixed the other calls to Title::makeTitleSafe in r98876. <blockquote><em>Please explicitly call ->plain() on the Message object here. Best I can tell, the default behavior of Message::toString() seems to be 'parse', which is not what you want here. (Occurs twice.)</em></blockquote> Together with other message fine-tuning, done in r98938. <blockquote><pre> + $out->addWikiMsg( 'gadgets-pagetext', + Title::newFromText( 'Special:Recentchanges/namespace=' . NS_GADGET_DEFINITION )->getPrefixedText() + ); </pre><em>You'll want to use localized titles here, using SpecialPage::getTitleFor( 'Recentchanges', 'namespace=' . NS_GADGET_DEFINITION )</em></blockquote> The first argument is used as a link target (<code><nowiki>[[$1|foo]]</nowiki></code>). When the linker parses this it localizes the url. That's why I didn't see it on my local dutch wiki (I specifically tested this to make sure it wouldn't parse a link that redirects). However were $1 to be used in a different way, then it'd be English only. Fixed in r98940. Whitespace fixed in r98940 also. <blockquote><em>The word "Exports" is misleading and suggests the functionality is related to Special:Export.</em></blockquote> It is indeed related. This code was mostly kept from the old Gadgets extension. It's a hidden form that posts to Special:Export. <blockquote><pre> + // Would be nice if we wouldn't have to duplicate + // this from Skin::subPageSubtitle. Slightly modified though </pre> <em>sofixit. It looks like the only thing wrong with subPageSubtitle() is that it checks for if ( $out->isArticle() && MWNamespace::hasSubpages( $out->getTitle()->getNamespace() ) ) { . The code inside that could be factored out into another function, which SpecialGadgets would then call directly.</em></blockquote> That's something we could do later in core. Note though that the code that follows has been slightly modified (as noted), I'm not sure the core function would be useful even if the code inside that if statement would be moved into a separate Skin-class method. _______________________________________________ MediaWiki-CodeReview mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
