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 a8fa1bb..cddcf54 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 ) { @@ -49,9 +50,11 @@ return [ $featureManager->registerRequirement( 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 */