From eca9fcbf7923a6d6cc515d7ebfe2ad8200ed98e1 Mon Sep 17 00:00:00 2001 From: Jon Robson Date: Fri, 1 Apr 2022 08:53:03 -0700 Subject: [PATCH] Drop the LatestSkinVersionRequirement The LatestSkinVersionRequirement is problematic for various reasons: 1) It uses the "useskin" query string parameter which may or may not refer to the correct skin (in the case of a non-existent skin it will always come to the conclusion that it is not modern Vector and thus must be legacy). 2) It uses the User object which may or may not be safeToLoad depending on when called. The feature seems redundant at this point, as we are separating code into separate classes Vector and Vector22 and all the features only apply to modern Vector. I suggest we remove it and use the features explicitly in the skin intended. Bug: T305232 Bug: T305262 Bug: T302627 Change-Id: I92fa33547bd601e05ddc8c1468e681892e47c16b --- includes/Constants.php | 22 ----- .../LatestSkinVersionRequirement.php | 91 ------------------- includes/Hooks.php | 9 +- includes/ServiceWiring.php | 25 ----- includes/SkinVector.php | 19 +--- includes/SkinVector22.php | 23 ++++- includes/SkinVectorLegacy.php | 7 ++ tests/phpunit/integration/VectorHooksTest.php | 11 --- .../LatestSkinVersionRequirementTest.php | 69 -------------- 9 files changed, 30 insertions(+), 246 deletions(-) delete mode 100644 includes/FeatureManagement/Requirements/LatestSkinVersionRequirement.php delete mode 100644 tests/phpunit/unit/FeatureManagement/Requirements/LatestSkinVersionRequirementTest.php diff --git a/includes/Constants.php b/includes/Constants.php index b1494011..60a4f54a 100644 --- a/includes/Constants.php +++ b/includes/Constants.php @@ -88,16 +88,6 @@ final class Constants { */ public const REQUIREMENT_FULLY_INITIALISED = 'FullyInitialised'; - /** - * @var string - */ - public const REQUIREMENT_LATEST_SKIN_VERSION = 'LatestSkinVersion'; - - /** - * @var string - */ - public const FEATURE_LATEST_SKIN = 'LatestSkin'; - /** * @var string */ @@ -142,18 +132,6 @@ final class Constants { */ public const QUERY_PARAM_LANGUAGE_IN_HEADER = 'languageinheader'; - /** - * Override the skin version user preference and site Config. See readme. - * @var string - */ - public const QUERY_PARAM_SKIN_VERSION = 'useskinversion'; - - /** - * Override the skin user preference and site Config. See readme. - * @var string - */ - public const QUERY_PARAM_SKIN = 'useskin'; - /** * @var string */ diff --git a/includes/FeatureManagement/Requirements/LatestSkinVersionRequirement.php b/includes/FeatureManagement/Requirements/LatestSkinVersionRequirement.php deleted file mode 100644 index 5ffaf1a4..00000000 --- a/includes/FeatureManagement/Requirements/LatestSkinVersionRequirement.php +++ /dev/null @@ -1,91 +0,0 @@ -request = $request; - $this->user = $user; - $this->userOptionsLookup = $userOptionsLookup; - } - - /** - * @inheritDoc - */ - public function getName(): string { - return Constants::REQUIREMENT_LATEST_SKIN_VERSION; - } - - /** - * @inheritDoc - * @throws \ConfigException - */ - public function isMet(): bool { - $useSkin = $this->request->getVal( 'useskin' ); - $user = $this->user; - if ( !$useSkin && $user->isSafeToLoad() ) { - $useSkin = $this->userOptionsLookup->getOption( - $user, - Constants::PREF_KEY_SKIN - ); - } - return $useSkin === Constants::SKIN_NAME_MODERN; - } -} diff --git a/includes/Hooks.php b/includes/Hooks.php index d9d7a3cb..097f3ff5 100644 --- a/includes/Hooks.php +++ b/includes/Hooks.php @@ -627,13 +627,6 @@ class Hooks { * @return bool */ private static function isSkinVersionLegacy( $skinName ): bool { - if ( $skinName === Constants::SKIN_NAME_MODERN ) { - return false; - } - - $isLatestSkinFeatureEnabled = VectorServices::getFeatureManager() - ->isFeatureEnabled( Constants::FEATURE_LATEST_SKIN ); - - return !$isLatestSkinFeatureEnabled; + return $skinName === Constants::SKIN_NAME_LEGACY; } } diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index 9bd10df6..85980eeb 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -26,7 +26,6 @@ use MediaWiki\MediaWikiServices; use Vector\Constants; use Vector\FeatureManagement\FeatureManager; use Vector\FeatureManagement\Requirements\DynamicConfigRequirement; -use Vector\FeatureManagement\Requirements\LatestSkinVersionRequirement; use Vector\FeatureManagement\Requirements\OverridableConfigRequirement; return [ @@ -44,26 +43,8 @@ return [ ) ); - // Feature: Latest skin - // ==================== $context = RequestContext::getMain(); - $featureManager->registerRequirement( - new LatestSkinVersionRequirement( - $context->getRequest(), - $context->getUser(), - $services->getUserOptionsLookup() - ) - ); - - $featureManager->registerFeature( - Constants::FEATURE_LATEST_SKIN, - [ - Constants::REQUIREMENT_FULLY_INITIALISED, - Constants::REQUIREMENT_LATEST_SKIN_VERSION, - ] - ); - // Feature: Languages in sidebar // ================================ $featureManager->registerRequirement( @@ -113,7 +94,6 @@ return [ Constants::FEATURE_LANGUAGE_IN_HEADER, [ Constants::REQUIREMENT_FULLY_INITIALISED, - Constants::REQUIREMENT_LATEST_SKIN_VERSION, Constants::REQUIREMENT_LANGUAGE_IN_HEADER, ] ); @@ -142,7 +122,6 @@ return [ Constants::FEATURE_LANGUAGE_IN_MAIN_PAGE_HEADER, [ Constants::REQUIREMENT_FULLY_INITIALISED, - Constants::REQUIREMENT_LATEST_SKIN_VERSION, Constants::REQUIREMENT_IS_MAIN_PAGE, Constants::REQUIREMENT_LANGUAGE_IN_HEADER, Constants::REQUIREMENT_LANGUAGE_IN_MAIN_PAGE_HEADER @@ -168,7 +147,6 @@ return [ Constants::FEATURE_LANGUAGE_ALERT_IN_SIDEBAR, [ Constants::REQUIREMENT_FULLY_INITIALISED, - Constants::REQUIREMENT_LATEST_SKIN_VERSION, Constants::REQUIREMENT_LANGUAGE_IN_HEADER, Constants::REQUIREMENT_LANGUAGE_ALERT_IN_SIDEBAR ] @@ -193,7 +171,6 @@ return [ Constants::FEATURE_TABLE_OF_CONTENTS, [ Constants::REQUIREMENT_FULLY_INITIALISED, - Constants::REQUIREMENT_LATEST_SKIN_VERSION, Constants::REQUIREMENT_TABLE_OF_CONTENTS ] ); @@ -230,7 +207,6 @@ return [ Constants::FEATURE_STICKY_HEADER, [ Constants::REQUIREMENT_FULLY_INITIALISED, - Constants::REQUIREMENT_LATEST_SKIN_VERSION, Constants::REQUIREMENT_STICKY_HEADER ] ); @@ -239,7 +215,6 @@ return [ Constants::FEATURE_STICKY_HEADER_EDIT, [ Constants::REQUIREMENT_FULLY_INITIALISED, - Constants::REQUIREMENT_LATEST_SKIN_VERSION, Constants::REQUIREMENT_STICKY_HEADER, Constants::REQUIREMENT_STICKY_HEADER_EDIT, ] diff --git a/includes/SkinVector.php b/includes/SkinVector.php index 01c837bc..9726df95 100644 --- a/includes/SkinVector.php +++ b/includes/SkinVector.php @@ -424,7 +424,7 @@ abstract class SkinVector extends SkinMustache { * @param bool $includeEditIcons * @return array */ - private function getStickyHeaderData( $searchBoxData, $includeEditIcons ): array { + final protected function getStickyHeaderData( $searchBoxData, $includeEditIcons ): array { $btns = [ self::TALK_ICON, self::HISTORY_ICON, @@ -541,21 +541,6 @@ abstract class SkinVector extends SkinMustache { 'searchform', true ), - 'data-vector-sticky-header' => $featureManager->isFeatureEnabled( - Constants::FEATURE_STICKY_HEADER - ) ? $this->getStickyHeaderData( - $this->getSearchData( - $parentData['data-search-box'], - // Collapse inside search box is disabled. - false, - false, - 'vector-sticky-search-form', - false - ), - $featureManager->isFeatureEnabled( - Constants::FEATURE_STICKY_HEADER_EDIT - ) - ) : false, 'data-toc' => $this->getTocData( $parentData['data-toc'] ?? [] ) ] ); @@ -642,7 +627,7 @@ abstract class SkinVector extends SkinMustache { * @param bool $autoExpandWidth * @return array modified version of $searchBoxData */ - private function getSearchData( + final protected function getSearchData( array $searchBoxData, bool $isCollapsible, bool $isPrimary, diff --git a/includes/SkinVector22.php b/includes/SkinVector22.php index 76c3ac43..032b0d6d 100644 --- a/includes/SkinVector22.php +++ b/includes/SkinVector22.php @@ -46,10 +46,27 @@ class SkinVector22 extends SkinVector { * @return array */ public function getTemplateData(): array { - $data = parent::getTemplateData(); + $featureManager = VectorServices::getFeatureManager(); + $parentData = parent::getTemplateData(); if ( !$this->isTableOfContentsVisibleInSidebar() ) { - unset( $data['data-toc'] ); + unset( $parentData['data-toc'] ); } - return $data; + return $parentData + [ + 'data-vector-sticky-header' => $featureManager->isFeatureEnabled( + Constants::FEATURE_STICKY_HEADER + ) ? $this->getStickyHeaderData( + $this->getSearchData( + $parentData['data-search-box'], + // Collapse inside search box is disabled. + false, + false, + 'vector-sticky-search-form', + false + ), + $featureManager->isFeatureEnabled( + Constants::FEATURE_STICKY_HEADER_EDIT + ) + ) : false, + ]; } } diff --git a/includes/SkinVectorLegacy.php b/includes/SkinVectorLegacy.php index a6eeef51..8127656d 100644 --- a/includes/SkinVectorLegacy.php +++ b/includes/SkinVectorLegacy.php @@ -16,4 +16,11 @@ class SkinVectorLegacy extends SkinVector { protected function isLegacy(): bool { return true; } + + /** + * @inheritDoc + */ + protected function isLanguagesInContentAt( $location ) { + return false; + } } diff --git a/tests/phpunit/integration/VectorHooksTest.php b/tests/phpunit/integration/VectorHooksTest.php index 791ffc44..3fd4a9f1 100644 --- a/tests/phpunit/integration/VectorHooksTest.php +++ b/tests/phpunit/integration/VectorHooksTest.php @@ -16,7 +16,6 @@ use RuntimeException; use Title; use User; use Vector\Constants; -use Vector\FeatureManagement\FeatureManager; use Vector\Hooks; use Vector\SkinVector22; use Vector\SkinVectorLegacy; @@ -289,16 +288,6 @@ class VectorHooksTest extends MediaWikiIntegrationTestCase { ); } - private function setFeatureLatestSkinVersionIsEnabled( $isEnabled ) { - $featureManager = new FeatureManager(); - $featureManager->registerSimpleRequirement( Constants::REQUIREMENT_LATEST_SKIN_VERSION, $isEnabled ); - $featureManager->registerFeature( Constants::FEATURE_LATEST_SKIN, [ - Constants::REQUIREMENT_LATEST_SKIN_VERSION - ] ); - - $this->setService( Constants::SERVICE_FEATURE_MANAGER, $featureManager ); - } - /** * @covers ::getVectorResourceLoaderConfig * @dataProvider provideGetVectorResourceLoaderConfig diff --git a/tests/phpunit/unit/FeatureManagement/Requirements/LatestSkinVersionRequirementTest.php b/tests/phpunit/unit/FeatureManagement/Requirements/LatestSkinVersionRequirementTest.php deleted file mode 100644 index 77231172..00000000 --- a/tests/phpunit/unit/FeatureManagement/Requirements/LatestSkinVersionRequirementTest.php +++ /dev/null @@ -1,69 +0,0 @@ - [ 'vector', null, false, '"1" isn\'t considered latest.' ]; - yield 'met' => [ 'vector-2022', null, true, '"2" is considered latest.' ]; - yield 'met (useskin override)' => [ 'vector', 'vector-2022', true, 'useskin overrides' ]; - yield 'not met (useskin override)' => [ 'vector-2022', 'vector', false, 'useskin overrides' ]; - } - - /** - * @dataProvider provideIsMet - * @covers ::isMet - */ - public function testIsMet( $skin, $useSkin, $expected, $msg ) { - $user = $this->createMock( User::class ); - $user->method( 'isRegistered' )->willReturn( true ); - $user->method( 'isSafeToLoad' )->willReturn( true ); - - $userOptionsLookup = $this->createMock( UserOptionsLookup::class ); - $userOptionsLookup->method( 'getOption' ) - ->willReturn( $skin ); - - $request = $this->createMock( WebRequest::class ); - $request->method( 'getVal' ) - ->willReturn( $useSkin ); - - $requirement = new LatestSkinVersionRequirement( - $request, - $user, - $userOptionsLookup - ); - - $this->assertSame( $expected, $requirement->isMet(), $msg ); - } - -}