From 3ea7bba4d46ba5869fd989867638d970bd5dee2f Mon Sep 17 00:00:00 2001 From: Max Semenik Date: Fri, 13 Jul 2018 20:14:06 -0700 Subject: [PATCH] ApiOptions: fix resetting some preferences to default For preferences like 'skin' that have a limited number of values, null is not a valid value, thus attempts to reset them fail with "Validation error for \"skin\": This value is required." Bug: T65080 Change-Id: I86554a6d30c8ab970740d8682fb2261476de0677 --- RELEASE-NOTES-1.32 | 1 + includes/api/ApiOptions.php | 16 +- tests/phpunit/includes/api/ApiOptionsTest.php | 259 ++++++++---------- 3 files changed, 130 insertions(+), 146 deletions(-) diff --git a/RELEASE-NOTES-1.32 b/RELEASE-NOTES-1.32 index d875017297..6c200d900b 100644 --- a/RELEASE-NOTES-1.32 +++ b/RELEASE-NOTES-1.32 @@ -122,6 +122,7 @@ production. * SpecialPage::execute() will now only call checkLoginSecurityLevel() if getLoginSecurityLevel() returns non-false. * (T43720, T46197) Improved page display title handling for category pages +* (T65080) Fixed resetting options of some types via API action=options. === Action API changes in 1.32 === * Added templated parameters. diff --git a/includes/api/ApiOptions.php b/includes/api/ApiOptions.php index fe7d10dde2..3ea827c13a 100644 --- a/includes/api/ApiOptions.php +++ b/includes/api/ApiOptions.php @@ -80,12 +80,18 @@ class ApiOptions extends ApiBase { switch ( $prefsKinds[$key] ) { case 'registered': // Regular option. - if ( $htmlForm === null ) { - // We need a dummy HTMLForm for the validate callback... - $htmlForm = new HTMLForm( [], $this ); + if ( $value === null ) { + // Reset it + $validation = true; + } else { + // Validate + if ( $htmlForm === null ) { + // We need a dummy HTMLForm for the validate callback... + $htmlForm = new HTMLForm( [], $this ); + } + $field = HTMLForm::loadInputFromParameters( $key, $prefs[$key], $htmlForm ); + $validation = $field->validate( $value, $user->getOptions() ); } - $field = HTMLForm::loadInputFromParameters( $key, $prefs[$key], $htmlForm ); - $validation = $field->validate( $value, $user->getOptions() ); break; case 'registered-multiselect': case 'registered-checkmatrix': diff --git a/tests/phpunit/includes/api/ApiOptionsTest.php b/tests/phpunit/includes/api/ApiOptionsTest.php index 29c7dae50d..30ba1c1b66 100644 --- a/tests/phpunit/includes/api/ApiOptionsTest.php +++ b/tests/phpunit/includes/api/ApiOptionsTest.php @@ -61,6 +61,11 @@ class ApiOptionsTest extends MediaWikiLangTestCase { [ $this, 'hookGetPreferences' ] ] ] ); + $this->mergeMwGlobalArrayValue( 'wgDefaultUserOptions', [ + 'testradio' => 'option1', + ] ); + // Workaround for static caching in User::getDefaultOptions() + $this->setContentLang( Language::factory( 'qqq' ) ); } public function hookGetPreferences( $user, &$preferences ) { @@ -90,7 +95,11 @@ class ApiOptionsTest extends MediaWikiLangTestCase { 'default' => [], ]; - return true; + $preferences['testradio'] = [ + 'type' => 'radio', + 'options' => [ 'Option 1' => 'option1', 'Option 2' => 'option2' ], + 'section' => 'test', + ]; } /** @@ -106,6 +115,7 @@ class ApiOptionsTest extends MediaWikiLangTestCase { 'willBeNull' => 'registered', 'willBeEmpty' => 'registered', 'willBeHappy' => 'registered', + 'testradio' => 'registered', 'testmultiselect-opt1' => 'registered-multiselect', 'testmultiselect-opt2' => 'registered-multiselect', 'testmultiselect-opt3' => 'registered-multiselect', @@ -243,65 +253,6 @@ class ApiOptionsTest extends MediaWikiLangTestCase { $this->assertEquals( self::$Success, $response ); } - public function testOptionWithValue() { - $this->mUserMock->expects( $this->never() ) - ->method( 'resetOptions' ); - - $this->mUserMock->expects( $this->once() ) - ->method( 'setOption' ) - ->with( $this->equalTo( 'name' ), $this->equalTo( 'value' ) ); - - $this->mUserMock->expects( $this->once() ) - ->method( 'saveSettings' ); - - $request = $this->getSampleRequest( [ 'optionname' => 'name', 'optionvalue' => 'value' ] ); - - $response = $this->executeQuery( $request ); - - $this->assertEquals( self::$Success, $response ); - } - - public function testOptionResetValue() { - $this->mUserMock->expects( $this->never() ) - ->method( 'resetOptions' ); - - $this->mUserMock->expects( $this->once() ) - ->method( 'setOption' ) - ->with( $this->equalTo( 'name' ), $this->identicalTo( null ) ); - - $this->mUserMock->expects( $this->once() ) - ->method( 'saveSettings' ); - - $request = $this->getSampleRequest( [ 'optionname' => 'name' ] ); - $response = $this->executeQuery( $request ); - - $this->assertEquals( self::$Success, $response ); - } - - public function testChange() { - $this->mUserMock->expects( $this->never() ) - ->method( 'resetOptions' ); - - $this->mUserMock->expects( $this->exactly( 3 ) ) - ->method( 'setOption' ) - ->withConsecutive( - [ $this->equalTo( 'willBeNull' ), $this->identicalTo( null ) ], - [ $this->equalTo( 'willBeEmpty' ), $this->equalTo( '' ) ], - [ $this->equalTo( 'willBeHappy' ), $this->equalTo( 'Happy' ) ] - ); - - $this->mUserMock->expects( $this->once() ) - ->method( 'saveSettings' ); - - $request = $this->getSampleRequest( [ - 'change' => 'willBeNull|willBeEmpty=|willBeHappy=Happy' - ] ); - - $response = $this->executeQuery( $request ); - - $this->assertEquals( self::$Success, $response ); - } - public function testResetChangeOption() { $this->mUserMock->expects( $this->once() ) ->method( 'resetOptions' ); @@ -328,95 +279,121 @@ class ApiOptionsTest extends MediaWikiLangTestCase { $this->assertEquals( self::$Success, $response ); } - public function testMultiSelect() { + /** + * @dataProvider provideOptionManupulation + * @param array $params + * @param array $setOptions + * @param array|null $result + */ + public function testOptionManupulation( array $params, array $setOptions, array $result = null, + $message = '' + ) { $this->mUserMock->expects( $this->never() ) ->method( 'resetOptions' ); - $this->mUserMock->expects( $this->exactly( 4 ) ) + $this->mUserMock->expects( $this->exactly( count( $setOptions ) ) ) ->method( 'setOption' ) - ->withConsecutive( - [ $this->equalTo( 'testmultiselect-opt1' ), $this->identicalTo( true ) ], - [ $this->equalTo( 'testmultiselect-opt2' ), $this->identicalTo( null ) ], - [ $this->equalTo( 'testmultiselect-opt3' ), $this->identicalTo( false ) ], - [ $this->equalTo( 'testmultiselect-opt4' ), $this->identicalTo( false ) ] - ); - - $this->mUserMock->expects( $this->once() ) - ->method( 'saveSettings' ); - - $request = $this->getSampleRequest( [ - 'change' => 'testmultiselect-opt1=1|testmultiselect-opt2|' - . 'testmultiselect-opt3=|testmultiselect-opt4=0' - ] ); - - $response = $this->executeQuery( $request ); - - $this->assertEquals( self::$Success, $response ); - } - - public function testSpecialOption() { - $this->mUserMock->expects( $this->never() ) - ->method( 'resetOptions' ); - - $this->mUserMock->expects( $this->never() ) - ->method( 'saveSettings' ); - - $request = $this->getSampleRequest( [ - 'change' => 'special=1' - ] ); - - $response = $this->executeQuery( $request ); - - $this->assertEquals( [ - 'options' => 'success', - 'warnings' => [ - 'options' => [ - 'warnings' => "Validation error for \"special\": cannot be set by this module." - ] - ] - ], $response ); - } - - public function testUnknownOption() { - $this->mUserMock->expects( $this->never() ) - ->method( 'resetOptions' ); - - $this->mUserMock->expects( $this->never() ) - ->method( 'saveSettings' ); - - $request = $this->getSampleRequest( [ - 'change' => 'unknownOption=1' - ] ); + ->withConsecutive( ...$setOptions ); + + if ( $setOptions ) { + $this->mUserMock->expects( $this->once() ) + ->method( 'saveSettings' ); + } else { + $this->mUserMock->expects( $this->never() ) + ->method( 'saveSettings' ); + } + $request = $this->getSampleRequest( $params ); $response = $this->executeQuery( $request ); - $this->assertEquals( [ - 'options' => 'success', - 'warnings' => [ - 'options' => [ - 'warnings' => "Validation error for \"unknownOption\": not a valid preference." - ] - ] - ], $response ); + if ( !$result ) { + $result = self::$Success; + } + $this->assertEquals( $result, $response, $message ); } - public function testUserjsOption() { - $this->mUserMock->expects( $this->never() ) - ->method( 'resetOptions' ); - - $this->mUserMock->expects( $this->once() ) - ->method( 'setOption' ) - ->with( $this->equalTo( 'userjs-option' ), $this->equalTo( '1' ) ); - - $this->mUserMock->expects( $this->once() ) - ->method( 'saveSettings' ); - - $request = $this->getSampleRequest( [ - 'change' => 'userjs-option=1' - ] ); - - $response = $this->executeQuery( $request ); - - $this->assertEquals( self::$Success, $response ); + public function provideOptionManupulation() { + return [ + [ + [ 'change' => 'userjs-option=1' ], + [ [ 'userjs-option', '1' ] ], + null, + 'Setting userjs options', + ], + [ + [ 'change' => 'willBeNull|willBeEmpty=|willBeHappy=Happy' ], + [ + [ 'willBeNull', null ], + [ 'willBeEmpty', '' ], + [ 'willBeHappy', 'Happy' ], + ], + null, + 'Basic option setting', + ], + [ + [ 'change' => 'testradio=option2' ], + [ [ 'testradio', 'option2' ] ], + null, + 'Changing radio options', + ], + [ + [ 'change' => 'testradio' ], + [ [ 'testradio', null ] ], + null, + 'Resetting radio options', + ], + [ + [ 'change' => 'unknownOption=1' ], + [], + [ + 'options' => 'success', + 'warnings' => [ + 'options' => [ + 'warnings' => "Validation error for \"unknownOption\": not a valid preference." + ], + ], + ], + 'Unrecognized options should be rejected', + ], + [ + [ 'change' => 'special=1' ], + [], + [ + 'options' => 'success', + 'warnings' => [ + 'options' => [ + 'warnings' => "Validation error for \"special\": cannot be set by this module." + ] + ] + ], + 'Refuse setting special options', + ], + [ + [ + 'change' => 'testmultiselect-opt1=1|testmultiselect-opt2|' + . 'testmultiselect-opt3=|testmultiselect-opt4=0' + ], + [ + [ 'testmultiselect-opt1', true ], + [ 'testmultiselect-opt2', null ], + [ 'testmultiselect-opt3', false ], + [ 'testmultiselect-opt4', false ], + ], + null, + 'Setting multiselect options', + ], + [ + [ 'optionname' => 'name', 'optionvalue' => 'value' ], + [ [ 'name', 'value' ] ], + null, + 'Setting options via optionname/optionvalue' + ], + [ + [ 'optionname' => 'name' ], + [ [ 'name', null ] ], + null, + 'Resetting options via optionname without optionvalue', + ], + ]; } } -- 2.20.1