Make constructor of Block accept array of options
authorMatěj Grabovský <mgrabovsky@yahoo.com>
Thu, 22 May 2014 14:45:46 +0000 (16:45 +0200)
committerBrad Jorsch <bjorsch@wikimedia.org>
Fri, 19 Jun 2015 18:20:01 +0000 (14:20 -0400)
Block::__construct now accepts an array of options instead of a myriad
of optional parameters.

Also add a test for the old constructor.

Change-Id: I6ccd4df569ab49ad841a1ad591e23cafb1715841

RELEASE-NOTES-1.26
includes/Block.php
tests/phpunit/includes/BlockTest.php
tests/phpunit/includes/TitlePermissionTest.php

index 419525b..896d1d5 100644 (file)
@@ -93,6 +93,9 @@ changes to languages because of Bugzilla reports.
 * wfSuppressWarnings() and wfRestoreWarnings() were split into a separate library,
   mediawiki/at-ease, and are now deprecated. Callers should use
   MediaWiki\suppressWarnings() and MediaWiki\restoreWarnings() directly.
+* The Block class constructor now takes an associative array of parameters
+  instead of many optional positional arguments. Calling the constructor the old
+  way will issue a deprecation warning.
 
 
 == Compatibility ==
index 85cfe59..2cbf2d7 100644 (file)
@@ -23,15 +23,16 @@ class Block {
        /** @var string */
        public $mReason;
 
-       /** @var bool|string */
+       /** @var string */
        public $mTimestamp;
 
-       /** @var int */
+       /** @var bool */
        public $mAuto;
 
-       /** @var bool|string */
+       /** @var string */
        public $mExpiry;
 
+       /** @var bool */
        public $mHideName;
 
        /** @var int */
@@ -65,10 +66,10 @@ class Block {
        protected $blocker;
 
        /** @var bool */
-       protected $isHardblock = true;
+       protected $isHardblock;
 
        /** @var bool */
-       protected $isAutoblocking = true;
+       protected $isAutoblocking;
 
        # TYPE constants
        const TYPE_USER = 1;
@@ -78,55 +79,84 @@ class Block {
        const TYPE_ID = 5;
 
        /**
-        * @todo FIXME: Don't know what the best format to have for this constructor
-        *   is, but fourteen optional parameters certainly isn't it.
-        * @param string $address
-        * @param int $user
-        * @param int $by
-        * @param string $reason
-        * @param mixed $timestamp
-        * @param int $auto
-        * @param string $expiry
-        * @param int $anonOnly
-        * @param int $createAccount
-        * @param int $enableAutoblock
-        * @param int $hideName
-        * @param int $blockEmail
-        * @param int $allowUsertalk
-        * @param string $byText
+        * Create a new block with specified parameters on a user, IP or IP range.
+        *
+        * @param array $options Parameters of the block:
+        *     address string|User  Target user name, User object, IP address or IP range
+        *     user int             Override target user ID (for foreign users)
+        *     by int               User ID of the blocker
+        *     reason string        Reason of the block
+        *     timestamp string     The time at which the block comes into effect
+        *     auto bool            Is this an automatic block?
+        *     expiry string        Timestamp of expiration of the block or 'infinity'
+        *     anonOnly bool        Only disallow anonymous actions
+        *     createAccount bool   Disallow creation of new accounts
+        *     enableAutoblock bool Enable automatic blocking
+        *     hideName bool        Hide the target user name
+        *     blockEmail bool      Disallow sending emails
+        *     allowUsertalk bool   Allow the target to edit its own talk page
+        *     byText string        Username of the blocker (for foreign users)
+        *
+        * @since 1.26 accepts $options array instead of individual parameters; order
+        * of parameters above reflects the original order
         */
-       function __construct( $address = '', $user = 0, $by = 0, $reason = '',
-               $timestamp = 0, $auto = 0, $expiry = '', $anonOnly = 0, $createAccount = 0, $enableAutoblock = 0,
-               $hideName = 0, $blockEmail = 0, $allowUsertalk = 0, $byText = ''
-       ) {
-               if ( $timestamp === 0 ) {
-                       $timestamp = wfTimestampNow();
-               }
+       function __construct( $options = array() ) {
+               $defaults = array(
+                       'address'         => '',
+                       'user'            => null,
+                       'by'              => null,
+                       'reason'          => '',
+                       'timestamp'       => '',
+                       'auto'            => false,
+                       'expiry'          => '',
+                       'anonOnly'        => false,
+                       'createAccount'   => false,
+                       'enableAutoblock' => false,
+                       'hideName'        => false,
+                       'blockEmail'      => false,
+                       'allowUsertalk'   => false,
+                       'byText'          => '',
+               );
 
-               if ( count( func_get_args() ) > 0 ) {
-                       # Soon... :D
-                       # wfDeprecated( __METHOD__ . " with arguments" );
+               if ( func_num_args() > 1 || !is_array( $options ) ) {
+                       $options = array_combine(
+                               array_slice( array_keys( $defaults ), 0, func_num_args() ),
+                               func_get_args()
+                       );
+                       //wfDeprecated( __METHOD__ . ' with multiple arguments', '1.26' );
                }
 
-               $this->setTarget( $address );
-               if ( $this->target instanceof User && $user ) {
-                       $this->forcedTargetID = $user; // needed for foreign users
+               $options += $defaults;
+
+               $this->setTarget( $options['address'] );
+
+               if ( $this->target instanceof User && $options['user'] ) {
+                       # Needed for foreign users
+                       $this->forcedTargetID = $options['user'];
                }
-               if ( $by ) { // local user
-                       $this->setBlocker( User::newFromId( $by ) );
-               } else { // foreign user
-                       $this->setBlocker( $byText );
+
+               if ( $options['by'] ) {
+                       # Local user
+                       $this->setBlocker( User::newFromID( $options['by'] ) );
+               } else {
+                       # Foreign user
+                       $this->setBlocker( $options['byText'] );
                }
-               $this->mReason = $reason;
-               $this->mTimestamp = wfTimestamp( TS_MW, $timestamp );
-               $this->mAuto = $auto;
-               $this->isHardblock( !$anonOnly );
-               $this->prevents( 'createaccount', $createAccount );
-               $this->mExpiry = wfGetDB( DB_SLAVE )->decodeExpiry( $expiry );
-               $this->isAutoblocking( $enableAutoblock );
-               $this->mHideName = $hideName;
-               $this->prevents( 'sendemail', $blockEmail );
-               $this->prevents( 'editownusertalk', !$allowUsertalk );
+
+               $this->mReason = $options['reason'];
+               $this->mTimestamp = wfTimestamp( TS_MW, $options['timestamp'] );
+               $this->mExpiry = wfGetDB( DB_SLAVE )->decodeExpiry( $options['expiry'] );
+
+               # Boolean settings
+               $this->mAuto = (bool)$options['auto'];
+               $this->mHideName = (bool)$options['hideName'];
+               $this->isHardblock( !$options['anonOnly'] );
+               $this->isAutoblocking( (bool)$options['enableAutoblock'] );
+
+               # Prevention measures
+               $this->prevents( 'sendemail', (bool)$options['blockEmail'] );
+               $this->prevents( 'editownusertalk', !$options['allowUsertalk'] );
+               $this->prevents( 'createaccount', (bool)$options['createAccount'] );
 
                $this->mFromMaster = false;
        }
index 1974162..7b0de86 100644 (file)
@@ -38,9 +38,13 @@ class BlockTest extends MediaWikiLangTestCase {
                        $oldBlock->delete();
                }
 
-               $this->block = new Block( 'UTBlockee', $user->getID(), 0,
-                       'Parce que', 0, false, time() + 100500
+               $blockOptions = array(
+                       'address' => 'UTBlockee',
+                       'user' => $user->getID(),
+                       'reason' => 'Parce que',
+                       'expiry' => time() + 100500,
                );
+               $this->block = new Block( $blockOptions );
                $this->madeAt = wfTimestamp( TS_MW );
 
                $this->block->insert();
@@ -151,22 +155,19 @@ class BlockTest extends MediaWikiLangTestCase {
                );
 
                // Foreign perspective (blockee not on current wiki)...
-               $block = new Block(
-                       /* $address */ $username,
-                       /* $user */ 14146,
-                       /* $by */ 0,
-                       /* $reason */ 'crosswiki block...',
-                       /* $timestamp */ wfTimestampNow(),
-                       /* $auto */ false,
-                       /* $expiry */ $this->db->getInfinity(),
-                       /* anonOnly */ false,
-                       /* $createAccount */ true,
-                       /* $enableAutoblock */ true,
-                       /* $hideName (ipb_deleted) */ true,
-                       /* $blockEmail */ true,
-                       /* $allowUsertalk */ false,
-                       /* $byName */ 'MetaWikiUser'
+               $blockOptions = array(
+                       'address' => $username,
+                       'user' => 14146,
+                       'reason' => 'crosswiki block...',
+                       'timestamp' => wfTimestampNow(),
+                       'expiry' => $this->db->getInfinity(),
+                       'createAccount' => true,
+                       'enableAutoblock' => true,
+                       'hideName' => true,
+                       'blockEmail' => true,
+                       'byText' => 'MetaWikiUser',
                );
+               $block = new Block( $blockOptions );
                $block->insert();
 
                // Reload block from DB
@@ -208,22 +209,19 @@ class BlockTest extends MediaWikiLangTestCase {
                $this->db->update( 'user', array( 'user_id' => 14146 ), array( 'user_id' => $user->getId() ) );
 
                // Foreign perspective (blockee not on current wiki)...
-               $block = new Block(
-                       /* $address */ 'UserOnForeignWiki',
-                       /* $user */ 14146,
-                       /* $by */ 0,
-                       /* $reason */ 'crosswiki block...',
-                       /* $timestamp */ wfTimestampNow(),
-                       /* $auto */ false,
-                       /* $expiry */ $this->db->getInfinity(),
-                       /* anonOnly */ false,
-                       /* $createAccount */ true,
-                       /* $enableAutoblock */ true,
-                       /* $hideName (ipb_deleted) */ true,
-                       /* $blockEmail */ true,
-                       /* $allowUsertalk */ false,
-                       /* $byName */ 'MetaWikiUser'
+               $blockOptions = array(
+                       'address' => 'UserOnForeignWiki',
+                       'user' => 14146,
+                       'reason' => 'crosswiki block...',
+                       'timestamp' => wfTimestampNow(),
+                       'expiry' => $this->db->getInfinity(),
+                       'createAccount' => true,
+                       'enableAutoblock' => true,
+                       'hideName' => true,
+                       'blockEmail' => true,
+                       'byText' => 'MetaWikiUser',
                );
+               $block = new Block( $blockOptions );
 
                $res = $block->insert( $this->db );
                $this->assertTrue( (bool)$res['id'], 'Block succeeded' );
@@ -367,4 +365,56 @@ class BlockTest extends MediaWikiLangTestCase {
                $block = Block::chooseBlock( $xffblocks, $list );
                $this->assertEquals( $exResult, $block->mReason, 'Correct block type for XFF header ' . $xff );
        }
+
+       public function testDeprecatedConstructor() {
+               $this->hideDeprecated( 'Block::__construct with multiple arguments' );
+               $username = 'UnthinkablySecretRandomUsername';
+               $reason = 'being irrational';
+
+               # Set up the target
+               $u = User::newFromName( $username );
+               if ( $u->getID() == 0 ) {
+                       $u->setPassword( 'TotallyObvious' );
+                       $u->addToDatabase();
+               }
+               unset( $u );
+
+               # Make sure the user isn't blocked
+               $this->assertNull(
+                       Block::newFromTarget( $username ),
+                       "$username should not be blocked"
+               );
+
+               # Perform the block
+               $block = new Block(
+                       /* address */ $username,
+                       /* user */ 0,
+                       /* by */ 0,
+                       /* reason */ $reason,
+                       /* timestamp */ 0,
+                       /* auto */ false,
+                       /* expiry */ 0
+               );
+               $block->insert();
+
+               # Check target
+               $this->assertEquals(
+                       $block->getTarget()->getName(),
+                       $username,
+                       "Target should be set properly"
+               );
+
+               # Check supplied parameter
+               $this->assertEquals(
+                       $block->mReason,
+                       $reason,
+                       "Reason should be non-default"
+               );
+
+               # Check default parameter
+               $this->assertFalse(
+                       (bool)$block->prevents( 'createaccount' ),
+                       "Account creation should not be blocked by default"
+               );
+       }
 }
index fd10ae7..f588ed6 100644 (file)
@@ -753,8 +753,14 @@ class TitlePermissionTest extends MediaWikiLangTestCase {
                $prev = time();
                $now = time() + 120;
                $this->user->mBlockedby = $this->user->getId();
-               $this->user->mBlock = new Block( '127.0.8.1', 0, $this->user->getId(),
-                       'no reason given', $prev + 3600, 1, 0 );
+               $this->user->mBlock = new Block( array(
+                       'address' => '127.0.8.1',
+                       'by' => $this->user->getId(),
+                       'reason' => 'no reason given',
+                       'timestamp' => $prev + 3600,
+                       'auto' => true,
+                       'expiry' => 0
+               ) );
                $this->user->mBlock->mTimestamp = 0;
                $this->assertEquals( array( array( 'autoblockedtext',
                                '[[User:Useruser|Useruser]]', 'no reason given', '127.0.0.1',
@@ -770,8 +776,14 @@ class TitlePermissionTest extends MediaWikiLangTestCase {
                global $wgLocalTZoffset;
                $wgLocalTZoffset = -60;
                $this->user->mBlockedby = $this->user->getName();
-               $this->user->mBlock = new Block( '127.0.8.1', 0, $this->user->getId(),
-                       'no reason given', $now, 0, 10 );
+               $this->user->mBlock = new Block( array(
+                       'address' => '127.0.8.1',
+                       'by' => $this->user->getId(),
+                       'reason' => 'no reason given',
+                       'timestamp' => $now,
+                       'auto' => false,
+                       'expiry' => 10,
+               ) );
                $this->assertEquals( array( array( 'blockedtext',
                                '[[User:Useruser|Useruser]]', 'no reason given', '127.0.0.1',
                                'Useruser', null, '23:00, 31 December 1969', '127.0.8.1',