r84251 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r84250‎ | r84251 | r84252 >
Date:16:35, 18 March 2011
Author:happy-melon
Status:resolved (Comments)
Tags:
Comment:
First forays into the swamp of the Block.php backend:
* Move Block::parseExpiryInput() to the frontend SpecialBlock::parseExpiryInput()
* consolidate the several implementations of the MediaWiki:Ipblockoptions parsing into SpecialBlock::getSuggestedDurations()
Modified paths:
  • /trunk/extensions/AbuseFilter/AbuseFilter.class.php (modified) (history)
  • /trunk/extensions/CheckUser/CheckUser_body.php (modified) (history)
  • /trunk/extensions/GlobalBlocking/GlobalBlocking.class.php (modified) (history)
  • /trunk/phase3/includes/Block.php (modified) (history)
  • /trunk/phase3/includes/api/ApiQueryLogEvents.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialBlock.php (modified) (history)
  • /trunk/phase3/languages/Language.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/api/ApiQueryLogEvents.php
——@@ -239,7 +239,7 @@
240240 list( $vals2['duration'], $vals2['flags'] ) = $params;
241241
242242 // Indefinite blocks have no expiry time
243 - if ( Block::parseExpiryInput( $params[0] ) !== Block::infinity() ) {
 243+ if ( SpecialBlock::parseExpiryInput( $params[0] ) !== Block::infinity() ) {
244244 $vals2['expiry'] = wfTimestamp( TS_ISO_8601,
245245 strtotime( $params[0], wfTimestamp( TS_UNIX, $ts ) ) );
246246 }
Index: trunk/phase3/includes/specials/SpecialBlock.php
——@@ -120,7 +120,7 @@
121121 'validation-callback' => array( __CLASS__, 'validateTargetField' ),
122122 ),
123123 'Expiry' => array(
124 - 'type' => 'selectorother',
 124+ 'type' => self::getSuggestedDurations() === array() ? 'text' : 'selectorother',
125125 'label-message' => 'ipbexpiry',
126126 'required' => true,
127127 'tabindex' => '2',
——@@ -139,10 +139,6 @@
140140 ),
141141 );
142142
143 - if( wfMsgForContent( 'ipboptions' ) == '-' ){
144 - $a['Expiry']['type'] = 'text';
145 - }
146 -
147143 if( self::canBlockEmail( $wgUser ) ) {
148144 $a['DisableEmail'] = array(
149145 'type' => 'check',
——@@ -501,7 +497,7 @@
502498 }
503499
504500 if( ( strlen( $data['Expiry'] ) == 0) || ( strlen( $data['Expiry'] ) > 50 )
505 - || !Block::parseExpiryInput( $data['Expiry'] ) )
 501+ || !self::parseExpiryInput( $data['Expiry'] ) )
506502 {
507503 return array( 'ipb_expiry_invalid' );
508504 }
——@@ -548,7 +544,7 @@
549545 $data['Reason'][0], # Reason string
550546 wfTimestampNow(), # Block Timestamp
551547 0, # Is this an autoblock (no)
552 - Block::parseExpiryInput( $data['Expiry'] ), # Expiry time
 548+ self::parseExpiryInput( $data['Expiry'] ), # Expiry time
