wikimedia/mediawiki-skins-Vector
1
0
Fork 0
mirror of https://gerrit.wikimedia.org/r/mediawiki/skins/Vector.git synced 2024-11-24 07:43:47 +00:00
Code Issues Packages Projects Releases Wiki Activity

[fix] "Existing account only" skin version config

Browse source 
de76ab5 added the config,
`$wgVectorDefaultSkinVersionForExistingAccounts`. Its usage in
`Hooks::onUserGetDefaultOptions()` was invoked not only for existing
accounts but anonymous users _as well._  This is a bug, due to my own
misconceptions about the hook, that went against both the config's name
and its documentation.

Unfortunately, user sessions are unavailable in
`Hooks::onUserGetDefaultOptions()` so it does not seem to be possible to
determine whether the active user is an anonymous or existing account.
This patch drops the hook and centralizes all version determination
logic in SkinVersionLookup::getVersion(). SkinVersionLookup requires a
the active User object and can make the anonymous / existing account
determination by checking login state.

The issued was identified while responding to review feedback given by
@polishdeveloper / @pmiazga in
I52d80942b4270c008d4e45050589ed9220255a50.

Bug: T251415
Change-Id: I7982b4c34283ba81d0232ee6f501c44cf0a74b98
This commit is contained in:
Stephen Niedzielski 2020-04-28 15:02:15 -06:00 committed by Jdlrobson
parent 28c286526f
commit 709772fa12
8 changed files with 350 additions and 164 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

49
includes/FeatureManagement/Requirements/LatestSkinVersionRequirement.php
View file

@ -22,11 +22,9 @@
namespace Vector\FeatureManagement\Requirements;
use Config;
use User;
use Vector\Constants;
use Vector\FeatureManagement\Requirement;
use WebRequest;
use Vector\SkinVersionLookup;
/**
* Retrieve the skin version for the request and compare it with `Constants::SKIN_VERSION_LATEST`.
@ -58,29 +56,17 @@ use WebRequest;
final class LatestSkinVersionRequirement implements Requirement {
/**
* @var WebRequest
* @var SkinVersionLookup
*/
private $request;
/**
* @var User
*/
private $user;
/**
* @var Config
*/
private $config;
private $skinVersionLookup;
/**
* This constructor accepts all dependencies needed to obtain the skin version.
*
* @param WebRequest $request
* @param User $user
* @param Config $config
* @param SkinVersionLookup $skinVersionLookup
*/
public function __construct( WebRequest $request, User $user, Config $config ) {
$this->request = $request;
$this->user = $user;
$this->config = $config;
public function __construct( SkinVersionLookup $skinVersionLookup ) {
$this->skinVersionLookup = $skinVersionLookup;
}
/**
@ -95,27 +81,6 @@ final class LatestSkinVersionRequirement implements Requirement {
* @throws \ConfigException
*/
public function isMet() : bool {
return $this->getVersion() === Constants::SKIN_VERSION_LATEST;
}
/**
* The skin version as a string. E.g., `Constants::SKIN_VERSION_LATEST`,
* `Constants::SKIN_VERSION_LATEST`, or maybe 'beta'. Note: it's likely someone will put
* arbitrary strings in the query parameter which means this function returns those strings as
* is.
*
* @return string
* @throws \ConfigException
*/
private function getVersion() : string {
// Obtain the skin version from the `useskinversion` URL query parameter override, the user
// preference, or the configured default.
return (string)$this->request->getVal(
Constants::QUERY_PARAM_SKIN_VERSION,
$this->user->getOption(
Constants::PREF_KEY_SKIN_VERSION,
$this->config->get( Constants::CONFIG_KEY_DEFAULT_SKIN_VERSION )
)
);
return $this->skinVersionLookup->getVersion() === Constants::SKIN_VERSION_LATEST;
}
}

31
includes/Hooks.php
View file

