r98729 MediaWiki - Code Review archive

Commit message cut off.

[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.

• Removed left-overs from SpecialGadgetManager

• Redesign the layout. Previously it was mostly just a long unordered list with titles, descriptions and comma-separated lists for the metadata. Redesigned to be less tech-savvy. Using Vector-style icons and progressive enhancements.

• The export-feature of the old SpecialGadgets was kept. Although changed to use modern core interface and different page collections. The gadget manager structor also made an annoying thing obsolete – the need to manually copy-paste the definition on to MediaWiki:Gadgets-definition (which Special:Export/Import can't do). Since that's a separate wiki page. We can now completely export and import an entire gadget including it's definition, js/css resources and interface messages and translations. Awesome!

• The permalink-system is still a stub but it's there (Special:Gadgets/id). For now this is only linked from the edit summaries used by the gadget manager when saving to NS_GADGET_DEFINITION.

• Added a group of boolean variables to gadgetConf in mw.config. Related to whether the current logged-in user has the ability to gadgets-edit, gadget-definition-create, gadget-definition-edit and gadget-definition-delete. We need those in JS.
  • In JS checking these variables to determine wether to add a 'Create' tab, and whether or not to add the 'modify gadget' and 'delete gadget' tool links to each gadget block.

• Reorganize i18n messages since SpecialGadgetManager is no more. (also small edit in ApiQueryGadgetCategories.php to use the new message key)

• Moved some logic out of SpecialGadget::execute into their own method. SpecialGadgets::execute is now purely an interpreter of the query-string and the subpage-syntax and dispatching from there calling the appropriate method.

• Creating modify/delete links from php now instead of from JavaScript. In PHP they link to the NS_GADGET_DEFINITION pages (action=[edit/delete]). JavaScript then binds events to them to trigger the modal dialogs. This also prevents any remote scenario where the links 'jump' as they're already there now.

• Added new utility function getDefinitionTitleFromID to avoid repeating or decentralizing this logic. Perhaps this can get a better place in one of the backend classes. Put in GadgetHooks for now (next to getIDFromTitle)

The 'gadgets-nosuchaction' 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.

+				GadgetHooks::getDefinitionTitleFromID( $gadget->getId() ),
[...]
+			$t = Title::makeTitleSafe( NS_MEDIAWIKI, $gadget->getTitleMessageKey() . $suffix );

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.

+				array( 'title' => wfMessage( 'gadgets-message-edit-tooltip', $t->getPrefixedText() ) ),

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.)

+			Title::newFromText( 'Special:Recentchanges/namespace=' . NS_GADGET_DEFINITION )->getPrefixedText()

You'll want to use localized titles here, using SpecialPage::getTitleFor( 'Recentchanges', 'namespace=' . NS_GADGET_DEFINITION )

+				$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() ) ),

Again, $t is not checked for null and ->plain() is not called on the tooltip Message object.

+		// NS_GADGET_DEFINITION page of this gadget
+			$exportTitles[] = GadgetHooks::getDefinitionTitleFromID( $gadget->getId() );
+
+		// Title message in NS_MEDIAWIKI
+				$exportTitles[] = Title::makeTitleSafe( NS_MEDIAWIKI, $gadget->getTitleMessageKey() );

Indentation?!? It's messed up for the next 10-15 lines as well.

+	 * Exports a gadget with its dependencies in a serialized form.

The word "Exports" is misleading and suggests the functionality is related to Special:Export.

+		// this from Skin::subPageSubtitle. Slightly modified though

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.

+				if ( ga.conf.userIsAllowed['gadgets-definition-delete'] ) {
+					$el.find( '.mw-gadgets-modify' ).click( function( e ) {

This typo was fixed in r98837.

OK otherwise, but marking fixme for lack of null checks and lack of explicit ->plain() calls.

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.

getDefinitionTitleFromID 's documentation covers for it returning in instance of Title or null. Fixed the other calls to Title::makeTitleSafe in r98876.

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.)

Together with other message fine-tuning, done in r98938.

+		$out->addWikiMsg( 'gadgets-pagetext',
+			Title::newFromText( 'Special:Recentchanges/namespace=' . NS_GADGET_DEFINITION )->getPrefixedText()
+		);

You'll want to use localized titles here, using SpecialPage::getTitleFor( 'Recentchanges', 'namespace=' . NS_GADGET_DEFINITION )

The first argument is used as a link target ([[$1|foo]]). 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.

The word "Exports" is misleading and suggests the functionality is related to Special:Export.

It is indeed related. This code was mostly kept from the old Gadgets extension. It's a hidden form that posts to Special:Export.

+		// Would be nice if we wouldn't have to duplicate
+		// this from Skin::subPageSubtitle. Slightly modified though

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.

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.

Re the exports issue: what I meant is that that comment is also used for showSingleGadget(), which looks like a copypaste snafu now that I look at it again.