User "Catrope" changed the status of MediaWiki.r98729. Old Status: new New Status: fixme
User "Catrope" also posted a comment on MediaWiki.r98729. Full URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/98729#c23714 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: The <code>'gadgets-nosuchaction'</code> message is used in two different cases: one where the action really doesn't exist, and one where the action is export and the gadget doesn't exist. In the latter case, using the nosuchaction message is confusing. <pre> + GadgetHooks::getDefinitionTitleFromID( $gadget->getId() ), [...] + $t = Title::makeTitleSafe( NS_MEDIAWIKI, $gadget->getTitleMessageKey() . $suffix ); </pre> 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. <pre> + array( 'title' => wfMessage( 'gadgets-message-edit-tooltip', $t->getPrefixedText() ) ), </pre> Please explicitly call <code>->plain()</code> 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.) <pre> + Title::newFromText( 'Special:Recentchanges/namespace=' . NS_GADGET_DEFINITION )->getPrefixedText() </pre> You'll want to use localized titles here, using <code>SpecialPage::getTitleFor( 'Recentchanges', 'namespace=' . NS_GADGET_DEFINITION )</code> <pre> + $t = Title::makeTitleSafe( NS_MEDIAWIKI, "gadgetcategory-{$category}{$suffix}" ); + $editLink = Linker::link( + $t, + wfMessage( 'gadgets-message-edit' )->escaped(), + array( 'title' => wfMessage( 'gadgets-message-edit-tooltip', $t->getPrefixedText() ) ), </pre> Again, $t is not checked for null and ->plain() is not called on the tooltip Message object. <pre> + // NS_GADGET_DEFINITION page of this gadget + $exportTitles[] = GadgetHooks::getDefinitionTitleFromID( $gadget->getId() ); + + // Title message in NS_MEDIAWIKI + $exportTitles[] = Title::makeTitleSafe( NS_MEDIAWIKI, $gadget->getTitleMessageKey() ); </pre> Indentation?!? It's messed up for the next 10-15 lines as well. <pre> + * Exports a gadget with its dependencies in a serialized form. </pre> The word "Exports" is misleading and suggests the functionality is related to Special:Export. <pre> + // this from Skin::subPageSubtitle. Slightly modified though </pre> sofixit. It looks like the only thing wrong with subPageSubtitle() is that it checks for <code>if ( $out->isArticle() && MWNamespace::hasSubpages( $out->getTitle()->getNamespace() ) ) {</code> . The code inside that could be factored out into another function, which SpecialGadgets would then call directly. <pre> + if ( ga.conf.userIsAllowed['gadgets-definition-delete'] ) { + $el.find( '.mw-gadgets-modify' ).click( function( e ) { </pre> This typo was fixed in r98837. OK otherwise, but marking fixme for lack of null checks and lack of explicit ->plain() calls. _______________________________________________ MediaWiki-CodeReview mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
