r68999 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r68998‎ | r68999 | r69000 >
Date:14:38, 4 July 2010
Author:ialex
Status:resolved (Comments)
Tags:
Comment:
* Modified Special:Ipblocklist to subclass SpecialPage instead of using wfSpecialIblocklist()
* Changed some calls from Xml:: to Html::
* Coding style
Modified paths:
  • /trunk/phase3/includes/SpecialPage.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialIpblocklist.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/specials/SpecialIpblocklist.php
——@@ -18,116 +18,111 @@
1919 */
2020
2121 /**
22 - * @file
 22+ * Implements Special:ipblocklist
2323 * @ingroup SpecialPage
2424 */
 25+class IPUnblockForm extends SpecialPage {
 26+ var $ip, $reason, $id;
 27+ var $hideuserblocks, $hidetempblocks, $hideaddressblocks;
2528
26 -/**
27 - * @param $ip part of title: Special:Ipblocklist/<ip>.
28 - * @todo document
29 - */
30 -function wfSpecialIpblocklist( $ip = '' ) {
31 - global $wgUser, $wgOut, $wgRequest;
32 - $ip = $wgRequest->getVal( 'ip', $ip );
33 - $ip = trim( $wgRequest->getVal( 'wpUnblockAddress', $ip ) );
34 - $id = $wgRequest->getVal( 'id' );
35 - $reason = $wgRequest->getText( 'wpUnblockReason' );
36 - $action = $wgRequest->getText( 'action' );
37 - $successip = $wgRequest->getVal( 'successip' );
 29+ function __construct() {
 30+ parent::__construct( 'Ipblocklist' );
 31+ }
3832
39 - $ipu = new IPUnblockForm( $ip, $id, $reason );
 33+ /**
 34+ * Main execution point
 35+ *
 36+ * @param $ip part of title: Special:Ipblocklist/<ip>.
 37+ */
 38+ function execute( $ip ) {
 39+ global $wgUser, $wgOut, $wgRequest;
4040
41 - if( $action == 'unblock' || $action == 'submit' && $wgRequest->wasPosted() ) {
42 - # Check permissions
43 - if( !$wgUser->isAllowed( 'block' ) ) {
44 - $wgOut->permissionRequired( 'block' );
45 - return;
46 - }
47 - # Check for database lock
48 - if( wfReadOnly() ) {
49 - $wgOut->readOnlyPage();
50 - return;
51 - }
 41+ $this->setHeaders();
 42+ $this->outputHeader();
 43+
 44+ $ip = $wgRequest->getVal( 'ip', $ip );
 45+ $this->ip = trim( $wgRequest->getVal( 'wpUnblockAddress', $ip ) );
 46+ $this->id = $wgRequest->getVal( 'id' );
 47+ $this->reason = $wgRequest->getText( 'wpUnblockReason' );
 48+ $this->hideuserblocks = $wgRequest->getBool( 'hideuserblocks' );
 49+ $this->hidetempblocks = $wgRequest->getBool( 'hidetempblocks' );
 50+ $this->hideaddressblocks = $wgRequest->getBool( 'hideaddressblocks' );
 51+
 52+ $action = $wgRequest->getText( 'action' );
 53+ $successip = $wgRequest->getVal( 'successip' );
 54+
 55+ if( $action == 'unblock' || $action == 'submit' && $wgRequest->wasPosted() ) {
 56+ # Check permissions
 57+ if( !$wgUser->isAllowed( 'block' ) ) {
 58+ $wgOut->permissionRequired( 'block' );
 59+ return;
 60+ }
 61+ # Check for database lock
 62+ if( wfReadOnly() ) {
 63+ $wgOut->readOnlyPage();
 64+ return;
 65+ }
5266
53 - # bug 15810: blocked admins should have limited access here
54 - if ( $wgUser->isBlocked() ) {
55 - if ( $id ) {
56 - # This doesn't pick up on autoblocks, but admins
57 - # should have the ipblock-exempt permission anyway
58 - $block = Block::newFromID( $id );
59 - $user = User::newFromName( $block->mAddress );
60 - } else {
61 - $user = User::newFromName( $ip );
 67+ # bug 15810: blocked admins should have limited access here
 68+ if ( $wgUser->isBlocked() ) {
 69+ if ( $id ) {
 70+ # This doesn't pick up on autoblocks, but admins
 71+ # should have the ipblock-exempt permission anyway
 72+ $block = Block::newFromID( $id );
 73+ $user = User::newFromName( $block->mAddress );
 74+ } else {
 75+ $user = User::newFromName( $ip );
 76+ }
 77+ $status = self::checkUnblockSelf( $user );
 78+ if ( $status !== true ) {
 79+ throw new ErrorPageError( 'badaccess', $status );
 80+ }
6281 }
63 - $status = IPBlockForm::checkUnblockSelf( $user );
64 - if ( $status !== true ) {
65 - throw new ErrorPageError( 'badaccess', $status );
 82+
 83+ if( $action == 'unblock' ){
 84+ # Show unblock form
 85+ $this->showForm( '' );
 86+ } elseif( $action == 'submit'
 87+ && $wgRequest->wasPosted()
 88+ && $wgUser->matchEditToken( $wgRequest->getVal( 'wpEditToken' ) ) )
 89+ {
 90+ # Remove blocks and redirect user to success page
 91+ $this->doSubmit();
6692 }
67 - }
68 -
69 - if( $action == 'unblock' ){
70 - # Show unblock form
71 - $ipu->showForm( '' );
72 - } elseif( $action == 'submit'
73 - && $wgRequest->wasPosted()
74 - && $wgUser->matchEditToken( $wgRequest->getVal( 'wpEditToken' ) ) )
75 - {
76 - # Remove blocks and redirect user to success page
77 - $ipu->doSubmit();
78 - }
79 -
80 - } elseif( $action == 'success' ) {
81 - # Inform the user of a successful unblock
82 - # (No need to check permissions or locks here,
83 - # if something was done, then it's too late!)
84 - if ( substr( $successip, 0, 1) == '#' ) {
85 - // A block ID was unblocked
86 - $ipu->showList( $wgOut->parse( wfMsg( 'unblocked-id', $successip ) ) );
 93+
 94+ } elseif( $action == 'success' ) {
 95+ # Inform the user of a successful unblock
 96+ # (No need to check permissions or locks here,
 97+ # if something was done, then it's too late!)
 98+ if ( substr( $successip, 0, 1) == '#' ) {
 99+ // A block ID was unblocked
 100+ $this->showList( $wgOut->parse( wfMsg( 'unblocked-id', $successip ) ) );
 101+ } else {
 102+ // A username/IP was unblocked
 103+ $this->showList( $wgOut->parse( wfMsg( 'unblocked', $successip ) ) );
 104+ }
87105 } else {
88 - // A username/IP was unblocked
89 - $ipu->showList( $wgOut->parse( wfMsg( 'unblocked', $successip ) ) );
 106+ # Just show the block list
 107+ $this->showList( '' );
90108 }
91 - } else {
92 - # Just show the block list
93 - $ipu->showList( '' );
94109 }
95110
96 -}
97 -
98 -/**
99 - * implements Special:ipblocklist GUI
100 - * @ingroup SpecialPage
101 - */
102 -class IPUnblockForm {
103 - var $ip, $reason, $id;
104 -
105 - function IPUnblockForm( $ip, $id, $reason ) {
106 - global $wgRequest;
107 - $this->ip = strtr( $ip, '_', ' ' );
108 - $this->id = $id;
109 - $this->reason = $reason;
110 - $this->hideuserblocks = $wgRequest->getBool( 'hideuserblocks' );
111 - $this->hidetempblocks = $wgRequest->getBool( 'hidetempblocks' );
112 - $this->hideaddressblocks = $wgRequest->getBool( 'hideaddressblocks' );
113 - }
114 -
115111 /**
116112 * Generates the unblock form
 113+ *
117114 * @param $err string: error message
118115 * @return $out string: HTML form
119116 */
120117 function showForm( $err ) {
121118 global $wgOut, $wgUser, $wgSysopUserBans;
122119
123 - $wgOut->setPagetitle( wfMsg( 'unblockip' ) );
124120 $wgOut->addWikiMsg( 'unblockiptext' );
125121
126 - $titleObj = SpecialPage::getTitleFor( "Ipblocklist" );
127 - $action = $titleObj->getLocalURL( "action=submit" );
 122+ $action = $this->getTitle()->getLocalURL( 'action=submit' );
128123
129 - if ( $err != "" ) {
130 - $wgOut->setSubtitle( wfMsg( "formerror" ) );
131 - $wgOut->addWikiText( Xml::tags( 'span', array( 'class' => 'error' ), $err ) . "\n" );
 124+ if ( $err != '' ) {
 125+ $wgOut->setSubtitle( wfMsg( 'formerror' ) );
 126+ $wgOut->addWikiText( Html::rawElement( 'span', array( 'class' => 'error' ), $err ) . "\n" );
132127 }
133128
134129 $addressPart = false;
——@@ -136,7 +131,7 @@
137132 if ( $block ) {
138133 $encName = htmlspecialchars( $block->getRedactedName() );
139134 $encId = $this->id;
140 - $addressPart = $encName . Xml::hidden( 'id', $encId );
 135+ $addressPart = $encName . Html::hidden( 'id', $encId );
141136 $ipa = wfMsgHtml( $wgSysopUserBans ? 'ipadressorusername' : 'ipaddress' );
142137 }
143138 }
——@@ -146,10 +141,10 @@
147142 }
148143
149144 $wgOut->addHTML(
150 - Xml::openElement( 'form', array( 'method' => 'post', 'action' => $action, 'id' => 'unblockip' ) ) .
151 - Xml::openElement( 'fieldset' ) .
152 - Xml::element( 'legend', null, wfMsg( 'ipb-unblock' ) ) .
153 - Xml::openElement( 'table', array( 'id' => 'mw-unblock-table' ) ).
 145+ Html::openElement( 'form', array( 'method' => 'post', 'action' => $action, 'id' => 'unblockip' ) ) .
 146+ Html::openElement( 'fieldset' ) .
 147+ Html::element( 'legend', null, wfMsg( 'ipb-unblock' ) ) .
 148+ Html::openElement( 'table', array( 'id' => 'mw-unblock-table' ) ).
154149 "<tr>
155150 <td class='mw-label'>
156151 {$ipa}
——@@ -172,10 +167,10 @@
173168 Xml::submitButton( wfMsg( 'ipusubmit' ), array( 'name' => 'wpBlock', 'tabindex' => '3' ) ) .
174169 "</td>
175170 </tr>" .
176 - Xml::closeElement( 'table' ) .
177 - Xml::closeElement( 'fieldset' ) .
178 - Xml::hidden( 'wpEditToken', $wgUser->editToken() ) .
179 - Xml::closeElement( 'form' ) . "\n"
 171+ Html::closeElement( 'table' ) .
 172+ Html::closeElement( 'fieldset' ) .
 173+ Html::hidden( 'wpEditToken', $wgUser->editToken() ) .
 174+ Html::closeElement( 'form' ) . "\n"
180175 );
181176
182177 }
——@@ -192,12 +187,11 @@
193188 * case it contains the range $ip is part of.
194189 * @return array array(message key, parameters) on failure, empty array on success
195190 */
196 -
197 - static function doUnblock(&$id, &$ip, &$reason, &$range = null, $blocker=null) {
 191+ public static function doUnblock( &$id, &$ip, &$reason, &$range = null, $blocker = null ) {
198192 if ( $id ) {
199193 $block = Block::newFromID( $id );
200194 if ( !$block ) {
201 - return array('ipb_cant_unblock', htmlspecialchars($id));
 195+ return array( 'ipb_cant_unblock', htmlspecialchars( $id ) );
202196 }
203197 $ip = $block->getRedactedName();
204198 } else {
——@@ -207,19 +201,19 @@
208202 $id = substr( $ip, 1 );
209203 $block = Block::newFromID( $id );
210204 if( !$block ) {
211 - return array('ipb_cant_unblock', htmlspecialchars($id));
 205+ return array( 'ipb_cant_unblock', htmlspecialchars( $id ) );
212206 }
213207 $ip = $block->getRedactedName();
214208 } else {
215209 $block = Block::newFromDB( $ip );
216210 if ( !$block ) {
217 - return array('ipb_cant_unblock', htmlspecialchars($id));
 211+ return array( 'ipb_cant_unblock', htmlspecialchars( $id ) );
218212 }
219213 if( $block->mRangeStart != $block->mRangeEnd && !strstr( $ip, "/" ) ) {
220214 /* If the specified IP is a single address, and the block is
221215 * a range block, don't unblock the range. */
222216 $range = $block->mAddress;
223 - return array('ipb_blocked_as_range', $ip, $range);
 217+ return array( 'ipb_blocked_as_range', $ip, $range );
224218 }
225219 }
226220 }
——@@ -228,18 +222,18 @@
229223
230224 # If the name was hidden and the blocking user cannot hide
231225 # names, then don't allow any block removals...
232 - if( $blocker && $block->mHideName && !$blocker->isAllowed('hideuser') ) {
233 - return array('ipb_cant_unblock', htmlspecialchars($id));
 226+ if( $blocker && $block->mHideName && !$blocker->isAllowed( 'hideuser' ) ) {
 227+ return array( 'ipb_cant_unblock', htmlspecialchars( $id ) );
234228 }
235229
236230 # Delete block
237231 if ( !$block->delete() ) {
238 - return array('ipb_cant_unblock', htmlspecialchars($id));
 232+ return array( 'ipb_cant_unblock', htmlspecialchars( $id ) );
239233 }
240234
241235 # Unset _deleted fields as needed
242236 if( $block->mHideName ) {
243 - IPBlockForm::unsuppressUserName( $block->mAddress, $block->mUser );
 237+ self::unsuppressUserName( $block->mAddress, $block->mUser );
244238 }
245239
246240 # Make log entry
——@@ -250,23 +244,23 @@
251245
252246 function doSubmit() {
253247 global $wgOut, $wgUser;
254 - $retval = self::doUnblock($this->id, $this->ip, $this->reason, $range, $wgUser);
255 - if( !empty($retval) ) {
256 - $key = array_shift($retval);
257 - $this->showForm(wfMsgReal($key, $retval));
 248+
 249+ $retval = self::doUnblock( $this->id, $this->ip, $this->reason, $range, $wgUser );
 250+ if( !empty( $retval ) ) {
 251+ $key = array_shift( $retval );
 252+ $this->showForm( wfMsgReal( $key, $retval ) );
258253 return;
259254 }
 255+
260256 # Report to the user
261 - $titleObj = SpecialPage::getTitleFor( "Ipblocklist" );
262 - $success = $titleObj->getFullURL( "action=success&successip=" . urlencode( $this->ip ) );
 257+ $success = $this->getTitle()->getFullURL( 'action=success&successip=' . urlencode( $this->ip ) );
263258 $wgOut->redirect( $success );
264259 }
265260
266261 function showList( $msg ) {
267262 global $wgOut, $wgUser;
268263
269 - $wgOut->setPagetitle( wfMsg( "ipblocklist" ) );
270 - if ( $msg != "" ) {
 264+ if ( $msg != '' ) {
271265 $wgOut->setSubtitle( $msg );
272266 }
273267
——@@ -285,25 +279,25 @@
286280 } elseif ( substr( $this->ip, 0, 1 ) == '#' ) {
287281 $conds['ipb_id'] = substr( $this->ip, 1 );
288282 // Single IPs
289 - } elseif ( IP::isIPAddress($this->ip) && strpos($this->ip,'/') === false ) {
290 - if( $iaddr = IP::toHex($this->ip) ) {
 283+ } elseif ( IP::isIPAddress( $this->ip ) && strpos( $this->ip, '/' ) === false ) {
 284+ if( $iaddr = IP::toHex( $this->ip ) ) {
291285 # Only scan ranges which start in this /16, this improves search speed
292286 # Blocks should not cross a /16 boundary.
293287 $range = substr( $iaddr, 0, 4 );
294288 // Fixme -- encapsulate this sort of query-building.
295289 $dbr = wfGetDB( DB_SLAVE );
296 - $encIp = $dbr->addQuotes( IP::sanitizeIP($this->ip) );
 290+ $encIp = $dbr->addQuotes( IP::sanitizeIP( $this->ip ) );
297291 $encAddr = $dbr->addQuotes( $iaddr );
298292 $conds[] = "(ipb_address = $encIp) OR
299293 (ipb_range_start" . $dbr->buildLike( $range, $dbr->anyString() ) . " AND
300294 ipb_range_start <= $encAddr
301295 AND ipb_range_end >= $encAddr)";
302296 } else {
303 - $conds['ipb_address'] = IP::sanitizeIP($this->ip);
 297+ $conds['ipb_address'] = IP::sanitizeIP( $this->ip );
304298 }
305299 $conds['ipb_auto'] = 0;
306300 // IP range
307 - } elseif ( IP::isIPAddress($this->ip) ) {
 301+ } elseif ( IP::isIPAddress( $this->ip ) ) {
308302 $conds['ipb_address'] = Block::normaliseRange( $this->ip );
309303 $conds['ipb_auto'] = 0;
310304 } else {
——@@ -345,7 +339,7 @@
346340 if ( $pager->getNumRows() ) {
347341 $wgOut->addHTML(
348342 $pager->getNavigationBar() .
349 - Xml::tags( 'ul', null, $pager->getBody() ) .
 343+ Html::rawElement( 'ul', null, $pager->getBody() ) .
350344 $pager->getNavigationBar()
351345 );
352346 } elseif ( $this->ip != '') {
——@@ -395,15 +389,15 @@
396390 $hl = $wgLang->pipeList( $links );
397391
398392 return
399 - Xml::tags( 'form', array( 'action' => $wgScript ),
400 - Xml::hidden( 'title', SpecialPage::getTitleFor( 'Ipblocklist' )->getPrefixedDbKey() ) .
401 - Xml::openElement( 'fieldset' ) .
402 - Xml::element( 'legend', null, wfMsg( 'ipblocklist-legend' ) ) .
 393+ Html::rawElement( 'form', array( 'action' => $wgScript ),
 394+ Html::hidden( 'title', $this->getTitle()->getPrefixedDbKey() ) .
 395+ Html::openElement( 'fieldset' ) .
 396+ Html::element( 'legend', null, wfMsg( 'ipblocklist-legend' ) ) .
403397 Xml::inputLabel( wfMsg( 'ipblocklist-username' ), 'ip', 'ip', /* size */ false, $this->ip ) .
404398 '&#160;' .
405399 Xml::submitButton( wfMsg( 'ipblocklist-submit' ) ) . '<br />' .
406400 $hl .
407 - Xml::closeElement( 'fieldset' )
 401+ Html::closeElement( 'fieldset' )
408402 );
409403 }
410404
——@@ -417,8 +411,7 @@
418412 global $wgUser;
419413 $sk = $wgUser->getSkin();
420414 $params = $override + $options;
421 - $ipblocklist = SpecialPage::getTitleFor( 'Ipblocklist' );
422 - return $sk->link( $ipblocklist, htmlspecialchars( $title ),
 415+ return $sk->link( $this->getTitle(), htmlspecialchars( $title ),
423416 ( $active ? array( 'style'=>'font-weight: bold;' ) : array() ), $params, array( 'known' ) );
424417 }
425418
——@@ -487,7 +480,7 @@
488481 $changeblocklink = '';
489482 $toolLinks = '';
490483 if ( $wgUser->isAllowed( 'block' ) ) {
491 - $unblocklink = $sk->link( SpecialPage::getTitleFor( 'Ipblocklist' ),
 484+ $unblocklink = $sk->link( $this->getTitle(),
492485 $msg['unblocklink'],
493486 array(),
494487 array( 'action' => 'unblock', 'id' => $block->mId ),
Index: trunk/phase3/includes/SpecialPage.php
——@@ -121,7 +121,7 @@
122122
123123 # Users and rights
124124 'Blockip' => array( 'SpecialPage', 'Blockip', 'block' ),
125 - 'Ipblocklist' => array( 'SpecialPage', 'Ipblocklist' ),
 125+ 'Ipblocklist' => 'IPUnblockForm',
126126 'Unblock' => array( 'SpecialRedirectToSpecial', 'Unblock', 'Ipblocklist', false, array( 'uselang', 'ip', 'id' ), array( 'action' => 'unblock' ) ),
127127 'Resetpass' => 'SpecialResetpass',
128128 'DeletedContributions' => 'DeletedContributionsPage',

Follow-up revisions

RevisionCommit summaryAuthorDate
r69484Corrected fatal errors introduced in r68999 (oops!)ialex17:38, 17 July 2010

Comments

#Comment by VasilievVV (talk | contribs)   15:39, 17 July 2010

- $status = IPBlockForm::checkUnblockSelf( $user ) + $status = self::checkUnblockSelf( $user );

I believe you confused IPBlockForm and IPUnblockForm. Due to this confusion, self-unblock attempt now results in PHP error.

#Comment by IAlex (talk | contribs)   17:39, 17 July 2010

Fixed in r69484.

#Comment by Hashar (talk | contribs)   19:22, 21 October 2010

marked follow up OK.

#Comment by Umherirrender (talk | contribs)   19:30, 31 March 2011

Bug 28352 - self unblock with username does not work under 1.17:
$ip is the wrong var, you have to use $this->ip, because that reflect wpUnblockAddress, the form field with the username 

Index: includes/specials/SpecialIpblocklist.php
@@ -77,7 +77,7 @@
-$user = User::newFromName( $ip );
+$user = User::newFromName( $this->ip );

Status & tagging log