From 709772fa129c46cae3134380c3e6c8a224cc5404 Mon Sep 17 00:00:00 2001 From: Stephen Niedzielski Date: Tue, 28 Apr 2020 15:02:15 -0600 Subject: [PATCH] [fix] "Existing account only" skin version config 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 --- .../LatestSkinVersionRequirement.php | 49 +---- includes/Hooks.php | 31 ++- includes/ServiceWiring.php | 9 +- includes/SkinVersionLookup.php | 119 +++++++++++ skin.json | 7 +- .../LatestSkinVersionRequirementTest.php | 69 ++----- .../integration/SkinVersionLookupTest.php | 184 ++++++++++++++++++ tests/phpunit/integration/VectorHooksTest.php | 46 +---- 8 files changed, 350 insertions(+), 164 deletions(-) create mode 100644 includes/SkinVersionLookup.php create mode 100644 tests/phpunit/integration/SkinVersionLookupTest.php diff --git a/includes/FeatureManagement/Requirements/LatestSkinVersionRequirement.php b/includes/FeatureManagement/Requirements/LatestSkinVersionRequirement.php index ba766c5..47554de 100644 --- a/includes/FeatureManagement/Requirements/LatestSkinVersionRequirement.php +++ b/includes/FeatureManagement/Requirements/LatestSkinVersionRequirement.php @@ -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; } } diff --git a/includes/Hooks.php b/includes/Hooks.php index cb2cfce..a1233be 100644 --- a/includes/Hooks.php +++ b/includes/Hooks.php @@ -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 ); } } diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index 7178974..c0771e4 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -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 ) + ) ) ); diff --git a/includes/SkinVersionLookup.php b/includes/SkinVersionLookup.php new file mode 100644 index 0000000..2a9b8d6 --- /dev/null +++ b/includes/SkinVersionLookup.php @@ -0,0 +1,119 @@ +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 + ) + ) + ); + } +} diff --git a/skin.json b/skin.json index c4a6710..d84f3cc 100644 --- a/skin.json +++ b/skin.json @@ -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": [ diff --git a/tests/phpunit/integration/FeatureManagement/Requirements/LatestSkinVersionRequirementTest.php b/tests/phpunit/integration/FeatureManagement/Requirements/LatestSkinVersionRequirementTest.php index dce38e9..64f4bdd 100644 --- a/tests/phpunit/integration/FeatureManagement/Requirements/LatestSkinVersionRequirementTest.php +++ b/tests/phpunit/integration/FeatureManagement/Requirements/LatestSkinVersionRequirementTest.php @@ -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.' ); } } diff --git a/tests/phpunit/integration/SkinVersionLookupTest.php b/tests/phpunit/integration/SkinVersionLookupTest.php new file mode 100644 index 0000000..4ba080a --- /dev/null +++ b/tests/phpunit/integration/SkinVersionLookupTest.php @@ -0,0 +1,184 @@ +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.' + ); + } +} diff --git a/tests/phpunit/integration/VectorHooksTest.php b/tests/phpunit/integration/VectorHooksTest.php index d2aaaba..ee7ebfb 100644 --- a/tests/phpunit/integration/VectorHooksTest.php +++ b/tests/phpunit/integration/VectorHooksTest.php @@ -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 */