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

diff --git a/RELEASE-NOTES-1.32 b/RELEASE-NOTES-1.32
index d875017..6c200d9 100644 (file)
--- 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 fe7d10d..3ea827c 100644 (file)
--- 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 29c7dae..30ba1c1 100644 (file)
--- 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',
+                       ],
+               ];
        }
 }