r51151 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r51150‎ | r51151 | r51152 >
Date:11:53, 29 May 2009
Author:jojo
Status:ok (Comments)
Tags:
Comment:
make the rights needed for saving collections configurable via $wgCollectionSaveAs{User|Community}PageRight
Modified paths:
  • /trunk/extensions/Collection/Collection.php (modified) (history)
  • /trunk/extensions/Collection/Collection.templates.php (modified) (history)
  • /trunk/extensions/Collection/README.txt (modified) (history)

Diff [purge]

Index: trunk/extensions/Collection/Collection.php
——@@ -94,6 +94,9 @@
9595
9696 $wgCollectionPortletForLoggedInUsersOnly = false;
9797
 98+$wgCollectionSaveAsUserPageRight = null;
 99+$wgCollectionSaveAsCommunityPageRight = 'autoconfirmed';
 100+
98101 $wgCollectionNavPopups = false;
99102
100103 # ==============================================================================
Index: trunk/extensions/Collection/README.txt
——@@ -144,6 +144,17 @@
145145
146146 Default is ``NS_PROJECT``.
147147
 148+ *$wgCollectionSaveAsUserPageRight, $wgCollectionSaveAsCommunityPageRight (string)*
 149+
 150+ The MediaWiki right a user has to have in order to be allowed to save
 151+ collection pages under the user namespace or the
 152+ $wgCommunityCollectionNamespace respectively.
 153+ If one of the variables is set to '' or null, no permission check is done for
 154+ that particular namespace.
 155+
 156+ Defaults are null for $wgCollectionSaveAsUserPageRight and 'autoconfirmed' for
 157+ $wgCollectionSaveAsCommunityPageRight.
 158+
148159 *$wgCollectionMaxArticles (integer)*
149160 Maximum number of articles allowed in a collection.
150161
Index: trunk/extensions/Collection/Collection.templates.php
——@@ -60,26 +60,26 @@
6161 <?php
6262 $partnerData = $this->data['podpartners']['pediapress'];
6363 $this->msgWiki('coll-book_text');
64 - ?>
65 - <div>
66 - <div style="float:right">
67 - <form action="<?php echo htmlspecialchars(SkinTemplate::makeSpecialUrlSubpage('Book', 'post_zip/')) ?>" method="post">
68 - <input type="hidden" name="partner" value="pediapress" />
69 - <input type="submit" value="<?php echo wfMsgHtml('coll-order_from_pp', htmlspecialchars($partnerData['name'])) ?>" class="order" <?php if (count($this->data['collection']['items']) == 0) { ?> disabled="disabled"<?php } ?> />
70 - </form>
71 - </div>
 64+ ?>
 65+ <div>
 66+ <div style="float:right">
 67+ <form action="<?php echo htmlspecialchars(SkinTemplate::makeSpecialUrlSubpage('Book', 'post_zip/')) ?>" method="post">
 68+ <input type="hidden" name="partner" value="pediapress" />
 69+ <input type="submit" value="<?php echo wfMsgHtml('coll-order_from_pp', htmlspecialchars($partnerData['name'])) ?>" class="order" <?php if (count($this->data['collection']['items']) == 0) { ?> disabled="disabled"<?php } ?> />
 70+ </form>
 71+ </div>