553549 !$data['HardBlock'], # Block anon only
554550 $data['CreateAccount'],
555551 $data['AutoBlock'],
——@@ -645,10 +641,20 @@
646642 * to the standard "**<duration>|<displayname>" format?
647643 * @return Array
648644 */
649 - protected static function getSuggestedDurations(){
 645+ public static function getSuggestedDurations( $lang = null ){
650646 $a = array();
651 - foreach( explode( ',', wfMsgForContent( 'ipboptions' ) ) as $option ) {
652 - if( strpos( $option, ':' ) === false ) $option = "$option:$option";
 647+ $msg = $lang === null
 648+ ? wfMessage( 'ipboptions' )->inContentLanguage()
 649+ : wfMessage( 'ipboptions' )->inLanguage( $lang );
 650+
 651+ if( $msg == '-' ){
 652+ return array();
 653+ }
 654+
 655+ foreach( explode( ',', $msg ) as $option ) {
 656+ if( strpos( $option, ':' ) === false ){
 657+ $option = "$option:$option";
 658+ }
653659 list( $show, $value ) = explode( ':', $option );
654660 $a[htmlspecialchars( $show )] = htmlspecialchars( $value );
655661 }
——@@ -656,6 +662,25 @@
657663 }
658664
659665 /**
 666+ * Convert a submitted expiry time, which may be relative ("2 weeks", etc) or absolute
 667+ * ("24 May 2034", etc), into an absolute timestamp we can put into the database.
 668+ * @param $expiry String: whatever was typed into the form
 669+ * @return String: timestamp or "infinity" string for the DB implementation
 670+ */
 671+ public static function parseExpiryInput( $expiry ) {
 672+ if ( $expiry == 'infinite' || $expiry == 'indefinite' ) {
 673+ $expiry = Block::infinity();
 674+ } else {
 675+ $expiry = strtotime( $expiry );
 676+ if ( $expiry < 0 || $expiry === false ) {
 677+ return false;
 678+ }
 679+ $expiry = wfTimestamp( TS_MW, $expiry );
 680+ }
 681+ return $expiry;
 682+ }
 683+
 684+ /**
660685 * Can we do an email block?
661686 * @param $user User: the sysop wanting to make a block
662687 * @return Boolean
Index: trunk/phase3/includes/Block.php
——@@ -885,8 +885,8 @@
886886 }
887887 }
888888
889 - $expiry = Block::decodeExpiry( $encoded_expiry );
890 - if ( $expiry == 'infinity' ) {
 889+ $expiry = self::decodeExpiry( $encoded_expiry );
 890+ if ( $expiry == self::infinity() ) {
891891 $expirystr = $msg['infiniteblock'];
892892 } else {
893893 global $wgLang;
——@@ -898,27 +898,20 @@
899899 return $expirystr;
900900 }
901901
 902+ # FIXME: everything above here is a mess, needs much cleaning up
 903+
902904 /**
903 - * Convert a typed-in expiry time into something we can put into the database.
904 - * @param $expiry_input String: whatever was typed into the form
905 - * @return String: more database friendly
 905+ * Convert a submitted expiry time, which may be relative ("2 weeks", etc) or absolute
 906+ * ("24 May 2034"), into an absolute timestamp we can put into the database.
 907+ * @param $expiry String: whatever was typed into the form
 908+ * @return String: timestamp or "infinity" string for th DB implementation
 909+ * @deprecated since 1.18 moved to SpecialBlock::parseExpiryInput()
906910 */
907 - public static function parseExpiryInput( $expiry_input ) {
908 - if ( $expiry_input == 'infinite' || $expiry_input == 'indefinite' ) {
909 - $expiry = 'infinity';
910 - } else {
911 - $expiry = strtotime( $expiry_input );
912 -
913 - if ( $expiry < 0 || $expiry === false ) {
914 - return false;
915 - }
916 - }
917 -
918 - return $expiry;
 911+ public static function parseExpiryInput( $expiry ) {
 912+ wfDeprecated( __METHOD__ );
 913+ return SpecialBlock::parseExpiryInput( $expiry );
919914 }
920915
921 - # FIXME: everything above here is a mess, needs much cleaning up
922 -
923916 /**
924917 * Given a target and the target's type, get an existing Block object if possible.
925918 * Note that passing an IP address will get an applicable rangeblock if the IP is
——@@ -1015,12 +1008,12 @@
10161009 }
10171010
10181011 public function getType(){
1019 - list( $target, $type ) = $this->getTargetAndType();
 1012+ list( /*...*/, $type ) = $this->getTargetAndType();
10201013 return $type;
10211014 }
10221015
10231016 public function getTarget(){
1024 - list( $target, $type ) = $this->getTargetAndType();
 1017+ list( $target, /*...*/ ) = $this->getTargetAndType();
10251018 return $target;
10261019 }
10271020 }
Index: trunk/phase3/languages/Language.php
——@@ -2675,28 +2675,19 @@
26762676 }
26772677
26782678 /**
2679 - * For translating of expiry times
2680 - * @param $str String: the validated block time in English
2681 - * @return Somehow translated block time
 2679+ * Maybe translate block durations. Note that this function is somewhat misnamed: it
 2680+ * deals with translating the *duration* ("1 week", "4 days", etc), not the expiry time
 2681+ * (which is an absolute timestamp).
 2682+ * @param $str String: the validated block duration in English
 2683+ * @return Somehow translated block duration
26822684 * @see LanguageFi.php for example implementation
26832685 */
26842686 function translateBlockExpiry( $str ) {
2685 - $scBlockExpiryOptions = $this->getMessageFromDB( 'ipboptions' );
2686 -
2687 - if ( $scBlockExpiryOptions == '-' ) {
2688 - return $str;
2689 - }
2690 -
2691 - foreach ( explode( ',', $scBlockExpiryOptions ) as $option ) {
2692 - if ( strpos( $option, ':' ) === false ) {
2693 - continue;
2694 - }
2695 - list( $show, $value ) = explode( ':', $option );
 2687+ foreach( SpecialBlock::getSuggestedDurations( $this ) as $show => $value ){
26962688 if ( strcmp( $str, $value ) == 0 ) {
26972689 return htmlspecialchars( trim( $show ) );
26982690 }
26992691 }
2700 -
27012692 return $str;
27022693 }
27032694
Index: trunk/extensions/GlobalBlocking/GlobalBlocking.class.php
——@@ -38,8 +38,7 @@
3939 return $result = array();
4040 }
4141
42 - if ( $user->isAllowed( 'ipblock-exempt' ) ||
43 - $user->isAllowed( 'globalblock-exempt' ) ) {
 42+ if ( $user->isAllowed( 'ipblock-exempt', 'globalblock-exempt' ) ) {
4443 // User is exempt from IP blocks.
4544 return $result = array();
4645 }
——@@ -243,7 +242,7 @@
244243 static function block( $address, $reason, $expiry, $options = array() ) {
245244 global $wgContLang;
246245
247 - $expiry = Block::parseExpiryInput( $expiry );
 246+ $expiry = SpecialBlock::parseExpiryInput( $expiry );
248247 $errors = self::insertBlock( $address, $reason, $expiry, $options );
249248
250249 if ( count($errors) > 0 )
Index: trunk/extensions/CheckUser/CheckUser_body.php
——@@ -296,7 +296,7 @@
297297 $usertalk = new Article( $userTalkTitle );
298298 $safeUsers[] = '[[' . $userTitle->getPrefixedText() . '|' . $userTitle->getText() . ']]';
299299 $expirestr = $u->getId() ? 'indefinite' : '1 week';
300 - $expiry = Block::parseExpiryInput( $expirestr );
 300+ $expiry = SpecialBlock::parseExpiryInput( $expirestr );
301301 $anonOnly = IP::isIPAddress( $u->getName() ) ? 1 : 0;
302302 // Create the block
303303 $block = new Block( $u->getName(), // victim
Index: trunk/extensions/AbuseFilter/AbuseFilter.class.php
——@@ -1015,7 +1015,7 @@
10161016 $block->mTimestamp = wfTimestampNow();
10171017 $block->mAnonOnly = 1;
10181018 $block->mCreateAccount = 1;
1019 - $block->mExpiry = Block::parseExpiryInput( $wgAbuseFilterBlockDuration );
 1019+ $block->mExpiry = SpecialBlock::parseExpiryInput( $wgAbuseFilterBlockDuration );
10201020
10211021 $block->insert();
10221022
——@@ -1058,7 +1058,7 @@
10591059 $block->mTimestamp = wfTimestampNow();
10601060 $block->mAnonOnly = 0;
10611061 $block->mCreateAccount = 1;
1062 - $block->mExpiry = Block::parseExpiryInput( '1 week' );
 1062+ $block->mExpiry = SpecialBlock::parseExpiryInput( '1 week' );
10631063
10641064 $block->insert();
10651065

Follow-up revisions

RevisionCommit summaryAuthorDate
r84475Blame hashar for this giant commit; he teased me for making so many smaller o...happy-melon19:12, 21 March 2011
r90857Follow-up r84251 - isAllowed( 'right', 'or-another-right' ) was reverted.happy-melon22:05, 26 June 2011

Comments

#Comment by Platonides (talk | contribs)   15:34, 21 March 2011

PHP Fatal error: Method Message::__toString() must not throw an exception in SpecialBlock.php (specials/SpecialBlock.php on line 647)

That's the 

if( $msg == '-' ){

Run tests/phpunit/includes/api/ApiBlockTest.php to reproduce

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

Presumably that's actually Message::__toString() it's complaining about, not anything in the Block stuff...?

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

Or I could actually read the error message... Ignore me :D

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

This was fixed in r84475.

#Comment by Aaron Schulz (talk | contribs)   06:31, 17 June 2011 
+ $user->isAllowed( 'ipblock-exempt', 'globalblock-exempt' )

Wasn't the OR behavior of isAllowed() changed reverted?

Also, I'm not sure I like the move of Block::parseExpiryInput().

#Comment by Aaron Schulz (talk | contribs)   06:42, 17 June 2011

Tagging this, though caused by r84266.

#Comment by Happy-melon (talk | contribs)   22:05, 26 June 2011

Reverted this part in r90857.

Status & tagging log