r100509 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r100508‎ | r100509 | r100510 >
Date:19:09, 22 October 2011
Author:liangent
Status:resolved (Comments)
Tags:
Comment:
(bug 31414) Skin specific gadgets
Modified paths:
  • /trunk/extensions/Gadgets/ApiQueryGadgets.php (modified) (history)
  • /trunk/extensions/Gadgets/Gadgets.i18n.php (modified) (history)
  • /trunk/extensions/Gadgets/Gadgets_body.php (modified) (history)
  • /trunk/extensions/Gadgets/SpecialGadgets.php (modified) (history)

Diff [purge]

Index: trunk/extensions/Gadgets/Gadgets_body.php
@@ -239,6 +239,7 @@
240240 $definition,
241241 $resourceLoaded = false,
242242 $requiredRights = array(),
 243+ $requiredSkins = array(),
243244 $onByDefault = false,
244245 $category;
245246
@@ -280,6 +281,9 @@
281282 case 'rights':
282283 $gadget->requiredRights = $params;
283284 break;
 285+ case 'skins':
 286+ $gadget->requiredSkins = array_intersect( array_keys( Skin::getSkinNames() ), $params );
 287+ break;
284288 case 'default':
285289 $gadget->onByDefault = true;
286290 break;
@@ -359,7 +363,8 @@
360364 * @return Boolean
361365 */
362366 public function isAllowed( $user ) {
363 - return count( array_intersect( $this->requiredRights, $user->getRights() ) ) == count( $this->requiredRights );
 367+ return count( array_intersect( $this->requiredRights, $user->getRights() ) ) == count( $this->requiredRights )
 368+ && ( !count( $this->requiredSkins ) || in_array( $user->getOption( 'skin' ), $this->requiredSkins ) );
364369 }
365370
366371 /**
@@ -466,6 +471,14 @@
467472 }
468473
469474 /**
 475+ * Returns array of skins where this gadget works
 476+ * @return Array
 477+ */
 478+ public function getRequiredSkins() {
 479+ return $this->requiredSkins;
 480+ }
 481+
 482+ /**
470483 * Loads and returns a list of all gadgets
471484 * @return Mixed: Array of gadgets or false
472485 */
Index: trunk/extensions/Gadgets/Gadgets.i18n.php
@@ -36,6 +36,7 @@
3737 'gadgets-required-rights' => 'Requires the following {{PLURAL:$2|right|rights}}:
3838
3939 $1',
 40+ 'gadgets-required-skins' => 'Available on the {{PLURAL:$2|$1 skin|following skins: $1}}.',
4041 'gadgets-default' => 'Enabled for everyone by default.',
4142 'gadgets-export' => 'Export',
4243 'gadgets-export-title' => 'Gadget export',
@@ -72,6 +73,9 @@
7374 'gadgets-required-rights' => 'Parameters:
7475 * $1 - a list in wikitext.
7576 * $2 - the number of items in list $1 for PLURAL use.',
 77+ 'gadgets-required-skins' => 'Parameters:
 78+* $1 - a comma list.
 79+* $2 - the number of items in list $1 for PLURAL use.',