7272 <?php
7373 $t = Title::newFromText(wfMsgForContent('coll-order_info_article'));
7474 $a = new Article($t);
7575 if ( $a->exists() ) { ?>
76 - <div id="coll-more_info" style="display:none">
77 - <a href="javascript:void(0)" onclick="coll_toggle_order_info(true);"><?php $this->msgWiki('coll-more_info') ?></a>
78 - </div>
79 - <div id="coll-hide_info" style="display:none">
80 - <a href="javascript:void(0)" onclick="coll_toggle_order_info(false);"><?php $this->msgWiki('coll-hide_info') ?></a>
81 - </div>
 76+ <div id="coll-more_info" style="display:none">
 77+ <a href="javascript:void(0)" onclick="coll_toggle_order_info(true);"><?php $this->msgWiki('coll-more_info') ?></a>
 78+ </div>
 79+ <div id="coll-hide_info" style="display:none">
 80+ <a href="javascript:void(0)" onclick="coll_toggle_order_info(false);"><?php $this->msgWiki('coll-hide_info') ?></a>
 81+ </div>
8282 <?php } else { ?>
83 - <a href="<?php echo htmlspecialchars($partnerData['url']) ?>" target="_blank"><?php echo wfMsgHtml('coll-about_pp', htmlspecialchars($partnerData['name'])) ?></a>
 83+ <a href="<?php echo htmlspecialchars($partnerData['url']) ?>" target="_blank"><?php echo wfMsgHtml('coll-about_pp', htmlspecialchars($partnerData['name'])) ?></a>
8484 <?php } ?>
8585 </div>
8686 <?php
——@@ -94,55 +94,64 @@
9595 ?>
9696 </div>
9797 <?php } ?>
98 - </div>
 98+ </div>
9999
100100 <div style="margin-bottom: 10px; padding: 10px; border: 1px solid #aaa; background-color: #f9f9f9;">
101101 <h2><span class="mw-headline"><?php $this->msg('coll-download_title') ?></span></h2>
102 - <?php if (count($this->data['formats']) == 1) {
103 - $writer = array_rand($this->data['formats']);
104 - echo $GLOBALS['wgParser']->parse(
105 - wfMsgNoTrans('coll-download_as_text', $this->data['formats'][$writer]),
106 - $GLOBALS['wgTitle'],
107 - $GLOBALS['wgOut']->parserOptions(),
108 - true
109 - )->getText();
110 - $buttonLabel = wfMsgHtml('coll-download_as', htmlspecialchars($this->data['formats'][$writer]));
111 - } else {
112 - $this->msgWiki('coll-download_text');
113 - $buttonLabel = wfMsgHtml('coll-download');
114 - } ?>
 102+ <?php if (count($this->data['formats']) == 1) {
 103+ $writer = array_rand($this->data['formats']);
 104+ echo $GLOBALS['wgParser']->parse(
 105+ wfMsgNoTrans('coll-download_as_text', $this->data['formats'][$writer]),
 106+ $GLOBALS['wgTitle'],
 107+ $GLOBALS['wgOut']->parserOptions(),
 108+ true
 109+ )->getText();
 110+ $buttonLabel = wfMsgHtml('coll-download_as', htmlspecialchars($this->data['formats'][$writer]));
 111+ } else {
 112+ $this->msgWiki('coll-download_text');
 113+ $buttonLabel = wfMsgHtml('coll-download');
 114+ } ?>
115115 <form id="downloadForm" action="<?php echo htmlspecialchars(SkinTemplate::makeSpecialUrlSubpage('Book', 'render/')) ?>" method="post">
116 - <table style="width:100%; background-color: transparent;"><tr><td><tbody><tr><td>
 116+ <table style="width:100%; background-color: transparent;"><tr><td><tbody><tr><td>
117117 <?php if (count($this->data['formats']) == 1) { ?>
118118 <input type="hidden" name="writer" value="<?php echo htmlspecialchars($writer) ?>" />
119119 <?php } else { ?>
120 - <label for="formatSelect"><?php $this->msg('coll-format_label') ?></label>
121 - <select id="formatSelect" name="writer">
122 - <?php foreach ($this->data['formats'] as $writer => $name) { ?>
123 - <option value="<?php echo htmlspecialchars($writer) ?>"><?php echo htmlspecialchars($name) ?></option>
124 - <?php } ?>
125 - </select>
 120+ <label for="formatSelect"><?php $this->msg('coll-format_label') ?></label>
 121+ <select id="formatSelect" name="writer">
 122+ <?php foreach ($this->data['formats'] as $writer => $name) { ?>
 123+ <option value="<?php echo htmlspecialchars($writer) ?>"><?php echo htmlspecialchars($name) ?></option>
 124+ <?php } ?>
 125+ </select>
126126 <?php } ?>
127 - </td><td style="text-align:right; vertical-align:bottom;">
128 - <input id="downloadButton" type="submit" value="<?php echo $buttonLabel ?>"<?php if (count($this->data['collection']['items']) == 0) { ?> disabled="disabled"<?php } ?> />
129 - </td></tr></tbody></table>
 127+ </td><td style="text-align:right; vertical-align:bottom;">
 128+ <input id="downloadButton" type="submit" value="<?php echo $buttonLabel ?>"<?php if (count($this->data['collection']['items']) == 0) { ?> disabled="disabled"<?php } ?> />
 129+ </td></tr></tbody></table>
