r37278 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r37277‎ | r37278 | r37279 >
Date:18:50, 7 July 2008
Author:brion
Status:old
Tags:
Comment:
Revert r37263 for now:
* (bug 12211) Show some gadgets only for admins
* (bug 13742) Allow for gadgets to be turned on by default

I'm a bit leery of the 'on by default' entirely at the moment. :)

A few comments:
* The readme examples don't seem to clearly show the option format
* Why are numeric constants being used as indexes to the option array? Strings are easier to work with and debug.
* There's a lot of stuff like this which feels very ugly:
if( isset( $gadget->options[Gadget::RIGHTS] ) && !empty( $gadget->options[Gadget::RIGHTS] ) ) {
Since it's all hard-coded anyway, why not just do something nice and clear like this?
if( !empty( $gadget->rights ) ) {
* And this:
if( wfGadgetAllowed( $gadget->options ) ) {
to:
if( $gadget->isAllowed() ) {
Modified paths:
  • /trunk/extensions/Gadgets/Gadgets.i18n.php (modified) (history)
  • /trunk/extensions/Gadgets/Gadgets.php (modified) (history)
  • /trunk/extensions/Gadgets/README (modified) (history)
  • /trunk/extensions/Gadgets/SpecialGadgets.php (modified) (history)

Diff [purge]

Index: trunk/extensions/Gadgets/Gadgets.i18n.php
——@@ -32,8 +32,6 @@
3333 'gadgets-pagetext' => "Below is a list of special gadgets users can enable on their preferences page, as defined by [[MediaWiki:Gadgets-definition]].
3434 This overview provides easy access to the system message pages that define each gadget's description and code.",
3535 'gadgets-uses' => 'Uses',
36 - 'gadgets-rights' => 'Requires the following rights',
37 - 'gadgets-default' => 'Enabled by default',
3836 );
3937
4038 /** Amharic (አማርኛ)
——@@ -899,6 +897,7 @@
900898 * @author Illusion
901899 * @author Siebrand
902900 * @author Александр Сигачёв
 901+ * @author Ahonc
903902 */
904903 $messages['ru'] = array(
905904 'gadgets-desc' => 'Позволяет участникам выбирать в [[Special:Preferences|настройках]] CSS- и JavaScript-гаджеты, которые они хотят подключить',
——@@ -1164,4 +1163,3 @@
11651164 'gadgets-uses' => '使用',
11661165 );
11671166
1168 -
Index: trunk/extensions/Gadgets/Gadgets.php
——@@ -38,23 +38,6 @@
3939 $wgAutoloadClasses['SpecialGadgets'] = $dir . 'SpecialGadgets.php';
4040 $wgSpecialPages['Gadgets'] = 'SpecialGadgets';
4141
42 -class Gadget {
43 - const CACHE_VERSION = 1;
44 - /**
45 - * This gadget option contains rights the user must have to use this gadget
46 - */
47 - const RIGHTS = 666;
48 -
49 - /**
50 - * This gadget should be enabled by default
51 - */
52 - const ON_BY_DEFAULT = 667;
53 -
54 - var $name = '';
55 - var $modules = array();
56 - var $options = array();
57 -}
58 -
5942 function wfGadgetsArticleSaveComplete( &$article, &$wgUser, &$text ) {
6043 //update cache if MediaWiki:Gadgets-definition was edited
6144 $title = $article->mTitle;
——@@ -94,10 +77,7 @@
9578 if ( $forceNewText === NULL ) {
9679 //cached?
9780 $gadgets = $wgMemc->get( $key );
98 - if ( is_array( $gadgets ) && isset( $gadgets['version'] ) && $gadgets['version'] == Gadget::CACHE_VERSION ) {
99 - unset( $gadgets['version'] );
100 - return $gadgets;
101 - }
 81+ if ( is_array($gadgets) ) return $gadgets;
10282
10383 $g = wfMsgForContentNoTrans( "gadgets-definition" );
10484 if ( wfEmptyMsg( "gadgets-definition", $g ) ) {
——@@ -117,48 +97,29 @@
11898 foreach ( $g as $line ) {
11999 if ( preg_match( '/^==+ *([^*:\s|]+?)\s*==+\s*$/', $line, $m ) ) {
120100 $section = $m[1];
121 - } else if ( preg_match( '/^\*+ *([a-zA-Z](?:[-_:.\w\d ]*[a-zA-Z0-9])?)\s*((\|[^|[]*)+)\s*(|\[(.*)\])\s*$/', $line, $m ) ) {
122 - $gadget = new Gadget();
123 -
 101+ }
 102+ else if ( preg_match( '/^\*+ *([a-zA-Z](?:[-_:.\w\d ]*[a-zA-Z0-9])?)\s*((\|[^|]*)+)\s*$/', $line, $m ) ) {
124103 //NOTE: the gadget name is used as part of the name of a form field,
125104 // and must follow the rules defined in http://www.w3.org/TR/html4/types.html#type-cdata
126105 // Also, title-normalization applies.
127 - $gadget->name = str_replace(' ', '_', $m[1] );
 106+ $name = str_replace(' ', '_', $m[1] );
128107
129 - $gadget->modules = preg_split( '/\s*\|\s*/', $m[2], -1, PREG_SPLIT_NO_EMPTY );
 108+ $code = preg_split( '/\s*\|\s*/', $m[2], -1, PREG_SPLIT_NO_EMPTY );
130109
131 - if( !empty( $m[5] ) && $gadget->modules )
132 - $gadget->options = wfProcessGadgetOptions( $m[5] );
133 -
134 - if ( $gadget->modules ) {
135 - $gadgets[$section][$gadget->name] = $gadget;
 110+ if ( $code ) {
 111+ $gadgets[$section][$name] = $code;
136112 }
137113 }
138114 }
139115
140116 //cache for a while. gets purged automatically when MediaWiki:Gadgets-definition is edited
141 - $gadgets['version'] = Gadget::CACHE_VERSION;
142 - $wgMemc->set( $key, $gadgets, 60*60*24 );
 117+ $wgMemc->set( $key, $gadgets, 60*60*24 );
143118 $source = $forceNewText !== NULL ? 'input text' : 'MediaWiki:Gadgets-definition';
144119 wfDebug( __METHOD__ . ": $source parsed, cache entry $key updated\n");
145120
146 - unset( $gadgets['version'] );
147121 return $gadgets;
148122 }
149123
150 -function wfProcessGadgetOptions( $options ) {
151 - $out = array();
152 - foreach( preg_split( '/\s*\|\s*/', $options, -1, PREG_SPLIT_NO_EMPTY ) as $option ) {
153 - if( preg_match( '/^rights\s*:\s*([\w\s,]+)$/', $option, $m ) ) {
154 - $out[Gadget::RIGHTS] = preg_split( '/\s*,\s*/', $m[1], -1, PREG_SPLIT_NO_EMPTY );
155 - } elseif( preg_match( '/^default$/', $option ) ) {
156 - $out[Gadget::ON_BY_DEFAULT] = true;
157 - }
158 - }
159 -
160 - return $out;
161 -}
162 -
163124 function wfGadgetsInitPreferencesForm( &$prefs, &$request ) {
164125 $gadgets = wfLoadGadgets();
165126 if ( !$gadgets ) return true;
——@@ -171,18 +132,6 @@
172133 return true;
173134 }
174135
175 -function wfGadgetAllowed( $options ) {
176 - global $wgUser;
177 -
178 - if( isset( $options[Gadget::RIGHTS] ) )
179 - foreach( $options[Gadget::RIGHTS] as $right ) {
180 - if( !in_array( $right, $wgUser->getRights() ) )
181 - return FALSE;
182 - }
183 -
184 - return TRUE;
185 -}
186 -
187136 function wfGadgetsResetPreferences( &$prefs, &$user ) {
188137 $gadgets = wfLoadGadgets();
189138 if ( !$gadgets ) return true;
——@@ -196,8 +145,6 @@
197146 }
198147
199148 function wfGadgetsRenderPreferencesForm( &$prefs, &$out ) {
200 - global $wgUser;
201 -
202149 $gadgets = wfLoadGadgetsStructured();
203150 if ( !$gadgets ) return true;
204151
——@@ -210,39 +157,22 @@
211158 $msgOpt = array( 'parseinline' );
212159
213160 foreach ( $gadgets as $section => $entries ) {
214 - $first = true;
 161+ if ( $section !== false && $section !== '' ) {
 162+ $ttext = wfMsgExt( "gadget-section-$section", $msgOpt );
 163+ $out->addHtml( "\n<h2 id=\"".htmlspecialchars("gadget-section-$section")."\">" . $ttext . "</h2>\n" );
 164+ }
215165
216 - foreach ( $entries as $gname => $gadget ) {
 166+ foreach ( $entries as $gname => $code ) {
217167 $tname = "gadget-$gname";
218168 $ttext = wfMsgExt( $tname, $msgOpt );
219 -
220 - if( ( ( $prefs->mToggles[$tname] != '' ) && ($prefs->mToggles[$tname] == 1) )
221 - || ( ( $prefs->mToggles[$tname] === '' ) && isset( $gadget->options[Gadget::ON_BY_DEFAULT] ) ) )
222 - $checked = ' checked="checked"';
223 - else
224 - $checked = '';
225 -
 169+ $checked = @$prefs->mToggles[$tname] == 1 ? ' checked="checked"' : '';
226170 $disabled = '';
227171
228 - if( wfGadgetAllowed( $gadget->options ) ) {
229 - // draw section heading
230 - if ($first && $section !== false && $section !== '' ) {
231 - $stext = wfMsgExt( "gadget-section-$section", $msgOpt );
232 - $out->addHtml( "\n<h2 id=\"".htmlspecialchars("gadget-section-$section")."\">" . $stext . "</h2>\n" );
233 - $first = false;
234 - }
235 -
236 - if( isset( $gadget->options[Gadget::ON_BY_DEFAULT] ) )
237 - $extra = ' (' . wfMsg( 'gadgets-default' ) . ')';
238 - else
239 - $extra = '';
240 -
241 - # NOTE: No label for checkmarks as this causes the checks to toggle
242 - # when clicking a link in the describing text.
243 - $out->addHtml( "<div class='toggle'><input type='checkbox' value='1' " .
244 - "id=\"$tname\" name=\"wpOp$tname\"$checked$disabled />" .
245 - " <span class='toggletext'>$ttext</span>$extra</div>\n" );
246 - }
 172+ # NOTE: No label for checkmarks as this causes the checks to toggle
 173+ # when clicking a link in the describing text.
 174+ $out->addHtml( "<div class='toggle'><input type='checkbox' value='1' " .
 175+ "id=\"$tname\" name=\"wpOp$tname\"$checked$disabled />" .
 176+ " <span class='toggletext'>$ttext</span></div>\n" );
247177 }
248178 }
249179
——@@ -266,13 +196,10 @@
267197
268198 $done = array();
269199
270 - foreach ( $gadgets as $gname => $gadget ) {
271 - if( wfGadgetAllowed( $gadget->options ) ) {
272 - $tname = "gadget-$gname";
273 - $default = isset( $gadget->options[Gadget::ON_BY_DEFAULT] );
274 - if ( $wgUser->getOption( $tname, $default ) ) {
275 - wfApplyGadgetCode( $gadget->modules, $out, $done );
276 - }
 200+ foreach ( $gadgets as $gname => $code ) {
 201+ $tname = "gadget-$gname";
 202+ if ( $wgUser->getOption( $tname ) ) {
 203+ wfApplyGadgetCode( $code, $out, $done );
277204 }
278205 }
279206
Index: trunk/extensions/Gadgets/SpecialGadgets.php
——@@ -59,33 +59,24 @@
6060 $wgOut->addHTML( "\n<h2>$ttext &nbsp; &nbsp; [$lnk]</h2>\n" );
6161 }
6262
63 - foreach ( $entries as $gname => $gadget ) {
 63+ foreach ( $entries as $gname => $code ) {
6464 $t = Title::makeTitleSafe( NS_MEDIAWIKI, "Gadget-$gname" );
6565 if ( !$t ) continue;
6666
6767 $lnk = $skin->makeLinkObj( $t, wfMsgHTML("edit"), 'action=edit' );
6868 $ttext = wfMsgExt( "gadget-$gname", $msgOpt );
69 -
 69+
7070 if( !$listOpen ) {
7171 $listOpen = true;
7272 $wgOut->addHTML( '<ul>' );
7373 }
7474 $wgOut->addHTML( "<li>" );
7575 $wgOut->addHTML( "$ttext &nbsp; &nbsp; [$lnk]<br />" );
76 -
77 - if( isset( $gadget->options[Gadget::RIGHTS] ) && !empty( $gadget->options[Gadget::RIGHTS] ) ) {
78 - $wgOut->addHTML( wfMsgHTML( 'gadgets-rights' ) . ": " );
79 - $wgOut->addHTML( htmlspecialchars( implode( ', ', $gadget->options[Gadget::RIGHTS] ) ) . '<br />' );
80 - }
81 -
82 - if( isset( $gadget->options[Gadget::ON_BY_DEFAULT] ) )
83 - $wgOut->addHTML( wfMsgHTML( "gadgets-default" ) . "<br />" );
84 -
8576
8677 $wgOut->addHTML( wfMsgHTML("gadgets-uses") . ": " );
8778
8879 $first = true;
89 - foreach ( $gadget->modules as $codePage ) {
 80+ foreach ( $code as $codePage ) {
9081 $t = Title::makeTitleSafe( NS_MEDIAWIKI, "Gadget-$codePage" );
9182 if ( !$t ) continue;
9283
Index: trunk/extensions/Gadgets/README
——@@ -33,7 +33,7 @@
3434 Each line in MediaWiki:Gadgets-definition that start with one or more "*"
3535 (asterisk) characters defines a gadget; it must have the following form:
3636
37 - * mygadget|mygadget.js|mygadget.css[option1|option2]
 37+ * mygadget|mygadget.js|mygadget.css
3838
3939 That is, each line consists of fields separated by a "|" (pipe) character.
4040 The first field ("mygadget" in the example) is the gadgets internal name,
——@@ -65,27 +65,7 @@
6666 This would define a new section, with the title defined on the page
6767 MediaWiki:Gadget-section-editing-gadgets
6868
69 -== Options ==
70 -Options are optionally appended to the end of the gadget declaration.
71 -They are enclosed in square brackets ("[...]") and are separated by pipe
72 -character ("|").
73 -
74 -Currently, there are two options: rights required to use the gadget, and
75 -whether the gadget should be enaled for everyone by default.
7669
77 -The rights option looks like "[rights:delete,userrights]". Note that it
78 -requires specific rights, not groups the user belongs to. This option is
79 -case-sensitive, but ignores whitespace, i.e. "[ rights: foo , bar ]" is a
80 -valid declaration, but not "[Rights:Foo]".
81 -
82 -The "default" option, if present, makes this gadget enabled for every
83 -eligible user who has not specifically opted out of it by unchecking it in
84 -their preferences. The option looks like that: "[default]".
85 -
86 -Options may be combined in any number or order, i.e. "[rights:foo, bar]",
87 -"[default]", "[rights:boz|default]" and "[default|rights:quux]" are all valid
88 -declarations.
89 -
9070 == Caveats ==
9171
9272 * Requires MediaWiki 1.11alpha, revision 24477 or later.

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r37263* (bug 12211) Show some gadgets only for admins...vasilievvv16:54, 7 July 2008

Status & tagging log