resourceloader: Remove use of $.isPlainObject() from mw.Map#set()

Look for typeof 'object' instead. The set() method has good test
coverage which uncovered a few cases that were previously only
working implicitly due to isPlainObject.

* Test case `mw.config.set( [], 'value' )`
  This is invalid because `key` must be a string. This was previously
  rejected because while array is an object, it isn't a plain object.
  This commit intends to remove this distinction and allow any object
  to be used with the set(Object) signature. However, we should still
  reject set(non-string, string) no matter what kind of object is passed.

  Changing from isPlainObject to 'is an object' made this case
  wrongly pass instead of fail (because arrays are objects). Fix that,
  as well as any other case of non-string as key, by making the code
  explicitly reject non-string keys when two arguments are given.

  Added test case for `mw.config.set( {}, value ) === false` that did
  not pass without the changes in src/.

* Missing `> 1` check in global #set(). The check for arguments.length
  was asserting truthiness (non-zero) rather than >1 (2 or more).
  This was causing things like `mw.config.set('key')` to throw
  "ReferenceError: value not defined" when the underlying mw.Map
  is global. The normal #set() method for maps other than mw.config,
  was already fine.

  Fixed a bug in mediawiki.language.init that was revealed by this.
  The bug was not happening previously because when an object
  was passed, the second parameter was ignored.

Bug: T192623
Change-Id: Ib53647b324fe3d31e3389ed9aa14a08280d9c830

diff --git a/resources/src/mediawiki.language/mediawiki.language.init.js b/resources/src/mediawiki.language/mediawiki.language.init.js
index e5cf26e..077473b 100644 (file)
--- a/resources/src/mediawiki.language/mediawiki.language.init.js
+++ b/resources/src/mediawiki.language/mediawiki.language.init.js
@@ -77,7 +77,11 @@
                        if ( !( langData[ langCode ] instanceof mw.Map ) ) {
                                langData[ langCode ] = new mw.Map();
                        }
-                       langData[ langCode ].set( dataKey, value );
+                       if ( arguments.length > 2 ) {
+                               langData[ langCode ].set( dataKey, value );
+                       } else {
+                               langData[ langCode ].set( dataKey );
+                       }
                }
        };
 

diff --git a/resources/src/mediawiki/mediawiki.js b/resources/src/mediawiki/mediawiki.js
index b1e1da3..fbe8af2 100644 (file)
--- a/resources/src/mediawiki/mediawiki.js
+++ b/resources/src/mediawiki/mediawiki.js
@@ -90,15 +90,17 @@
                        // Override #set to also set the global variable
                        this.set = function ( selection, value ) {
                                var s;
-
-                               if ( $.isPlainObject( selection ) ) {
-                                       for ( s in selection ) {
-                                               setGlobalMapValue( this, s, selection[ s ] );
+                               if ( arguments.length > 1 ) {
+                                       if ( typeof selection !== 'string' ) {
+                                               return false;
                                        }
+                                       setGlobalMapValue( this, selection, value );
                                        return true;
                                }
-                               if ( typeof selection === 'string' && arguments.length ) {
-                                       setGlobalMapValue( this, selection, value );
+                               if ( typeof selection === 'object' ) {
+                                       for ( s in selection ) {
+                                               setGlobalMapValue( this, s, selection[ s ] );
+                                       }
                                        return true;
                                }
                                return false;
@@ -183,15 +185,18 @@
                 */
                set: function ( selection, value ) {
                        var s;
-
-                       if ( $.isPlainObject( selection ) ) {
-                               for ( s in selection ) {
-                                       this.values[ s ] = selection[ s ];
+                       // Use `arguments.length` because `undefined` is also a valid value.
+                       if ( arguments.length > 1 ) {
+                               if ( typeof selection !== 'string' ) {
+                                       return false;
                                }
+                               this.values[ selection ] = value;
                                return true;
                        }
-                       if ( typeof selection === 'string' && arguments.length > 1 ) {
-                               this.values[ selection ] = value;
+                       if ( typeof selection === 'object' ) {
+                               for ( s in selection ) {
+                                       this.values[ s ] = selection[ s ];
+                               }
                                return true;
                        }
                        return false;

diff --git a/tests/qunit/suites/resources/mediawiki/mediawiki.test.js b/tests/qunit/suites/resources/mediawiki/mediawiki.test.js
index 119222a..75dc665 100644 (file)
--- a/tests/qunit/suites/resources/mediawiki/mediawiki.test.js
+++ b/tests/qunit/suites/resources/mediawiki/mediawiki.test.js
@@ -113,6 +113,8 @@
                assert.strictEqual( conf.set( funky, 'Funky' ), false, 'Map.set returns boolean false if key was invalid (Function)' );
                assert.strictEqual( conf.set( arry, 'Arry' ), false, 'Map.set returns boolean false if key was invalid (Array)' );
                assert.strictEqual( conf.set( nummy, 'Nummy' ), false, 'Map.set returns boolean false if key was invalid (Number)' );
+               assert.strictEqual( conf.set( null, 'Null' ), false, 'Map.set returns false if key is invalid (null)' );
+               assert.strictEqual( conf.set( {}, 'Object' ), false, 'Map.set returns false if key is invalid (plain object)' );
 
                conf.set( String( nummy ), 'I used to be a number' );