7680 'gadgets-export' => 'Used on [[Special:Gadgets]]. This is a verb, not noun.
7781 {{Identical|Export}}',
7882 'gadgets-export-download' => 'Use the verb for this message. Submit button.
Index: trunk/extensions/Gadgets/ApiQueryGadgets.php
@@ -119,6 +119,7 @@
120120 return array(
121121 'settings' => array(
122122 'rights' => $g->getRequiredRights(),
 123+ 'skins' => $g->getRequiredSkins(),
123124 'default' => $g->isOnByDefault(),
124125 'hidden' => false, // Only exists in RL2 branch
125126 'shared' => false, // Only exists in RL2 branch
@@ -136,6 +137,7 @@
137138 private function setIndexedTagNameForMetadata( &$metadata ) {
138139 static $tagNames = array(
139140 'rights' => 'right',
 141+ 'skins' => 'skin',
140142 'scripts' => 'script',
141143 'styles' => 'style',
142144 'dependencies' => 'dependency',
Index: trunk/extensions/Gadgets/SpecialGadgets.php
@@ -117,17 +117,27 @@
118118 $lnk[] = $skin->link( $t, htmlspecialchars( $t->getText() ) );
119119 }
120120 $wgOut->addHTML( $wgLang->commaList( $lnk ) );
 121+
121122 $rights = array();
122123 foreach ( $gadget->getRequiredRights() as $right ) {
123124 $rights[] = '* ' . wfMessage( "right-$right" )->plain();
124125 }
125 -
126126 if ( count( $rights ) ) {
127127 $wgOut->addHTML( '<br />' .
128128 wfMessage( 'gadgets-required-rights', implode( "\n", $rights ), count( $rights ) )->parse()
129129 );
130130 }
131131
 132+ $skins = array();
 133+ foreach ( $gadget->getRequiredSkins() as $skinid ) {
 134+ $skins[] = wfMessage( "skinname-$skinid" )->plain();
 135+ }
 136+ if ( count( $skins ) ) {
 137+ $wgOut->addHTML( '<br />' .
 138+ wfMessage( 'gadgets-required-skins', $wgLang->commaList( $skins ), count( $skins ) )->parse()
 139+ );
 140+ }
 141+
132142 if ( $gadget->isOnByDefault() ) {
133143 $wgOut->addHTML( '<br />' . wfMessage( 'gadgets-default' )->parse() );
134144 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r100602Followup r100509: Increase GADGET_CLASS_VERSIONliangent12:49, 24 October 2011
r109290Followup r100509: Don't validate skin names written in the definition page.liangent02:44, 18 January 2012

Comments

#Comment by MaxSem (talk | contribs)   02:55, 24 October 2011
  1. You didn't increment GADGET_CLASS_VERSION
  2. What's the point if Gadgets 3.0 is waiting to be merged back?
#Comment by Liangent (talk | contribs)   12:50, 24 October 2011

I had known nothing about Gadgets 3.0 (and had a look at it just now). It's just because I wanted that feature and found the bug then I followed the original reporter's instruction to make a change and committed it.

It seems it will be more difficult to add such features in Gadgets 3.0.

#Comment by Krinkle (talk | contribs)   22:55, 13 November 2011

+					$gadget->requiredSkins = array_intersect( array_keys( Skin::getSkinNames() ), $params );

Right now if a gadget is indicated to require a certain skin, and that skin is then removed from the install, it will be available as an option in all skins (because this would return an empty array, and empty array means no requirements according to isAllowed()). Whereas is should probably be hidden for all skins if it requires a skin that doesn't exist.

Do we need the array_intersect ?

#Comment by Hashar (talk | contribs)   11:24, 17 January 2012

Liagent, can you have a look at Krinkle comment from november 13th 2011? Thanks :-)

#Comment by Liangent (talk | contribs)   13:49, 17 January 2012

I want some others' input about "Do we need the array_intersect ?"

#Comment by Krinkle (talk | contribs)   17:36, 17 January 2012

Actually array_intersect isn't as much an issue as much as much as the issue is in the nature of filtering user input in a different context that it was entered by the user.

So if we've got a wiki with skin foo, bar and quux and gadget X and gadget Y:

*GadgetX[skin=foo]|GadgetX.js|GadgetX.js
*GadgetY|GadgetY.js|GadgetY.js

Then at this moment in time GadgetX is only for users with "foo" skin and GadgetY for everyone. If this gadget remains untouched and at some point skin "foo" is removed then suddently due to the above cited code (if I understand correctly) it will be evaluated as if it was the following:

*GadgetX|GadgetX.js|GadgetX.js
*GadgetY|GadgetY.js|GadgetY.js

Eventough no change took place and GadgetX is still incompatible with other skins.

So do we need to filter them at all ? Assuming the reason is validation, I think it's fine without in this case. Invalid skins simply never match anyones preference.

In ResourceLoader2/Gadgets 2.0 we will do this in a "nicer" way in the front-end by limiting the users choice in the first place, but that's another story.

#Comment by Liangent (talk | contribs)   02:47, 18 January 2012

I removed that line in r109290 and now if one write the following line:

* GadgetX[skin=]|GadgetX.js|GadgetX.js

then it can never be used because there can't be a skin whose name is an empty string.

Do you think this is expected, or should empty strings be filtered first?

Status & tagging log