From 258e635ae51002f28d2899e5522e03c5934b49eb Mon Sep 17 00:00:00 2001 From: Piotr Miazga Date: Wed, 10 Apr 2019 23:43:50 +0200 Subject: [PATCH] Extract SkinOptions to separate class SkinOptions array was used to determine which options are available for current session. Once we started extracting things from SkinMinerva class, we found out that lots of things depend on SkinOptions. For example MainMenu/PageActionsMenu depend on skinsOptions var. We could pass $skin object as dependency to a menu builder, but this would cause a circural dependency (Skin depends on menu builder, menu builder depends on skin) which is an anti-pattern. In order to avoid such situations lets prepare first, and extract the SkinOptions to a separate class, register it as a service so different parts of Skin Minerva can freely use a single instance of SkinOptions object. Bug: T216152 Bug: T221012 Change-Id: Icd5da546e1bfaf8d9bfe86dab3b659a88eae19e4 --- includes/MinervaHooks.php | 26 +++--- includes/ServiceWiring.php | 4 + includes/SkinOptions.php | 102 ++++++++++++++++++++++ includes/skins/MinervaTemplate.php | 9 +- includes/skins/SkinMinerva.php | 107 ++++++------------------ skin.json | 1 + tests/phpunit/SkinOptionsTest.php | 50 +++++++++++ tests/phpunit/skins/SkinMinervaTest.php | 60 ++++--------- 8 files changed, 217 insertions(+), 142 deletions(-) create mode 100644 includes/SkinOptions.php create mode 100644 tests/phpunit/SkinOptionsTest.php diff --git a/includes/MinervaHooks.php b/includes/MinervaHooks.php index 89a9b9b..9a2f29b 100644 --- a/includes/MinervaHooks.php +++ b/includes/MinervaHooks.php @@ -19,6 +19,7 @@ */ use MediaWiki\MediaWikiServices; +use MediaWiki\Minerva\SkinOptions; /** * Hook handlers for Minerva skin. @@ -191,33 +192,34 @@ class MinervaHooks { ) { // setSkinOptions is not available if ( $skin instanceof SkinMinerva ) { - $services = \MediaWiki\MediaWikiServices::getInstance(); + $services = MediaWikiServices::getInstance(); $featureManager = $services ->getService( 'MobileFrontend.FeaturesManager' ); $userMode = $services->getService( 'MobileFrontend.AMC.UserMode' ); + $skinOptions = $services->getService( 'Minerva.SkinOptions' ); $isBeta = $mobileContext->isBetaGroupMember(); - $skin->setSkinOptions( [ - SkinMinerva::OPTION_AMC => $userMode->isEnabled(), - SkinMinerva::OPTIONS_TALK_AT_TOP => $featureManager->isFeatureAvailableForCurrentUser( + $skinOptions->setMultiple( [ + SkinOptions::OPTION_AMC => $userMode->isEnabled(), + SkinOptions::OPTIONS_TALK_AT_TOP => $featureManager->isFeatureAvailableForCurrentUser( 'MinervaTalkAtTop' ), - SkinMinerva::OPTIONS_MOBILE_BETA + SkinOptions::OPTIONS_MOBILE_BETA => $isBeta, - SkinMinerva::OPTION_CATEGORIES + SkinOptions::OPTION_CATEGORIES => $featureManager->isFeatureAvailableInContext( 'MinervaShowCategoriesButton', $mobileContext ), - SkinMinerva::OPTION_BACK_TO_TOP + SkinOptions::OPTION_BACK_TO_TOP => $featureManager->isFeatureAvailableInContext( 'MinervaEnableBackToTop', $mobileContext ), - SkinMinerva::OPTION_PAGE_ISSUES + SkinOptions::OPTION_PAGE_ISSUES => $featureManager->isFeatureAvailableInContext( 'MinervaPageIssuesNewTreatment', $mobileContext ), - SkinMinerva::OPTION_SHARE_BUTTON + SkinOptions::OPTION_SHARE_BUTTON => $featureManager->isFeatureAvailableInContext( 'MinervaShareButton', $mobileContext ), - SkinMinerva::OPTION_TOGGLING => true, - SkinMinerva::OPTION_MOBILE_OPTIONS => true, - SkinMinerva::OPTIONS_HISTORY_PAGE_ACTIONS => $featureManager->isFeatureAvailableForCurrentUser( + SkinOptions::OPTION_TOGGLING => true, + SkinOptions::OPTION_MOBILE_OPTIONS => true, + SkinOptions::OPTIONS_HISTORY_PAGE_ACTIONS => $featureManager->isFeatureAvailableForCurrentUser( 'MinervaHistoryInPageActions' ), ] ); diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index 8253fcb..23c6686 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -1,6 +1,7 @@ function ( MediaWikiServices $services ) { return new SkinUserPageHelper( RequestContext::getMain()->getTitle() ); + }, + 'Minerva.SkinOptions' => function ( MediaWikiServices $services ) { + return new SkinOptions(); } ]; diff --git a/includes/SkinOptions.php b/includes/SkinOptions.php new file mode 100644 index 0000000..e6eab61 --- /dev/null +++ b/includes/SkinOptions.php @@ -0,0 +1,102 @@ + true, + self::OPTIONS_MOBILE_BETA => false, + /** + * Whether the main menu should include a link to + * Special:Preferences of Special:MobileOptions + */ + self::OPTION_MOBILE_OPTIONS => false, + /** Whether a categories button should appear at the bottom of the skin. */ + self::OPTION_CATEGORIES => false, + /** Whether a back to top button appears at the bottom of the view page */ + self::OPTION_BACK_TO_TOP => false, + /** Whether a share button should appear in icons section */ + self::OPTION_SHARE_BUTTON => false, + /** Whether sections can be collapsed (requires MobileFrontend and MobileFormatter) */ + self::OPTION_TOGGLING => false, + self::OPTION_PAGE_ISSUES => false, + self::OPTIONS_TALK_AT_TOP => false, + self::OPTIONS_HISTORY_PAGE_ACTIONS => false, + ]; + + /** + * override an existing option or options with new values + * @param array $options + */ + public function setMultiple( array $options ) { + $this->skinOptions = array_merge( $this->skinOptions, $options ); + } + + /** + * Return whether a skin option is truthy. Should be one of self:OPTION_* flags + * @param string $key + * @return bool + */ + public function get( $key ) { + if ( !array_key_exists( $key, $this->skinOptions ) ) { + throw new \OutOfBoundsException( "SkinOption $key doesn't exist" ); + } + return $this->skinOptions[$key]; + } + + /** + * Get all skin options + * @return array + */ + public function getAll() { + return $this->skinOptions; + } + + /** + * Return whether any of the skin options have been set + * @return bool + */ + public function hasSkinOptions() { + foreach ( $this->skinOptions as $key => $val ) { + if ( $val ) { + return true; + } + } + return false; + } + +} diff --git a/includes/skins/MinervaTemplate.php b/includes/skins/MinervaTemplate.php index 4b09599..d01fb05 100644 --- a/includes/skins/MinervaTemplate.php +++ b/includes/skins/MinervaTemplate.php @@ -17,6 +17,8 @@ * * @file */ +use MediaWiki\MediaWikiServices; +use MediaWiki\Minerva\SkinOptions; /** * Extended Template class of BaseTemplate for mobile devices @@ -221,8 +223,9 @@ class MinervaTemplate extends BaseTemplate { * @todo replace with template engines */ protected function render( $data ) { + $skinOptions = MediaWikiServices::getInstance()->getService( 'Minerva.SkinOptions' ); $templateParser = new TemplateParser( __DIR__ ); - $skin = $this->getSkin(); + $internalBanner = $data[ 'internalBanner' ]; $preBodyHtml = isset( $data['prebodyhtml'] ) ? $data['prebodyhtml'] : ''; $hasHeadingHolder = $internalBanner || $preBodyHtml || isset( $data['page_actions'] ); @@ -264,8 +267,8 @@ class MinervaTemplate extends BaseTemplate { 'contenthtml' => $this->getContentHtml( $data ), 'secondaryactionshtml' => $this->getSecondaryActionsHtml(), 'footer' => $this->getFooterTemplateData( $data ), - 'isBeta' => $skin->getSkinOption( SkinMinerva::OPTIONS_MOBILE_BETA ), - 'tabs' => $hasTalkTabs && $skin->getSkinOption( SkinMinerva::OPTIONS_TALK_AT_TOP ) ? [ + 'isBeta' => $skinOptions->get( SkinOptions::OPTIONS_MOBILE_BETA ), + 'tabs' => $hasTalkTabs && $skinOptions->get( SkinOptions::OPTIONS_TALK_AT_TOP ) ? [ 'items' => array_values( $data['content_navigation']['namespaces'] ), ] : false, ]; diff --git a/includes/skins/SkinMinerva.php b/includes/skins/SkinMinerva.php index 9259d6d..ec3607c 100644 --- a/includes/skins/SkinMinerva.php +++ b/includes/skins/SkinMinerva.php @@ -19,6 +19,7 @@ */ use MediaWiki\Minerva\MenuBuilder; +use MediaWiki\Minerva\SkinOptions; use MediaWiki\MediaWikiServices; /** @@ -27,17 +28,6 @@ use MediaWiki\MediaWikiServices; * @ingroup Skins */ class SkinMinerva extends SkinTemplate { - /** Set of keys for available skin options. See $skinOptions. */ - const OPTION_MOBILE_OPTIONS = 'mobileOptionsLink'; - const OPTION_AMC = 'amc'; - const OPTION_CATEGORIES = 'categories'; - const OPTION_BACK_TO_TOP = 'backToTop'; - const OPTION_PAGE_ISSUES = 'pageIssues'; - const OPTION_SHARE_BUTTON = 'shareButton'; - const OPTION_TOGGLING = 'toggling'; - const OPTIONS_MOBILE_BETA = 'beta'; - const OPTIONS_TALK_AT_TOP = 'talkAtTop'; - const OPTIONS_HISTORY_PAGE_ACTIONS = 'historyInPageActions'; /** @const LEAD_SECTION_NUMBER integer which corresponds to the lead section in editing mode */ @@ -51,6 +41,17 @@ class SkinMinerva extends SkinTemplate { /** @var bool Whether the page is also available in other languages or variants */ protected $doesPageHaveLanguages = false; + /** @var SkinOptions */ + private $skinOptions; + + /** + * Initialize Minerva Skin + */ + public function __construct() { + parent::__construct( 'minerva' ); + $this->skinOptions = MediaWikiServices::getInstance()->getService( 'Minerva.SkinOptions' ); + } + /** * Returns the site name for the footer, either as a text or tag * @return string @@ -89,59 +90,6 @@ class SkinMinerva extends SkinTemplate { return $sitename; } - /** @var array skin specific options */ - protected $skinOptions = [ - // Defaults to true for desktop mode. - self::OPTION_AMC => true, - self::OPTIONS_MOBILE_BETA => false, - /** - * Whether the main menu should include a link to - * Special:Preferences of Special:MobileOptions - */ - self::OPTION_MOBILE_OPTIONS => false, - /** Whether a categories button should appear at the bottom of the skin. */ - self::OPTION_CATEGORIES => false, - /** Whether a back to top button appears at the bottom of the view page */ - self::OPTION_BACK_TO_TOP => false, - /** Whether a share button should appear in icons section */ - self::OPTION_SHARE_BUTTON => false, - /** Whether sections can be collapsed (requires MobileFrontend and MobileFormatter) */ - self::OPTION_TOGGLING => false, - self::OPTION_PAGE_ISSUES => false, - self::OPTIONS_TALK_AT_TOP => false, - self::OPTIONS_HISTORY_PAGE_ACTIONS => false, - ]; - - /** - * override an existing option or options with new values - * @param array $options - */ - public function setSkinOptions( $options ) { - $this->skinOptions = array_merge( $this->skinOptions, $options ); - } - - /** - * Return whether a skin option is truthy - * @param string $key - * @return bool - */ - public function getSkinOption( $key ) { - return $this->skinOptions[$key]; - } - - /** - * Return whether any of the skin options have been set - * @return bool - */ - public function hasSkinOptions() { - foreach ( $this->skinOptions as $key => $val ) { - if ( $val ) { - return true; - } - } - return false; - } - /** * initialize various variables and generate the template * @return QuickTemplate @@ -198,7 +146,7 @@ class SkinMinerva extends SkinTemplate { // If it's a talk page, add a link to the main namespace page // In AMC we do not need to do this as there is an easy way back to the article page // via the talk/article tabs. - if ( $title->isTalkPage() && !$this->getSkinOption( self::OPTION_AMC ) ) { + if ( $title->isTalkPage() && !$this->skinOptions->get( SkinOptions::OPTION_AMC ) ) { // if it's a talk page for which we have a special message, use it switch ( $title->getNamespace() ) { case NS_USER_TALK: @@ -259,7 +207,7 @@ class SkinMinerva extends SkinTemplate { } if ( $action === 'history' && $title->exists() ) { - return $this->getSkinOption( self::OPTIONS_HISTORY_PAGE_ACTIONS ); + return $this->skinOptions->get( SkinOptions::OPTIONS_HISTORY_PAGE_ACTIONS ); } if ( @@ -313,7 +261,8 @@ class SkinMinerva extends SkinTemplate { */ public function getPageClasses( $title ) { $className = parent::getPageClasses( $title ); - $className .= ' ' . ( $this->getSkinOption( self::OPTIONS_MOBILE_BETA ) ? 'beta' : 'stable' ); + $className .= ' ' . ( $this->skinOptions->get( SkinOptions::OPTIONS_MOBILE_BETA ) + ? 'beta' : 'stable' ); if ( $title->isMainPage() ) { $className .= ' page-Main_Page '; @@ -325,7 +274,7 @@ class SkinMinerva extends SkinTemplate { // The new treatment should only apply to the main namespace if ( $title->getNamespace() === NS_MAIN && - $this->getSkinOption( self::OPTION_PAGE_ISSUES ) + $this->skinOptions->get( SkinOptions::OPTION_PAGE_ISSUES ) ) { $className .= ' issues-group-B'; } @@ -346,7 +295,7 @@ class SkinMinerva extends SkinTemplate { * @return bool */ private function hasCategoryLinks() { - if ( !$this->getSkinOption( self::OPTION_CATEGORIES ) ) { + if ( !$this->skinOptions->get( SkinOptions::OPTION_CATEGORIES ) ) { return false; } $categoryLinks = $this->getOutput()->getCategoryLinks(); @@ -538,7 +487,7 @@ class SkinMinerva extends SkinTemplate { $returnToTitle = $this->getTitle()->getPrefixedText(); // Links specifically for mobile mode - if ( $this->getSkinOption( self::OPTION_MOBILE_OPTIONS ) ) { + if ( $this->skinOptions->get( SkinOptions::OPTION_MOBILE_OPTIONS ) ) { // Settings link $menu->insert( 'settings' ) ->addComponent( @@ -1084,7 +1033,7 @@ class SkinMinerva extends SkinTemplate { // in stable it will link to the wikitext talk page $title = $this->getTitle(); $subjectPage = $title->getSubjectPage(); - $talkAtBottom = !$this->getSkinOption( self::OPTIONS_TALK_AT_TOP ) || + $talkAtBottom = !$this->skinOptions->get( SkinOptions::OPTIONS_TALK_AT_TOP ) || $subjectPage->isMainPage(); $namespaces = $tpl->data['content_navigation']['namespaces']; if ( !$this->getUserPageHelper()->isUserPage() @@ -1306,12 +1255,8 @@ class SkinMinerva extends SkinTemplate { * @return array */ public function getSkinConfigVariables() { - $title = $this->getTitle(); - $user = $this->getUser(); - $out = $this->getOutput(); - $vars = [ - 'wgMinervaFeatures' => $this->skinOptions, + 'wgMinervaFeatures' => $this->skinOptions->getAll(), 'wgMinervaDownloadNamespaces' => $this->getConfig()->get( 'MinervaDownloadNamespaces' ), 'wgMinervaMenuData' => $this->getMenuData(), ]; @@ -1374,10 +1319,10 @@ class SkinMinerva extends SkinTemplate { $modules[] = 'skins.minerva.talk'; } - if ( $this->hasSkinOptions() ) { + if ( $this->skinOptions->hasSkinOptions() ) { $modules[] = 'skins.minerva.options'; } - if ( $this->getSkinOption( self::OPTION_SHARE_BUTTON ) ) { + if ( $this->skinOptions->get( SkinOptions::OPTION_SHARE_BUTTON ) ) { $modules[] = 'skins.minerva.share'; } return $modules; @@ -1407,7 +1352,7 @@ class SkinMinerva extends SkinTemplate { ] ); - if ( $this->getSkinOption( self::OPTION_TOGGLING ) ) { + if ( $this->skinOptions->get( SkinOptions::OPTION_TOGGLING ) ) { // Extension can unload "toggling" modules via the hook $modules['toggling'] = [ 'skins.minerva.toggling' ]; } @@ -1428,7 +1373,7 @@ class SkinMinerva extends SkinTemplate { */ public function addToBodyAttributes( $out, &$bodyAttrs ) { $classes = $out->getProperty( 'bodyClassName' ); - if ( $this->getSkinOption( self::OPTION_AMC ) ) { + if ( $this->skinOptions->get( SkinOptions::OPTION_AMC ) ) { $classes .= ' minerva--amc-enabled'; } else { $classes .= ' minerva--amc-disabled'; @@ -1463,7 +1408,7 @@ class SkinMinerva extends SkinTemplate { $styles[] = 'skins.minerva.icons.loggedin'; } - if ( $this->getSkinOption( self::OPTION_AMC ) ) { + if ( $this->skinOptions->get( SkinOptions::OPTION_AMC ) ) { $styles[] = 'skins.minerva.amc.styles'; } diff --git a/skin.json b/skin.json index b9a9810..d304877 100644 --- a/skin.json +++ b/skin.json @@ -76,6 +76,7 @@ "MediaWiki\\Minerva\\MenuBuilder": "includes/skins/MenuBuilder.php", "MediaWiki\\Minerva\\MenuEntry": "includes/skins/MenuEntry.php", "MediaWiki\\Minerva\\ResourceLoaderLessVarFileModule": "includes/ResourceLoaderLessVarFileModule.php", + "MediaWiki\\Minerva\\SkinOptions": "includes/SkinOptions.php", "MediaWiki\\Minerva\\SkinUserPageHelper": "includes/skins/SkinUserPageHelper.php" }, "ConfigRegistry": { diff --git a/tests/phpunit/SkinOptionsTest.php b/tests/phpunit/SkinOptionsTest.php new file mode 100644 index 0000000..bad1320 --- /dev/null +++ b/tests/phpunit/SkinOptionsTest.php @@ -0,0 +1,50 @@ +get( SkinOptions::OPTION_AMC ); + $options->setMultiple( [ SkinOptions::OPTION_AMC => !$defaultValue ] ); + + $allOptions = $options->getAll(); + + $this->assertEquals( !$defaultValue, $options->get( SkinOptions::OPTION_AMC ) ); + $this->assertArrayHasKey( SkinOptions::OPTION_AMC, $allOptions ); + $this->assertEquals( !$defaultValue, $allOptions[ SkinOptions::OPTION_AMC ] ); + } + + /** + * @covers ::hasSkinOptions + */ + public function testHasSkinOptions() { + $options = new SkinOptions(); + // set OPTION_AMC to true just in case someone decides to set everything to false + // sometime in the future. + $options->setMultiple( [ SkinOptions::OPTION_AMC => true ] ); + $this->assertTrue( $options->hasSkinOptions() ); + $options->setMultiple( [ SkinOptions::OPTION_BACK_TO_TOP => true ] ); + $this->assertTrue( $options->hasSkinOptions() ); + $options->setMultiple( [ + SkinOptions::OPTION_AMC => false, + SkinOptions::OPTION_BACK_TO_TOP => false + ] ); + $this->assertFalse( $options->hasSkinOptions() ); + } +} diff --git a/tests/phpunit/skins/SkinMinervaTest.php b/tests/phpunit/skins/SkinMinervaTest.php index 2267e4d..f8e241a 100644 --- a/tests/phpunit/skins/SkinMinervaTest.php +++ b/tests/phpunit/skins/SkinMinervaTest.php @@ -2,6 +2,7 @@ namespace Tests\MediaWiki\Minerva; +use MediaWiki\Minerva\SkinOptions; use MediaWikiTestCase; use MinervaUI; use MWTimestamp; @@ -41,43 +42,14 @@ class EchoNotifUser { class SkinMinervaTest extends MediaWikiTestCase { const OPTIONS_MODULE = 'skins.minerva.options'; - /** - * @covers ::addToBodyAttributes - */ - public function testAddToBodyAttributes() { - // The `class` attribute gets set to the "bodyClassName" property by - // default. - $this->assertContains( - 'no-js', - $this->addToBodyAttributes( 'no-js', false ) - ); - - $classes = $this->addToBodyAttributes( 'no-js', true ); - - $this->assertContains( 'no-js', $classes ); - } - - private function addToBodyAttributes( - $bodyClassName - ) { - $context = new RequestContext(); - $context->setTitle( Title::newFromText( 'Test' ) ); - - $outputPage = $context->getOutput(); - $outputPage->setProperty( 'bodyClassName', $bodyClassName ); - - $bodyAttrs = [ 'class' => '' ]; - - $skin = new SkinMinerva(); - $skin->setContext( $context ); - $skin->addToBodyAttributes( $outputPage, $bodyAttrs ); - - return explode( ' ', $bodyAttrs[ 'class' ] ); + private function overrideSkinOptions( $options ) { + $mockOptions = new SkinOptions(); + $mockOptions->setMultiple( $options ); + $this->setService( 'Minerva.SkinOptions', $mockOptions ); } /** * @covers ::setContext - * @covers ::setSkinOptions * @covers ::hasCategoryLinks */ public function testHasCategoryLinksWhenOptionIsOff() { @@ -87,14 +59,13 @@ class SkinMinervaTest extends MediaWikiTestCase { $outputPage->expects( $this->never() ) ->method( 'getCategoryLinks' ); + $this->overrideSkinOptions( [ SkinOptions::OPTION_CATEGORIES => false ] ); $context = new RequestContext(); $context->setTitle( Title::newFromText( 'Test' ) ); $context->setOutput( $outputPage ); $skin = new SkinMinerva(); $skin->setContext( $context ); - $skin->setSkinOptions( [ SkinMinerva::OPTION_CATEGORIES => false ] ); - $skin = TestingAccessWrapper::newFromObject( $skin ); $this->assertEquals( $skin->hasCategoryLinks(), false ); @@ -105,7 +76,6 @@ class SkinMinervaTest extends MediaWikiTestCase { * @param array $categoryLinks * @param bool $expected * @covers ::setContext - * @covers ::setSkinOptions * @covers ::hasCategoryLinks */ public function testHasCategoryLinks( array $categoryLinks, $expected ) { @@ -116,13 +86,14 @@ class SkinMinervaTest extends MediaWikiTestCase { ->method( 'getCategoryLinks' ) ->will( $this->returnValue( $categoryLinks ) ); + $this->overrideSkinOptions( [ SkinOptions::OPTION_CATEGORIES => true ] ); + $context = new RequestContext(); $context->setTitle( Title::newFromText( 'Test' ) ); $context->setOutput( $outputPage ); $skin = new SkinMinerva(); $skin->setContext( $context ); - $skin->setSkinOptions( [ SkinMinerva::OPTION_CATEGORIES => true ] ); $skin = TestingAccessWrapper::newFromObject( $skin ); @@ -169,21 +140,18 @@ class SkinMinervaTest extends MediaWikiTestCase { * @param string $moduleName Module name that is being tested * @param bool $expected Whether the module is expected to be returned by the function being tested */ - public function testGetContextSpecificModules( $backToTopValue, - $moduleName, $expected - ) { - $skin = new SkinMinerva(); - $skin->setSkinOptions( [ - SkinMinerva::OPTION_AMC => false, + public function testGetContextSpecificModules( $backToTopValue, $moduleName, $expected ) { + $this->overrideSkinOptions( [ + SkinOptions::OPTION_AMC => false, + 'backToTop' => $backToTopValue, ] ); + + $skin = new SkinMinerva(); $title = Title::newFromText( 'Test' ); $testContext = RequestContext::getMain(); $testContext->setTitle( $title ); $skin->setContext( $testContext ); - $skin->setSkinOptions( [ - 'backToTop' => $backToTopValue, - ] ); if ( $expected ) { $this->assertContains( $moduleName, $skin->getContextSpecificModules() );