130130 </form>
131131 </div>
132132
133 - <?php if ($GLOBALS['wgEnableWriteAPI']) { ?>
 133+ <?php
 134+ if ($GLOBALS['wgUser']->isLoggedIn()) {
 135+ $showLoginInfo = false;
 136+ $canSaveUserPage = (empty($GLOBALS['wgCollectionSaveAsUserPageRight']) || $GLOBALS['wgUser']->isAllowed($GLOBALS['wgCollectionSaveAsUserPageRight']));
 137+ $canSaveCommunityPage = (empty($GLOBALS['wgCollectionSaveAsCommunityPageRight']) || $GLOBALS['wgUser']->isAllowed($GLOBALS['wgCollectionSaveAsCommunityPageRight']));
 138+ } else {
 139+ $showLoginInfo = (empty($GLOBALS['wgCollectionSaveAsCommunityPageRight']) || empty($GLOBALS['wgCollectionSaveAsUserPageRight']));
 140+ }
 141+ if ($GLOBALS['wgEnableWriteAPI'] && ($showLoginInfo || $canSaveUserPage || $canSaveCommunityPage)) {
 142+ ?>
134143 <div id="coll-savebox" style="margin-bottom: 10px; padding: 10px; border: 1px solid #aaa; background-color: #f9f9f9;">
135144 <h2><span class="mw-headline"><?php $this->msg('coll-save_collection_title') ?></span></h2>
136 - <?php if ($GLOBALS['wgUser']->isLoggedIn()) { ?>
137 - <?php $this->msgWiki('coll-save_collection_text') ?>
138145 <?php
139 - $bookname = wfMsgForContent('coll-collections');
 146+ if (!$showLoginInfo) {
 147+ $this->msgWiki('coll-save_collection_text');
 148+ $bookname = wfMsgForContent('coll-collections');
 149+ $communityCollNS = $GLOBALS['wgCommunityCollectionNamespace'];
140150 ?>
141151 <form id="saveForm" action="<?php echo htmlspecialchars(SkinTemplate::makeSpecialUrlSubpage('Book', 'save_collection/')) ?>" method="post">
142 - <table style="width:100%; background-color: transparent;"><tr><td>
143 - <?php
144 - if (!$GLOBALS['wgUser']->isNewbie()) {
145 - $communityCollNS = $GLOBALS['wgCommunityCollectionNamespace'];
146 - ?>
 152+ <table style="width:100%; background-color: transparent;"><tbody>
 153+ <?php if ($canSaveUserPage) { ?>
 154+ <tr><td>
 155+ <?php if ($canSaveCommunityPage) { ?>
147156 <input id="personalCollType" type="radio" name="colltype" value="personal" checked="checked" />
148157 <?php } else { ?>
149158 <input type="hidden" name="colltype" value="personal" />
——@@ -152,22 +161,23 @@
153162 <td style="text-align:right;">
154163 <input id="personalCollTitle" type="text" name="pcollname" />
155164 </td></tr>
156 - <?php
157 - if (!$GLOBALS['wgUser']->isNewbie()) {
158 - $communityCollNS = $GLOBALS['wgCommunityCollectionNamespace'];
159 - ?>
 165+ <?php } // if ($canSaveUserPage) ?>
 166+ <?php if ($canSaveCommunityPage) { ?>
160167 <tr><td>
 168+ <?php if ($canSaveUserPage) { ?>
161169 <input id="communityCollType" type="radio" name="colltype" value="community" />
 170+ <?php } else { ?>
 171+ <input type="hidden" name="colltype" value="community" />
 172+ <?php } ?>
162173 <label for="communityCollTitle"><a href="<?php echo htmlspecialchars(SkinTemplate::makeSpecialUrl('Prefixindex', 'from=' . $bookname . '/&namespace=' . $communityCollNS)) ?>"><?php echo htmlspecialchars(Title::makeTitle($communityCollNS, $bookname)->getPrefixedText() . '/') ?></a></label>
163174 </td>
164175 <td style="text-align:right;">
165176 <input id="communityCollTitle" type="text" name="ccollname" disabled="disabled" />
166177 </td></tr>
167 - <?php } // autoconfirmed ?>
 178+ <?php } // if ($canSaveCommunityPage) ?>
168179 <tr><td>&nbsp;</td><td style="text-align:right;">
169180 <input id="saveButton" type="submit" value="<?php $this->msg('coll-save_collection') ?>"<?php if (count($this->data['collection']['items']) == 0) { ?> disabled="disabled"<?php } ?> />
170 - </tr>
171 - </table>
 181+ </tr></tbody></table>
172182 </form>
173183
174184 <?php } else {
——@@ -176,7 +186,7 @@
177187 $this->msgWiki('coll-save_category');
178188 ?>
179189 </div>
180 - <?php } ?>
 190+ <?php } ?>
181191
182192 </div>
183193
——@@ -370,12 +380,12 @@
371381 $t = Title::newFromText($title_string);
372382 $a = new Article($t);
373383 if ( $a->exists() ) {
374 - echo $GLOBALS['wgParser']->parse(
375 - wfMsgNoTrans('coll-blacklisted-templates', $title_string),
376 - $GLOBALS['wgTitle'],
377 - $GLOBALS['wgOut']->parserOptions(),
378 - true
379 - )->getText();
 384+ echo $GLOBALS['wgParser']->parse(
 385+ wfMsgNoTrans('coll-blacklisted-templates', $title_string),
 386+ $GLOBALS['wgTitle'],
 387+ $GLOBALS['wgOut']->parserOptions(),
 388+ true
 389+ )->getText();
380390 }
381391 if ($this->data['return_to']) {
382392 // We are doing this the hard way (i.e. via the HTML detour), to prevent

Comments

#Comment by Happy-melon (talk | contribs)   10:39, 12 June 2009

This doesn't implement any permissions checks for actually saving the book, only for displaying the form. I can still save books without the permission by spoofing the form.

Wouldn't it be nicer just to implement these in the 'normal' way: have hardcoded permissions 'collectionsaveasuserpage' and 'collectionsaveascommunitypage', or some such, and assign them using $wgGroupPermissions and check them using userCan(...) in the usual way? It seems to me that making the *internal name* of the permission a variable is counterintuitive; nothing is gained in terms of ease of use because the permissions, once named, then have to also be assigned in the usual way.

#Comment by Jbeigel (talk | contribs)   10:41, 12 June 2009
  • Cough* You're right of course. I've reopened bug18902.
#Comment by Jbeigel (talk | contribs)   10:44, 12 June 2009

The fact that these rights are configurable resulted from discussions with WMF staff and my colleagues: a wiki sysop might want to use e.g. special rights (like autoconfirmed) without having to configure the group permissions etc.

#Comment by Happy-melon (talk | contribs)   11:01, 12 June 2009

Hmm, I guess; I hadn't thought about 'bolting it on' to an existing permission. It still doesn't seem very intuitive: the normal process is that we assign an action to a permission label, then assign the permission to a group, then assign the group to a user. Eg "edit a page" --> 'edit' --> 'user' --> "All registered users". Normally the first is hardcoded. I'm not sure that we really need the additional flexibility of having three layers of abstraction instead of two.

#Comment by Platonides (talk | contribs)   12:25, 12 June 2009

Then you would assign the collectionsaveascommunitypage right to the autoconfirmed group. $wgGroupPermissions is the way to go.

#Comment by Jbeigel (talk | contribs)   09:43, 15 June 2009

OK. Implemented in r51870. Thank you guys for your feedback!

Status & tagging log