From 5a9d3916016c75d810791a36e00549c1213e8498 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Mat=C4=9Bj=20Grabovsk=C3=BD?= Date: Thu, 22 May 2014 16:45:46 +0200 Subject: [PATCH] Make constructor of Block accept array of options 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 | 3 + includes/Block.php | 126 +++++++++++------- tests/phpunit/includes/BlockTest.php | 114 +++++++++++----- .../phpunit/includes/TitlePermissionTest.php | 20 ++- 4 files changed, 179 insertions(+), 84 deletions(-) diff --git a/RELEASE-NOTES-1.26 b/RELEASE-NOTES-1.26 index 419525b587..896d1d583e 100644 --- a/RELEASE-NOTES-1.26 +++ b/RELEASE-NOTES-1.26 @@ -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 == diff --git a/includes/Block.php b/includes/Block.php index 85cfe59399..2cbf2d70e7 100644 --- a/includes/Block.php +++ b/includes/Block.php @@ -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; } diff --git a/tests/phpunit/includes/BlockTest.php b/tests/phpunit/includes/BlockTest.php index 197416218a..7b0de8667b 100644 --- a/tests/phpunit/includes/BlockTest.php +++ b/tests/phpunit/includes/BlockTest.php @@ -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" + ); + } } diff --git a/tests/phpunit/includes/TitlePermissionTest.php b/tests/phpunit/includes/TitlePermissionTest.php index fd10ae73e8..f588ed63b7 100644 --- a/tests/phpunit/includes/TitlePermissionTest.php +++ b/tests/phpunit/includes/TitlePermissionTest.php @@ -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', -- 2.20.1