From: Timo Tijhof Date: Sat, 4 Mar 2017 00:46:24 +0000 (-0800) Subject: resourceloader: Enforce strict string keys in mw.Map X-Git-Tag: 1.31.0-rc.0~3873^2 X-Git-Url: https://git.cyclocoop.org/%7B%24admin_url%7Dmembres/%7B%7B%20url_for%28%27vote%27%2C%20idvote=vote.voteid%29%20%7D%7D?a=commitdiff_plain;h=e696524c0082f4cf45efb92238206cb363bda56d;p=lhc%2Fweb%2Fwiklou.git resourceloader: Enforce strict string keys in mw.Map * No longer perform any key-to-string casting in get(), set(), or exists(). - Setting a non-string key is ignored. - Looking for a non-string key that looks like a string key that exists will not find it. Clean up: * Remove needless `selection=slice.call(selection)` in mw.Map#get Follows-up 77d71f743 (r75486). Not sure why this was here. * Inline logic for the multi-key retrieval instead of recursing. This also removes undocumented behaviour where a nested array could be passed to mw.config.get(), e.g. mw.config.get([['debug']]), which would produces a nested object. Change-Id: I84506930919e59ce16d96d6d623db5aae5f273e1 --- diff --git a/resources/src/mediawiki/mediawiki.js b/resources/src/mediawiki/mediawiki.js index 00b275fb5e..33f146b9d4 100644 --- a/resources/src/mediawiki/mediawiki.js +++ b/resources/src/mediawiki/mediawiki.js @@ -52,6 +52,7 @@ /* eslint-enable no-bitwise */ } + // StringSet = window.Set || ( function () { /** * @private @@ -64,7 +65,7 @@ this.set[ value ] = true; }; StringSet.prototype.has = function ( value ) { - return this.set.hasOwnProperty( value ); + return hasOwn.call( this.set, value ); }; return StringSet; }() ); @@ -133,22 +134,22 @@ * * @param {string|Array} [selection] Key or array of keys to retrieve values for. * @param {Mixed} [fallback=null] Value for keys that don't exist. - * @return {Mixed|Object| null} If selection was a string, returns the value, + * @return {Mixed|Object|null} If selection was a string, returns the value, * If selection was an array, returns an object of key/values. - * If no selection is passed, the internal container is returned. (Beware that, - * as is the default in JavaScript, the object is returned by reference.) + * If no selection is passed, a new object with all key/values is returned. */ get: function ( selection, fallback ) { var results, i; - // If we only do this in the `return` block, it'll fail for the - // call to get() from the mutli-selection block. fallback = arguments.length > 1 ? fallback : null; if ( $.isArray( selection ) ) { - selection = slice.call( selection ); results = {}; for ( i = 0; i < selection.length; i++ ) { - results[ selection[ i ] ] = this.get( selection[ i ], fallback ); + if ( typeof selection[ i ] === 'string' ) { + results[ selection[ i ] ] = hasOwn.call( this.values, selection[ i ] ) ? + this.values[ selection[ i ] ] : + fallback; + } } return results; } @@ -160,11 +161,15 @@ } if ( selection === undefined ) { - return this.values; + results = {}; + for ( i in this.values ) { + results[ i ] = this.values[ i ]; + } + return results; } // Invalid selection key - return null; + return fallback; }, /** diff --git a/tests/qunit/suites/resources/mediawiki/mediawiki.test.js b/tests/qunit/suites/resources/mediawiki/mediawiki.test.js index 697f32d521..119222a61e 100644 --- a/tests/qunit/suites/resources/mediawiki/mediawiki.test.js +++ b/tests/qunit/suites/resources/mediawiki/mediawiki.test.js @@ -114,16 +114,23 @@ 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.get( funky ), null, 'Map.get ruturns null if selection was invalid (Function)' ); - assert.strictEqual( conf.get( nummy ), null, 'Map.get ruturns null if selection was invalid (Number)' ); - conf.set( String( nummy ), 'I used to be a number' ); + assert.strictEqual( conf.get( funky ), null, 'Map.get returns null if selection was invalid (Function)' ); + assert.strictEqual( conf.get( nummy ), null, 'Map.get returns null if selection was invalid (Number)' ); + assert.propEqual( conf.get( [ nummy ] ), {}, 'Map.get returns null if selection was invalid (multiple)' ); + assert.strictEqual( conf.get( nummy, false ), false, 'Map.get returns custom fallback for invalid selection' ); + assert.strictEqual( conf.exists( 'doesNotExist' ), false, 'Map.exists where property does not exist' ); assert.strictEqual( conf.exists( 'undef' ), true, 'Map.exists where value is `undefined`' ); - assert.strictEqual( conf.exists( nummy ), false, 'Map.exists where key is invalid but looks like an existing key' ); + assert.strictEqual( conf.exists( [ 'undef', 'example' ] ), true, 'Map.exists with multiple keys (all existing)' ); + assert.strictEqual( conf.exists( [ 'example', 'doesNotExist' ] ), false, 'Map.exists with multiple keys (some non-existing)' ); + assert.strictEqual( conf.exists( [] ), true, 'Map.exists with no keys' ); + assert.strictEqual( conf.exists( nummy ), false, 'Map.exists with invalid key that looks like an existing key' ); + assert.strictEqual( conf.exists( [ nummy ] ), false, 'Map.exists with invalid key that looks like an existing key' ); // Multiple values at once + conf = new mw.Map(); someValues = { foo: 'bar', lorem: 'ipsum', @@ -140,9 +147,10 @@ notExist: null }, 'Map.get return includes keys that were not found as null values' ); - // Interacting with globals and accessing the values object - assert.strictEqual( conf.get(), conf.values, 'Map.get returns the entire values object by reference (if called without arguments)' ); + assert.propEqual( conf.values, someValues, 'Map.values is an internal object with all values (exposed for convenience)' ); + assert.propEqual( conf.get(), someValues, 'Map.get() returns an object with all values' ); + // Interacting with globals conf.set( 'globalMapChecker', 'Hi' ); assert.ok( ( 'globalMapChecker' in window ) === false, 'Map does not its store values in the window object by default' );