From 933dc0e37005ef6a9e4b7c3ca765c8be7b6264b6 Mon Sep 17 00:00:00 2001 From: Stephen Niedzielski Date: Mon, 22 Jul 2019 14:58:34 -0600 Subject: [PATCH] [dev] Replace menu entry inheritance with functions Break up Menu/DefaultBuilder into functions that are reusable without inheritance. The functions do not need much state to produce their outputs and a weighty inheritance hierarchy makes the code difficult to reason about. The functions are used in a following patch for the user menu. They're now simple, independent, static functions in BuilderUtil that are easy to reason about and compose. Also, ban inheritance via `final` in a few places nearby. Inheritance has not worked well in MobileFrontend and enabling it should be a special deliberate case, not a default. E.g., in the user menu, the changes could have been to the base class' getPersonalTools() method such that the client passes a parameter for the advanced config or maybe just override it in the subclass. In either case, it makes the whole hierarchy nuanced and harder to reason about for something that should be dead simple. Bug: T214540 Change-Id: I6e9a2b36a1bff387eb3b33ea65b0a6806962810a --- includes/menu/Definitions.php | 5 +- includes/menu/Main/AdvancedBuilder.php | 69 ++++++++++++++++++-- includes/menu/Main/BuilderUtil.php | 90 ++++++++++++++++++++++++++ includes/menu/Main/DefaultBuilder.php | 66 ++++--------------- 4 files changed, 168 insertions(+), 62 deletions(-) create mode 100644 includes/menu/Main/BuilderUtil.php diff --git a/includes/menu/Definitions.php b/includes/menu/Definitions.php index 66105fb..8ccc1c0 100644 --- a/includes/menu/Definitions.php +++ b/includes/menu/Definitions.php @@ -76,7 +76,6 @@ final class Definitions { 'contributions', $this->context->msg( 'mobile-frontend-main-menu-contributions' )->escaped(), SpecialPage::getTitleFor( 'Contributions', $this->user->getName() )->getLocalURL() - ) ); } @@ -110,12 +109,12 @@ final class Definitions { } /** - * Creates a login or logout button + * Creates a login or logout button with a profile button. * * @param Group $group * @throws MWException */ - public function insertLogInOutMenuItem( Group $group ) { + public function insertAuthMenuItem( Group $group ) { $group->insertEntry( new AuthMenuEntry( $this->user, $this->context, diff --git a/includes/menu/Main/AdvancedBuilder.php b/includes/menu/Main/AdvancedBuilder.php index 2490c23..e455910 100644 --- a/includes/menu/Main/AdvancedBuilder.php +++ b/includes/menu/Main/AdvancedBuilder.php @@ -23,6 +23,8 @@ namespace MediaWiki\Minerva\Menu\Main; use FatalError; use Hooks; use MWException; +use User; +use MediaWiki\Minerva\Menu\Definitions; use MediaWiki\Minerva\Menu\Group; /** @@ -32,29 +34,88 @@ use MediaWiki\Minerva\Menu\Group; * * @package MediaWiki\Minerva\Menu\Main */ -class AdvancedBuilder extends DefaultBuilder { +final class AdvancedBuilder implements IBuilder { + /** + * @var bool + */ + private $showMobileOptions; /** + * Currently logged in user + * @var User + */ + private $user; + + /** + * @var Definitions + */ + private $definitions; + + /** + * Initialize the Default Main Menu builder + * + * @param bool $showMobileOptions Show MobileOptions instead of Preferences + * @param User $user The current user + * @param Definitions $definitions A menu items definitions set + */ + public function __construct( $showMobileOptions, User $user, Definitions $definitions ) { + $this->showMobileOptions = $showMobileOptions; + $this->user = $user; + $this->definitions = $definitions; + } + + /** + * @inheritDoc * @return Group[] * @throws FatalError * @throws MWException */ public function getGroups(): array { return [ - $this->getDiscoveryTools(), + BuilderUtil::getDiscoveryTools( $this->definitions ), $this->getPersonalTools(), $this->getSiteTools(), - $this->getConfigurationTools(), + BuilderUtil::getConfigurationTools( $this->definitions, $this->showMobileOptions ), ]; } + /** + * @inheritDoc + * @throws FatalError + * @throws MWException + */ + public function getSiteLinks(): Group { + return BuilderUtil::getSiteLinks( $this->definitions ); + } + + /** + * Builds the personal tools menu item group. + * @return Group + * @throws FatalError + * @throws MWException + */ + private function getPersonalTools(): Group { + $group = new Group(); + + $this->definitions->insertAuthMenuItem( $group ); + + if ( $this->user->isLoggedIn() ) { + $this->definitions->insertWatchlistMenuItem( $group ); + $this->definitions->insertContributionsMenuItem( $group ); + } + + // Allow other extensions to add or override tools + Hooks::run( 'MobileMenu', [ 'personal', &$group ] ); + return $group; + } + /** * Prepares a list of links that have the purpose of discovery in the main navigation menu * @return Group * @throws FatalError * @throws MWException */ - public function getSiteTools(): Group { + private function getSiteTools(): Group { $group = new Group(); $this->definitions->insertSpecialPages( $group ); diff --git a/includes/menu/Main/BuilderUtil.php b/includes/menu/Main/BuilderUtil.php new file mode 100644 index 0000000..493e600 --- /dev/null +++ b/includes/menu/Main/BuilderUtil.php @@ -0,0 +1,90 @@ +insertHomeItem( $group ); + $definitions->insertRandomItem( $group ); + $definitions->insertNearbyIfSupported( $group ); + + // Allow other extensions to add or override tools + Hooks::run( 'MobileMenu', [ 'discovery', &$group ] ); + return $group; + } + + /** + * Like SkinMinerva#getDiscoveryTools and #getPersonalTools, create + * a group of configuration-related menu items. Currently, only the Settings menu item is in the + * group. + * @param Definitions $definitions A menu items definitions set + * @param bool $showMobileOptions Show MobileOptions instead of Preferences + * @return Group + * @throws MWException + */ + public static function getConfigurationTools( + Definitions $definitions, $showMobileOptions + ): Group { + $group = new Group(); + + $showMobileOptions ? + $definitions->insertMobileOptionsItem( $group ) : + $definitions->insertPreferencesItem( $group ); + + return $group; + } + + /** + * Returns an array of sitelinks to add into the main menu footer. + * @param Definitions $definitions A menu items definitions set + * @return Group Collection of site links + * @throws MWException + */ + public static function getSiteLinks( Definitions $definitions ): Group { + $group = new Group(); + + $definitions->insertAboutItem( $group ); + $definitions->insertDisclaimersItem( $group ); + // Allow other extensions to add or override tools + Hooks::run( 'MobileMenu', [ 'sitelinks', &$group ] ); + return $group; + } +} diff --git a/includes/menu/Main/DefaultBuilder.php b/includes/menu/Main/DefaultBuilder.php index 8bd6d12..1695eac 100644 --- a/includes/menu/Main/DefaultBuilder.php +++ b/includes/menu/Main/DefaultBuilder.php @@ -30,7 +30,7 @@ use MediaWiki\Minerva\Menu\Group; /** * Used to build default (available for everyone by default) main menu */ -class DefaultBuilder implements IBuilder { +final class DefaultBuilder implements IBuilder { /** * @var bool @@ -41,12 +41,12 @@ class DefaultBuilder implements IBuilder { * Currently logged in user * @var User */ - protected $user; + private $user; /** * @var Definitions */ - protected $definitions; + private $definitions; /** * Initialize the Default Main Menu builder @@ -62,34 +62,24 @@ class DefaultBuilder implements IBuilder { } /** - * @return Group[] + * @inheritDoc * @throws FatalError * @throws MWException */ public function getGroups(): array { return [ - $this->getDiscoveryTools(), + BuilderUtil::getDiscoveryTools( $this->definitions ), $this->getPersonalTools(), - $this->getConfigurationTools(), + BuilderUtil::getConfigurationTools( $this->definitions, $this->showMobileOptions ), ]; } /** - * Prepares a list of links that have the purpose of discovery in the main navigation menu - * @return Group - * @throws FatalError + * @inheritDoc * @throws MWException */ - protected function getDiscoveryTools(): Group { - $group = new Group(); - - $this->definitions->insertHomeItem( $group ); - $this->definitions->insertRandomItem( $group ); - $this->definitions->insertNearbyIfSupported( $group ); - - // Allow other extensions to add or override tools - Hooks::run( 'MobileMenu', [ 'discovery', &$group ] ); - return $group; + public function getSiteLinks(): Group { + return BuilderUtil::getSiteLinks( $this->definitions ); } /** @@ -101,10 +91,10 @@ class DefaultBuilder implements IBuilder { * @throws FatalError * @throws MWException */ - protected function getPersonalTools(): Group { + private function getPersonalTools(): Group { $group = new Group(); - $this->definitions->insertLogInOutMenuItem( $group ); + $this->definitions->insertAuthMenuItem( $group ); if ( $this->user->isLoggedIn() ) { $this->definitions->insertWatchlistMenuItem( $group ); @@ -115,38 +105,4 @@ class DefaultBuilder implements IBuilder { Hooks::run( 'MobileMenu', [ 'personal', &$group ] ); return $group; } - - /** - * Like SkinMinerva#getDiscoveryTools and #getPersonalTools, create - * a group of configuration-related menu items. Currently, only the Settings menu item is in the - * group. - * - * @return Group - * @throws MWException - */ - protected function getConfigurationTools(): Group { - $group = new Group(); - - $this->showMobileOptions ? - $this->definitions->insertMobileOptionsItem( $group ) : - $this->definitions->insertPreferencesItem( $group ); - - return $group; - } - - /** - * Returns an array of sitelinks to add into the main menu footer. - * @return Group Collection of site links - * @throws MWException - */ - public function getSiteLinks(): Group { - $group = new Group(); - - $this->definitions->insertAboutItem( $group ); - $this->definitions->insertDisclaimersItem( $group ); - // Allow other extensions to add or override tools - Hooks::run( 'MobileMenu', [ 'sitelinks', &$group ] ); - return $group; - } - }