r83755 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r83754‎ | r83755 | r83756 >
Date:12:13, 12 March 2011
Author:happy-melon
Status:resolved (Comments)
Tags:
Comment:
Deprecate $wgSysopUserBans and $wgSysopRangeBans, both of which are pre-1.2, and totally antiquated. Can't think of any reason why a modern wiki might want to make blocks IP-only; syadmins can still disable rangeblocks by setting $wgBlockCIDRLimit to the maximum for each IP mode (32 for IP4, 128 for IP6).
Modified paths:
  • /trunk/extensions/CleanChanges/CleanChanges_body.php (modified) (history)
  • /trunk/phase3/includes/DefaultSettings.php (modified) (history)
  • /trunk/phase3/includes/Linker.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialBlockip.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialContributions.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialDeletedContributions.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialIpblocklist.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/Linker.php
——@@ -880,9 +880,9 @@
881881 * @return String: HTML fragment
882882 */
883883 public function userToolLinks( $userId, $userText, $redContribsWhenNoEdits = false, $flags = 0, $edits = null ) {
884 - global $wgUser, $wgDisableAnonTalk, $wgSysopUserBans, $wgLang;
 884+ global $wgUser, $wgDisableAnonTalk, $wgLang;
885885 $talkable = !( $wgDisableAnonTalk && 0 == $userId );
886 - $blockable = ( $wgSysopUserBans || 0 == $userId ) && !$flags & self::TOOL_LINKS_NOBLOCK;
 886+ $blockable = !$flags & self::TOOL_LINKS_NOBLOCK;
887887
888888 $items = array();
889889 if ( $talkable ) {
Index: trunk/phase3/includes/DefaultSettings.php
——@@ -3175,10 +3175,16 @@
31763176 * @{
31773177 */
31783178
3179 -/** Allow sysops to ban logged-in users */
 3179+/**
 3180+ * Allow sysops to ban logged-in users
 3181+ * @deprecated @since 1.18
 3182+ */
31803183 $wgSysopUserBans = true;
31813184
3182 -/** Allow sysops to ban IP ranges */
 3185+/**
 3186+ * Allow sysops to ban IP ranges
 3187+ * @deprecated @since 1.18; set $wgBlockCIDRLimit to array( 'IPv4' => 32, 'IPv6 => 128 ) instead.
 3188+ */
31833189 $wgSysopRangeBans = true;
31843190
31853191 /**
Index: trunk/phase3/includes/specials/SpecialIpblocklist.php
——@@ -121,7 +121,7 @@
122122 * @return $out string: HTML form
123123 */
124124 function showForm( $err = null ) {
125 - global $wgOut, $wgUser, $wgSysopUserBans;
 125+ global $wgOut, $wgUser;
126126
127127 $wgOut->addWikiMsg( 'unblockiptext' );
128128
——@@ -139,12 +139,12 @@
140140 $encName = htmlspecialchars( $block->getRedactedName() );
141141 $encId = $this->id;
142142 $addressPart = $encName . Html::hidden( 'id', $encId );
143 - $ipa = wfMsgHtml( $wgSysopUserBans ? 'ipadressorusername' : 'ipaddress' );
 143+ $ipa = wfMsgHtml( 'ipadressorusername' );
144144 }
145145 }
146146 if ( !$addressPart ) {
147147 $addressPart = Xml::input( 'wpUnblockAddress', 40, $this->ip, array( 'type' => 'text', 'tabindex' => '1' ) );
148 - $ipa = Xml::label( wfMsg( $wgSysopUserBans ? 'ipadressorusername' : 'ipaddress' ), 'wpUnblockAddress' );
 148+ $ipa = Xml::label( wfMsg( 'ipadressorusername' ), 'wpUnblockAddress' );
149149 }
150150
151151 $wgOut->addHTML(
Index: trunk/phase3/includes/specials/SpecialContributions.php
——@@ -240,7 +240,6 @@
241241 * @param $subject User: The viewing user ($wgUser is still checked in some cases, like userrights page!!)
242242 */
243243 public static function getUserLinks( Title $userpage, Title $talkpage, User $target, User $subject ) {
244 - global $wgSysopUserBans;
245244
246245 $sk = $subject->getSkin();
247246 $id = $target->getId();
——@@ -248,7 +247,7 @@
249248
250249 $tools[] = $sk->link( $talkpage, wfMsgHtml( 'sp-contributions-talk' ) );
251250
252 - if( ( $id !== null && $wgSysopUserBans ) || ( $id === null && IP::isIPAddress( $username ) ) ) {
 251+ if( ( $id !== null ) || ( $id === null && IP::isIPAddress( $username ) ) ) {
253252 if( $subject->isAllowed( 'block' ) ) { # Block / Change block / Unblock links
254253 if ( $target->isBlocked() ) {
255254 $tools[] = $sk->linkKnown( # Change block link
Index: trunk/phase3/includes/specials/SpecialBlockip.php
——@@ -106,16 +106,12 @@
107107 }
108108
109109 public function showForm( $err ) {
110 - global $wgOut, $wgUser, $wgSysopUserBans;
 110+ global $wgOut, $wgUser;
111111
112112 $wgOut->setPageTitle( wfMsg( 'blockip-title' ) );
113113 $wgOut->addWikiMsg( 'blockiptext' );
114114
115 - if( $wgSysopUserBans ) {
116 - $mIpaddress = Xml::label( wfMsg( 'ipadressorusername' ), 'mw-bi-target' );
117 - } else {
118 - $mIpaddress = Xml::label( wfMsg( 'ipaddress' ), 'mw-bi-target' );
119 - }
 115+ $mIpaddress = Xml::label( wfMsg( 'ipadressorusername' ), 'mw-bi-target' );
120116 $mIpbexpiry = Xml::label( wfMsg( 'ipbexpiry' ), 'wpBlockExpiry' );
121117 $mIpbother = Xml::label( wfMsg( 'ipbother' ), 'mw-bi-other' );
122118 $mIpbreasonother = Xml::label( wfMsg( 'ipbreason' ), 'wpBlockReasonList' );
——@@ -426,7 +422,7 @@
427423 * @return array(message key, arguments) on failure, empty array on success
428424 */
429425 function doBlock( &$userId = null, &$expiry = null ) {
430 - global $wgUser, $wgSysopUserBans, $wgSysopRangeBans, $wgBlockAllowsUTEdit, $wgBlockCIDRLimit;
 426+ global $wgUser, $wgBlockAllowsUTEdit, $wgBlockCIDRLimit;
431427
432428 $userId = 0;
433429 # Expand valid IPv6 addresses, usernames are left as is
——@@ -441,7 +437,7 @@
442438 $matches = array();
443439 if( preg_match( "/^($rxIP4)\\/(\\d{1,2})$/", $this->BlockAddress, $matches ) ) {
444440 # IPv4
445 - if( $wgSysopRangeBans ) {
 441+ if( $wgBlockCIDRLimit['IPv4'] != 32 ){
446442 if( !IP::isIPv4( $this->BlockAddress ) || $matches[2] > 32 ) {
447443 return array( 'ip_range_invalid' );
448444 } elseif ( $matches[2] < $wgBlockCIDRLimit['IPv4'] ) {
——@@ -454,7 +450,7 @@
455451 }
456452 } elseif( preg_match( "/^($rxIP6)\\/(\\d{1,3})$/", $this->BlockAddress, $matches ) ) {
457453 # IPv6
458 - if( $wgSysopRangeBans ) {
 454+ if( $wgBlockCIDRLimit['IPv6'] != 128 ) {
459455 if( !IP::isIPv6( $this->BlockAddress ) || $matches[2] > 128 ) {
460456 return array( 'ip_range_invalid' );
461457 } elseif( $matches[2] < $wgBlockCIDRLimit['IPv6'] ) {
——@@ -467,17 +463,13 @@
468464 }
469465 } else {
470466 # Username block
471 - if( $wgSysopUserBans ) {
472 - $user = User::newFromName( $this->BlockAddress );
473 - if( $user instanceof User && $user->getId() ) {
474 - # Use canonical name
475 - $userId = $user->getId();
476 - $this->BlockAddress = $user->getName();
477 - } else {
478 - return array( 'nosuchusershort', htmlspecialchars( $user ? $user->getName() : $this->BlockAddress ) );
479 - }
 467+ $user = User::newFromName( $this->BlockAddress );
 468+ if( $user instanceof User && $user->getId() ) {
 469+ # Use canonical name
 470+ $userId = $user->getId();
 471+ $this->BlockAddress = $user->getName();
480472 } else {
481 - return array( 'badipaddress' );
 473+ return array( 'nosuchusershort', htmlspecialchars( $user ? $user->getName() : $this->BlockAddress ) );
482474 }
483475 }
484476 }
Index: trunk/phase3/includes/specials/SpecialDeletedContributions.php
——@@ -354,7 +354,7 @@
355355 * @todo Fixme: almost the same as contributionsSub in SpecialContributions.php. Could be combined.
356356 */
357357 function getSubTitle( $nt, $id ) {
358 - global $wgSysopUserBans, $wgLang, $wgUser, $wgOut;
 358+ global $wgLang, $wgUser, $wgOut;
359359
360360 $sk = $wgUser->getSkin();
361361
——@@ -368,7 +368,7 @@
369369 if( $talk ) {
370370 # Talk page link
371371 $tools[] = $sk->link( $talk, wfMsgHtml( 'sp-contributions-talk' ) );
372 - if( ( $id !== null && $wgSysopUserBans ) || ( $id === null && IP::isIPAddress( $nt->getText() ) ) ) {
 372+ if( ( $id !== null ) || ( $id === null && IP::isIPAddress( $nt->getText() ) ) ) {
373373 if( $wgUser->isAllowed( 'block' ) ) { # Block / Change block / Unblock links
374374 if ( $userObj->isBlocked() ) {
375375 $tools[] = $sk->linkKnown( # Change block link
Index: trunk/extensions/CleanChanges/CleanChanges_body.php
——@@ -432,9 +432,8 @@
433433 * Enhanced user tool links, with javascript functionality.
434434 */
435435 public function userToolLinks( $userId, $userText ) {
436 - global $wgUser, $wgDisableAnonTalk, $wgSysopUserBans;
 436+ global $wgUser, $wgDisableAnonTalk;
437437 $talkable = !( $wgDisableAnonTalk && 0 == $userId );
438 - $blockable = ( $wgSysopUserBans || 0 == $userId );
439438
440439 /*
441440 * Assign each different user a running id. This is used to show user tool
——@@ -483,7 +482,7 @@
484483 $items[] = $this->skin->makeKnownLinkObj( $targetPage,
485484 wfMsgHtml( 'contribslink' ) );
486485 }
487 - if( $blockable && $wgUser->isAllowed( 'block' ) ) {
 486+ if( $wgUser->isAllowed( 'block' ) ) {
488487 $items[] = $this->skin->blockLink( $userId, $userText );
489488 }
490489 if( $userId ) {

Follow-up revisions

RevisionCommit summaryAuthorDate
r83757Follow-up r83755: Message 'ipaddress' is unused now, in core and extensionsraymond13:42, 12 March 2011
r83772Follow-up r83755: @deprecated @since is wrong, doesn't have the expected sema...happy-melon18:14, 12 March 2011
r83776Backport the deprecation of $wgSysopRangeBans and $wgSysopUserBans from r8375...happy-melon18:43, 12 March 2011
r83777RELEASE-NOTES for r83755happy-melon18:51, 12 March 2011
r86125Followup r83755: You removed them, you didn't deprecate them (there is a diff...demon18:06, 15 April 2011

Comments

#Comment by Nikerabbit (talk | contribs)   16:55, 12 March 2011

I asked about this before, and now I checked it: @deprecated @since is wrong. See for example http://svn.wikimedia.org/doc/classTitle.html#a7ae5d9288dfbd52d3f0b9581aa557fe5

#Comment by Happy-melon (talk | contribs)   17:39, 12 March 2011

As opposed to, say, http://svn.wikimedia.org/doc/classCategoryViewer.html#a5c61115ab1191580e9e8408bed738e55 ?? I do see what you mean.

#Comment by Happy-melon (talk | contribs)   18:16, 12 March 2011

Fixed in r83772.

#Comment by Nikerabbit (talk | contribs)   16:57, 12 March 2011

Besides, you are not deprecating the variables, because they no longer work! Either they work or they should be removed and clear notice put into RELEASE-NOTES

#Comment by Happy-melon (talk | contribs)   17:31, 12 March 2011

They do indeed no longer work. However, the DefaultSettings implementation needs to be left, AFAIK, for some time to guard against register_globals attacks; therefore they need to be marked with something, might as well be @deprecated.

I do agree it needs RELEASE-NOTES; as you might have guessed I'm working through a full rewrite of SpecialBlockip.php which makes quite a few changes, and was going to bundle them all up into one entry; but I guess there's no reason why that entry can't be added to over multiple revisions.

#Comment by Nikerabbit (talk | contribs)   17:48, 12 March 2011

There is no register_globals issue if they are never read anywhere. Didn't you remove all uses of them?

#Comment by Happy-melon (talk | contribs)   18:10, 12 March 2011

They might be read in non-svn extensions. Normally I wouldn't care, but security is security... Can't we just start to die on detecting register_globals? Would make our lives a lot easier...

#Comment by Nikerabbit (talk | contribs)   18:22, 12 March 2011

Doesn't installer barf on it already?

#Comment by MaxSem (talk | contribs)   18:31, 12 March 2011

It barfs, but allows to continue. It's an unavoidable evil of having to live with hosts that strive to maintain "compatibility" with PHP 3 and other funny things.

#Comment by Krinkle (talk | contribs)   17:45, 12 March 2011

I'd say add a @deprecate note in REL1_17 and mention it in RELEASE-NOTES of REL1_17, then fully remove in trunk/1.18 and again note in release notes.

If a variable has @deprecated it should fully work as it did before, not like it is now.

#Comment by Happy-melon (talk | contribs)   18:08, 12 March 2011

We do have that option available, yes :D

It's certainly possible to deprecate $wgSysopRangeBans in that way, because there is an alternate route to achieving the same functionality. The functionality of $wgSysopUserBans is being completely removed, because it's useless in the modern software; there's no way to do that in a backwards-compatible way :D

#Comment by Happy-melon (talk | contribs)   18:44, 12 March 2011

Backported a deprecation in r83776; noted that you can achieve the functionality of $wgSysopUserBans with a hook if you're really desperate.

#Comment by Happy-melon (talk | contribs)   16:16, 14 March 2011

Think these issues are resolved? Resetting to new.

#Comment by 😂 (talk | contribs)   18:01, 15 April 2011

This has to do with bug 18807, yay :)

Status & tagging log