jonny
/
Vector-Local
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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
This commit is contained in:
Jon Robson 2022-04-01 08:53:03 -07:00 committed by Jdlrobson
parent b25e0ac738
commit eca9fcbf79
9 changed files with 30 additions and 246 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
includes/Constants.php
View File

@ -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
*/

91
includes/FeatureManagement/Requirements/LatestSkinVersionRequirement.php
View File

@ -1,91 +0,0 @@
<?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\FeatureManagement\Requirements;
use MediaWiki\User\UserOptionsLookup;
use User;
use Vector\Constants;
use Vector\FeatureManagement\Requirement;
use WebRequest;
/**
* Checks if the current skin is modern Vector.
*
* @unstable
*
* @package Vector\FeatureManagement\Requirements
* @internal
*/
final class LatestSkinVersionRequirement implements Requirement {
/**
* @var WebRequest
*/
private $request;
/**
* @var User
*/
private $user;
/**
* @var UserOptionsLookup
*/
private $userOptionsLookup;
/**
* This constructor accepts all dependencies needed to obtain the skin version.
*
* @param WebRequest $request
* @param User $user
* @param UserOptionsLookup $userOptionsLookup
*/
public function __construct( WebRequest $request, User $user, UserOptionsLookup $userOptionsLookup ) {
$this->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;
}
}

9
includes/Hooks.php
View File

@ -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;
}
}

25
includes/ServiceWiring.php
View File

@ -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,
]

19
includes/SkinVector.php
View File

@ -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,

23
includes/SkinVector22.php
View File

@ -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,
];
}
}

7
includes/SkinVectorLegacy.php
View File

@ -16,4 +16,11 @@ class SkinVectorLegacy extends SkinVector {
protected function isLegacy(): bool {
return true;
}
/**
* @inheritDoc
*/
protected function isLanguagesInContentAt( $location ) {
return false;
}
}

11
tests/phpunit/integration/VectorHooksTest.php
View File

@ -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

69
tests/phpunit/unit/FeatureManagement/Requirements/LatestSkinVersionRequirementTest.php
View File

@ -1,69 +0,0 @@
<?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
*/
namespace Vector\FeatureManagement\Tests;
use MediaWiki\User\UserOptionsLookup;
use User;
use Vector\FeatureManagement\Requirements\LatestSkinVersionRequirement;
use WebRequest;
/**
* @group Vector
* @group FeatureManagement
* @coversDefaultClass \Vector\FeatureManagement\Requirements\LatestSkinVersionRequirement
*/
class LatestSkinVersionRequirementTest extends \MediaWikiUnitTestCase {
public function provideIsMet() {
// $version, $expected, $msg
yield 'not met' => [ '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 );
}
}