@ -5,6 +5,7 @@ namespace Vector;
use HTMLForm;
use MediaWiki\MediaWikiServices;
use OutputPage;
use RequestContext;
use SkinTemplate;
use SkinVector;
use User;
@ -44,6 +45,10 @@ class Hooks {
return;
}
$skinVersionLookup = new SkinVersionLookup(
RequestContext::getMain()->getRequest(), $user, self::getServiceConfig()
);
// Preferences to add.
$vectorPrefs = [
Constants::PREF_KEY_SKIN_VERSION => [
@ -56,10 +61,7 @@ class Hooks {
// indicates that a prefs-skin-prefs string will be provided.
'section' => 'rendering/skin/skin-prefs',
// Convert the preference string to a boolean presentation.
'default' =>
$user->getOption( Constants::PREF_KEY_SKIN_VERSION ) === Constants::SKIN_VERSION_LATEST ?
'0' :
'1',
'default' => $skinVersionLookup->isLegacy() ? '1' : '0',
// Only show this section when the Vector skin is checked. The JavaScript client also uses
// this state to determine whether to show or hide the whole section.
'hide-if' => [ '!==', 'wpskin', Constants::SKIN_NAME ]
@ -119,16 +121,6 @@ class Hooks {
}
}
/**
* Called on each pageview to populate preference defaults for existing users.
*
* @param array &$defaultPrefs
*/
public static function onUserGetDefaultOptions( array &$defaultPrefs ) {
$default = self::getConfig( Constants::CONFIG_KEY_DEFAULT_SKIN_VERSION_FOR_EXISTING_ACCOUNTS );
$defaultPrefs[ Constants::PREF_KEY_SKIN_VERSION ] = $default;
}
/**
* Called one time when initializing a users preferences for a newly created account.
*
@ -150,8 +142,13 @@ class Hooks {
* @throws \ConfigException
*/
private static function getConfig( $name ) {
/* @var Config */ $service =
MediaWikiServices::getInstance()->getService( Constants::SERVICE_CONFIG );
return $service->get( $name );
return self::getServiceConfig()->get( $name );
}
/**
* @return \Config
*/
private static function getServiceConfig() {
return MediaWikiServices::getInstance()->getService( Constants::SERVICE_CONFIG );
}
}

9
includes/ServiceWiring.php
View file

@ -27,6 +27,7 @@ use Vector\Constants;
use Vector\FeatureManagement\FeatureManager;
use Vector\FeatureManagement\Requirements\DynamicConfigRequirement;
use Vector\FeatureManagement\Requirements\LatestSkinVersionRequirement;
use Vector\SkinVersionLookup;
return [
Constants::SERVICE_CONFIG => function ( MediaWikiServices $services ) {
@ -48,9 +49,11 @@ return [
$featureManager->registerComplexRequirement(
new LatestSkinVersionRequirement(
$context->getRequest(),
$context->getUser(),
$context->getConfig()
new SkinVersionLookup(
$context->getRequest(),
$context->getUser(),
$services->getService( Constants::SERVICE_CONFIG )
)
)
);

119
includes/SkinVersionLookup.php Normal file
View file

@ -0,0 +1,119 @@
<?php
/**
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License along
* with this program; if not, write to the Free Software Foundation, Inc.,
* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
* http://www.gnu.org/copyleft/gpl.html
*
* @file
* @since 1.35
*/
namespace Vector;
use Config;
use User;
use WebRequest;
/**
* Given initial dependencies, retrieve the current skin version. This class does no parsing, just
* the lookup.
*
* Skin version is evaluated in the following order:
*
* - useskinversion URL query parameter override. See readme.
*
* - User preference. The User object for new accounts is updated (persisted as a user preference)
* by hook according to VectorDefaultSkinVersionForNewAccounts. See Hooks and skin.json. The user
* may then change the preference at will.
*
* - Site configuration default. The default is controlled by VectorDefaultSkinVersion and
* VectorDefaultSkinVersionForExistingAccounts based on login state. The former is used
* for anonymous users and as a fallback configuration, the latter is logged in users (existing
* accounts). See skin.json.
*
* @unstable
*
* @package Vector
* @internal
*/
final class SkinVersionLookup {
/**
* @var WebRequest
*/
private $request;
/**
* @var User
*/
private $user;
/**
* @var Config
*/
private $config;
/**
* This constructor accepts all dependencies needed to obtain the skin version. The dependencies
* are lazily evaluated, not cached, meaning they always return the current results.
*
* @param WebRequest $request
* @param User $user
* @param Config $config
*/
public function __construct( WebRequest $request, User $user, Config $config ) {
$this->request = $request;
$this->user = $user;
$this->config = $config;
}
/**
* Whether or not the legacy skin is being used.
*
* @return bool
* @throws \ConfigException
*/
public function isLegacy(): bool {
return $this->getVersion() === Constants::SKIN_VERSION_LEGACY;
}
/**
* The skin version as a string. E.g., `Constants::SKIN_VERSION_LATEST`,
* `Constants::SKIN_VERSION_LATEST`, or maybe 'beta'. Note: it's likely someone will put arbitrary
* strings in the query parameter which means this function returns those strings as is.
*
* @return string
* @throws \ConfigException
*/
public function getVersion(): string {
// Obtain the skin version from the 1) `useskinversion` URL query parameter override, 2) the
// user preference, 3) the configured default for logged in users, 4) or the site default.
//
// The latter two configurations cannot be set by `Hooks::onUserGetDefaultOptions()` as user
// sessions are unavailable at that time so it's not possible to determine whether the
// preference is for a logged in user or an anonymous user. Since new users are known to have
// had their user preferences initialized in `Hooks::onLocalUserCreated()`, that means all
// subsequent requests to `User->getOption()` that do not have a preference set are either
// existing accounts or anonymous users. Login state makes the distinction.
return (string)$this->request->getVal(
Constants::QUERY_PARAM_SKIN_VERSION,
$this->user->getOption(
Constants::PREF_KEY_SKIN_VERSION,
$this->config->get(
$this->user->isLoggedIn()
? Constants::CONFIG_KEY_DEFAULT_SKIN_VERSION_FOR_EXISTING_ACCOUNTS
: Constants::CONFIG_KEY_DEFAULT_SKIN_VERSION
)
)
);
}
}

7
skin.json
View file

@ -35,7 +35,6 @@
"BeforePageDisplayMobile": "Vector\\Hooks::onBeforePageDisplayMobile",
"GetPreferences": "Vector\\Hooks::onGetPreferences",
"PreferencesFormPreSave": "Vector\\Hooks::onPreferencesFormPreSave",
"UserGetDefaultOptions": "Vector\\Hooks::onUserGetDefaultOptions",
"LocalUserCreated": "Vector\\Hooks::onLocalUserCreated"
},
"@note": "When modifying skins.vector.styles definition, make sure the installer still works",
@ -122,15 +121,15 @@
},
"VectorDefaultSkinVersion": {
"value": "2",
"description": "@var string:['2'|'1'] The version ('2' for latest, '1' for legacy) of the Vector skin to use for anonymous users and as a fallback."
"description": "@var string:['2'|'1'] The version ('2' for latest, '1' for legacy) of the Vector skin to use for anonymous users and as a fallback. The value is _not_ persisted."
},
"VectorDefaultSkinVersionForExistingAccounts": {
"value": "2",
"description": "@var string:['2'|'1'] The version ('2' for latest, '1' for legacy) of the Vector skin to use when an existing user has not specified a preference. This configuration is not used for new accounts (see VectorDefaultSkinVersionForNewAccounts) and is impermanent. In the future, this field may contains versions such as \"beta\" which when specified and the BetaFeatures extension is installed, and the user is enrolled, the latest version is used otherwise legacy."
"description": "@var string:['2'|'1'] The version ('2' for latest, '1' for legacy) of the Vector skin to use when an existing user has not specified a preference. This configuration is not used for new accounts (see VectorDefaultSkinVersionForNewAccounts) and is impermanent. In the future, this field may contains versions such as \"beta\" which when specified and the BetaFeatures extension is installed, and the user is enrolled, the latest version is used otherwise legacy. The value is _not_ persisted."
},
"VectorDefaultSkinVersionForNewAccounts": {
"value": "2",
"description": "@var string:['2'|'1'] The version ('2' for latest, '1' for legacy) of the Vector skin to **set** for newly created user accounts. This configuration is not used for preexisting accounts (see VectorDefaultSkinVersion) and only ever executed once at new account creation time."
"description": "@var string:['2'|'1'] The version ('2' for latest, '1' for legacy) of the Vector skin to **set** for newly created user accounts. **The value is persisted as a user preference.** This configuration is not used for preexisting accounts (see VectorDefaultSkinVersion) and only ever executed once at new account creation time."
}
},
"ServiceWiringFiles": [

69
tests/phpunit/integration/FeatureManagement/Requirements/LatestSkinVersionRequirementTest.php
View file

@ -20,6 +20,7 @@
*/
use Vector\FeatureManagement\Requirements\LatestSkinVersionRequirement;
use Vector\SkinVersionLookup;
/**
* @group Vector
@ -29,73 +30,27 @@ class LatestSkinVersionRequirementTest extends \MediaWikiTestCase {
/**
* @covers ::isMet
* @covers ::getVersion
*/
public function testRequest() {
$request = $this->getMockBuilder( \WebRequest::class )->getMock();
$request
->expects( $this->exactly( 1 ) )
->method( 'getVal' )
->with( $this->anything(), $this->equalTo( '1' ) )
->willReturn( 'beta' );
public function testUnmet() {
$config = new HashConfig( [ 'VectorDefaultSkinVersionForExistingAccounts' => '1' ] );
$user = $this->createMock( \User::class );
$user
->expects( $this->exactly( 1 ) )
->method( 'getOption' )
->with( $this->anything(), $this->equalTo( '2' ) )
->willReturn( '1' );
$config = new \HashConfig( [ 'VectorDefaultSkinVersion' => '2' ] );
$requirement = new LatestSkinVersionRequirement( $request, $user, $config );
$this->assertFalse(
$requirement->isMet(),
'WebRequest::getVal takes precedence. "beta" isn\'t considered latest.'
$requirement = new LatestSkinVersionRequirement(
new SkinVersionLookup( new WebRequest(), $this->getTestUser()->getUser(), $config )
);
$this->assertFalse( $requirement->isMet(), '"1" isn\'t considered latest.' );
}
/**
* @covers ::isMet
* @covers ::getVersion
*/
public function testUserPreference() {
$request = new WebRequest();
public function testMet() {
$config = new HashConfig( [ 'VectorDefaultSkinVersionForExistingAccounts' => '2' ] );
$user = $this->createMock( \User::class );
$user
->expects( $this->exactly( 1 ) )
->method( 'getOption' )
->with( $this->anything(), $this->equalTo( '2' ) )
->willReturn( '1' );
$config = new \HashConfig( [ 'VectorDefaultSkinVersion' => '2' ] );
$requirement = new LatestSkinVersionRequirement( $request, $user, $config );
$this->assertFalse(
$requirement->isMet(),
'User preference takes second place. "1" (legacy) isn\'t considered latest.'
$requirement = new LatestSkinVersionRequirement(
new SkinVersionLookup( new WebRequest(), $this->getTestUser()->getUser(), $config )
);
}
/**
* @covers ::isMet
* @covers ::getVersion
*/
public function testConfig() {
$request = new WebRequest();
$user = \MediaWikiTestCase::getTestUser()
->getUser();
$config = new HashConfig( [ 'VectorDefaultSkinVersion' => '2' ] );
$requirement = new LatestSkinVersionRequirement( $request, $user, $config );
$this->assertTrue(
$requirement->isMet(),
'Config takes third place. "2" is considered latest.'
);
$this->assertTrue( $requirement->isMet(), '"2" is considered latest.' );
}
}

184
tests/phpunit/integration/SkinVersionLookupTest.php Normal file
View file

@ -0,0 +1,184 @@
<?php
/**
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License along
* with this program; if not, write to the Free Software Foundation, Inc.,
* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
* http://www.gnu.org/copyleft/gpl.html
*
* @file
* @since 1.35
*/
use Vector\SkinVersionLookup;
/**
* @group Vector
* @coversDefaultClass \Vector\SkinVersionLookup
*/
class SkinVersionLookupTest extends \MediaWikiTestCase {
/**
* @covers ::isLegacy
* @covers ::getVersion
*/
public function testRequest() {
$request = $this->getMockBuilder( \WebRequest::class )->getMock();
$request
->method( 'getVal' )
->with( $this->anything(), $this->equalTo( 'beta' ) )
->willReturn( 'alpha' );
$user = $this->createMock( \User::class );
$user
->method( 'isLoggedIn' )
->willReturn( false );
$user
->method( 'getOption' )
->with( $this->anything(), $this->equalTo( '2' ) )
->willReturn( 'beta' );
$config = new HashConfig( [
'VectorDefaultSkinVersion' => '2',
'VectorDefaultSkinVersionForExistingAccounts' => '1'
] );
$skinVersionLookup = new SkinVersionLookup( $request, $user, $config );
$this->assertSame(
$skinVersionLookup->getVersion(),
'alpha',
'Query parameter is the first priority.'
);
$this->assertSame(
$skinVersionLookup->isLegacy(),
false,
'Version is non-Legacy.'
);
}
/**
* @covers ::getVersion
* @covers ::isLegacy
*/
public function testUserPreference() {
$request = $this->getMockBuilder( \WebRequest::class )->getMock();
$request
->method( 'getVal' )
->with( $this->anything(), $this->equalTo( 'beta' ) )
->willReturn( 'beta' );
$user = $this->createMock( \User::class );
$user
->method( 'isLoggedIn' )
->willReturn( false );
$user
->method( 'getOption' )
->with( $this->anything(), $this->equalTo( '2' ) )
->willReturn( 'beta' );
$config = new HashConfig( [
'VectorDefaultSkinVersion' => '2',
'VectorDefaultSkinVersionForExistingAccounts' => '1'
] );
$skinVersionLookup = new SkinVersionLookup( $request, $user, $config );
$this->assertSame(
$skinVersionLookup->getVersion(),
'beta',
'User preference is the second priority.'
);
$this->assertSame(
$skinVersionLookup->isLegacy(),
false,
'Version is non-Legacy.'
);
}
/**
* @covers ::getVersion
* @covers ::isLegacy
*/
public function testConfigLoggedIn() {
$request = $this->getMockBuilder( \WebRequest::class )->getMock();
$request
->method( 'getVal' )
->with( $this->anything(), $this->equalTo( '1' ) )
->willReturn( '1' );
$user = $this->createMock( \User::class );
$user
->method( 'isLoggedIn' )
->willReturn( true );
$user
->method( 'getOption' )
->with( $this->anything(), $this->equalTo( '1' ) )
->willReturn( '1' );
$config = new HashConfig( [
'VectorDefaultSkinVersion' => '2',
'VectorDefaultSkinVersionForExistingAccounts' => '1'
] );
$skinVersionLookup = new SkinVersionLookup( $request, $user, $config );
$this->assertSame(
$skinVersionLookup->getVersion(),
'1',
'Config is the third priority and distinguishes logged in users from anonymous users.'
);
$this->assertSame(
$skinVersionLookup->isLegacy(),
true,
'Version is Legacy.'
);
}
/**
* @covers ::getVersion
* @covers ::isLegacy
*/
public function testConfigAnon() {
$request = $this->getMockBuilder( \WebRequest::class )->getMock();
$request
->method( 'getVal' )
->with( $this->anything(), $this->equalTo( '2' ) )
->willReturn( '2' );
$user = $this->createMock( \User::class );
$user
->method( 'isLoggedIn' )
->willReturn( false );
$user
->method( 'getOption' )
->with( $this->anything(), $this->equalTo( '2' ) )
->willReturn( '2' );
$config = new HashConfig( [
'VectorDefaultSkinVersion' => '2',
'VectorDefaultSkinVersionForExistingAccounts' => '1'
] );
$skinVersionLookup = new SkinVersionLookup( $request, $user, $config );
$this->assertSame(
$skinVersionLookup->getVersion(),
'2',
'Config is the third priority and distinguishes anonymous users from logged in users.'
);
$this->assertSame(
$skinVersionLookup->isLegacy(),
false,
'Version is non-Legacy.'
);
}
}

46
tests/phpunit/integration/VectorHooksTest.php
View file

@ -35,7 +35,7 @@ class VectorHooksTest extends \MediaWikiTestCase {
public function testOnGetPreferencesShowPreferencesEnabledSkinSectionFoundLegacy() {
$config = new HashConfig( [
'VectorShowSkinPreferences' => true,
// Required by test user's onUserGetDefaultOptions() hook but unused for this test.
// '1' is Legacy.
'VectorDefaultSkinVersionForExistingAccounts' => '1',
] );
$this->setService( 'Vector.Config', $config );
@ -72,7 +72,7 @@ class VectorHooksTest extends \MediaWikiTestCase {
public function testOnGetPreferencesShowPreferencesEnabledSkinSectionMissingLegacy() {
$config = new HashConfig( [
'VectorShowSkinPreferences' => true,
// Required by test user's onUserGetDefaultOptions() hook but unused for this test.
// '1' is Legacy.
'VectorDefaultSkinVersionForExistingAccounts' => '1',
] );
$this->setService( 'Vector.Config', $config );
@ -107,8 +107,8 @@ class VectorHooksTest extends \MediaWikiTestCase {
public function testOnGetPreferencesShowPreferencesEnabledSkinSectionMissingLatest() {
$config = new HashConfig( [
'VectorShowSkinPreferences' => true,
// Required by test user's onUserGetDefaultOptions() hook but unused for this test.
'VectorDefaultSkinVersionForExistingAccounts' => '1',
// '2' is latest.
'VectorDefaultSkinVersionForExistingAccounts' => '2',
] );
$this->setService( 'Vector.Config', $config );
@ -116,13 +116,7 @@ class VectorHooksTest extends \MediaWikiTestCase {
'foo' => [],
'bar' => [],
];
$user = $this->createMock( \User::class );
$user->expects( $this->once() )
->method( 'getOption' )
->with( 'VectorSkinVersion' )
// '2' is latest.
->will( $this->returnValue( '2' ) );
Hooks::onGetPreferences( $user, $prefs );
Hooks::onGetPreferences( $this->getTestUser()->getUser(), $prefs );
$this->assertSame(
$prefs,
[
@ -240,36 +234,6 @@ class VectorHooksTest extends \MediaWikiTestCase {
Hooks::onPreferencesFormPreSave( $formData, $form, $user, $result, $oldPreferences );
}
/**
* @covers ::onUserGetDefaultOptions
*/
public function testOnUserGetDefaultOptionsLegacy() {
$config = new HashConfig( [
// '1' is Legacy.
'VectorDefaultSkinVersionForExistingAccounts' => '1',
] );
$this->setService( 'Vector.Config', $config );
$prefs = [];
Hooks::onUserGetDefaultOptions( $prefs );
$this->assertSame( $prefs, [ 'VectorSkinVersion' => '1' ], 'Version is Legacy.' );
}
/**
* @covers ::onUserGetDefaultOptions
*/
public function testOnUserGetDefaultOptionsLatest() {
$config = new HashConfig( [
// '2' is latest.
'VectorDefaultSkinVersionForExistingAccounts' => '2',
] );
$this->setService( 'Vector.Config', $config );
$prefs = [];
Hooks::onUserGetDefaultOptions( $prefs );
$this->assertSame( $prefs, [ 'VectorSkinVersion' => '2' ], 'Version is latest.' );
}
/**
* @covers ::onLocalUserCreated
*/