From 3f95820467bcc9bd000aebe51bcebcb3aeb054b8 Mon Sep 17 00:00:00 2001 From: Sam Smith Date: Fri, 28 Feb 2020 15:42:05 +0000 Subject: [PATCH] featureManager: Set -> Requirement As was noted in https://phabricator.wikimedia.org/T244481#5859513, the term "set" doesn't seem natural. Piotr Miazga (polishdeveloper, pmiazga) and Nicholas Ray (nray) suggested a number of good replacements, including "requirement." Serendipitously, this term is already used in FeatureManager's documentation. Bug: T244481 Change-Id: I559c2d4149db69235cdd4bb880697deb1a145743 --- includes/FeatureManagement/FeatureManager.php | 93 ++++++++++--------- includes/FeatureManagement/TODO.md | 12 +-- .../FeatureManagement/FeatureManagerTest.php | 46 ++++----- 3 files changed, 79 insertions(+), 72 deletions(-) diff --git a/includes/FeatureManagement/FeatureManager.php b/includes/FeatureManagement/FeatureManager.php index 0d1231289..c7f4ca948 100644 --- a/includes/FeatureManagement/FeatureManager.php +++ b/includes/FeatureManagement/FeatureManager.php @@ -47,11 +47,11 @@ final class FeatureManager { private $features = []; /** - * A map of set name to whether the set is enabled. + * A map of requirement name to whether the requirement is met. * * @var Array */ - private $sets = []; + private $requirements = []; /** * Register a feature and its requirements. @@ -59,42 +59,46 @@ final class FeatureManager { * Essentially, a "feature" is a friendly (hopefully) name for some component, however big or * small, that has some requirements. A feature manager allows us to decouple the component's * logic from its requirements, allowing them to vary independently. Moreover, the use of - * friendly names wherever possible allows us to define common languages with our non-technical + * friendly names wherever possible allows us to define a common language with our non-technical * colleagues. * * ```php - * $featureManager->registerFeature( 'featureB', 'setA' ); + * $featureManager->registerFeature( 'featureA', 'requirementA' ); * ``` * - * defines the "featureB" feature, which is enabled when the "setA" set is enabled. + * defines the "featureA" feature, which is enabled when the "requirementA" requirement is met. * * ```php - * $featureManager->registerFeature( 'featureC', [ 'setA', 'setB' ] ); + * $featureManager->registerFeature( 'featureB', [ 'requirementA', 'requirementB' ] ); * ``` * - * defines the "featureC" feature, which is enabled when the "setA" and "setB" sets are enabled. - * Note well that the feature is only enabled when _all_ requirements are met, i.e. the - * requirements are evaluated in order and logically `AND`ed together. + * defines the "featureB" feature, which is enabled when the "requirementA" and "requirementB" + * requirements are met. Note well that the feature is only enabled when _all_ requirements are + * met, i.e. the requirements are evaluated in order and logically `AND`ed together. * * @param string $feature The name of the feature - * @param string|array $requirements Which sets the feature requires to be enabled. As above, - * you can define a feature that requires a single set via the shorthand + * @param string|array $requirements The feature's requirements. As above, you can define a + * feature that requires a single requirement via the shorthand * * ```php - * $featureManager->registerFeature( 'feature', 'set' ); - * // Equivalent to $featureManager->registerFeature( 'feature', [ 'set' ] ); + * $featureManager->registerFeature( 'feature', 'requirementA' ); + * // Equivalent to $featureManager->registerFeature( 'feature', [ 'requirementA' ] ); * ``` * * @throws \LogicException If the feature is already registered * @throws \Wikimedia\Assert\ParameterAssertionException If the feature's requirements aren't - * the name of a single set or an array of sets - * @throws \InvalidArgumentException If the feature requires a set that isn't registered + * the name of a single requirement or a list of requirements + * @throws \InvalidArgumentException If the feature references a requirement that isn't + * registered */ public function registerFeature( $feature, $requirements ) { // // Validation if ( array_key_exists( $feature, $this->features ) ) { - throw new \LogicException( "Feature \"{$feature}\" is already registered." ); + throw new \LogicException( sprintf( + 'Feature "%s" is already registered.', + $feature + ) ); } Assert::parameterType( 'string|array', $requirements, 'requirements' ); @@ -103,11 +107,13 @@ final class FeatureManager { Assert::parameterElementType( 'string', $requirements, 'requirements' ); - foreach ( $requirements as $set ) { - if ( !array_key_exists( $set, $this->sets ) ) { - throw new \InvalidArgumentException( - "Feature \"{$feature}\" references set \"{$set}\", which hasn't been registered" - ); + foreach ( $requirements as $name ) { + if ( !array_key_exists( $name, $this->requirements ) ) { + throw new \InvalidArgumentException( sprintf( + 'Feature "%s" references requirement "%s", which hasn\'t been registered', + $feature, + $name + ) ); } } @@ -130,8 +136,8 @@ final class FeatureManager { $requirements = $this->features[$feature]; - foreach ( $requirements as $set ) { - if ( !$this->sets[$set] ) { + foreach ( $requirements as $name ) { + if ( !$this->requirements[$name] ) { return false; } } @@ -140,43 +146,44 @@ final class FeatureManager { } /** - * Register a set. + * Register a requirement. * - * A set is some condition of the application state that a feature requires to be true or false. + * A requirement is some condition of the application state that a feature requires to be true + * or false. * - * At the moment, these conditions can only be evaluated when the set is being defined, i.e. at - * boot time. At that time, certain objects mightn't have been fully loaded - * (see User::isSafeToLoad). See TODO.md for the proposed list of steps to allow this feature + * At the moment, these conditions can only be evaluated when the requirement is being defined, + * i.e. at boot time. At that time, certain objects mightn't have been fully loaded (see + * User::isSafeToLoad). See TODO.md for the proposed list of steps to allow this feature * manager to handle that scenario. * - * @param string $set The name of the set - * @param bool $isEnabled Whether the set is enabled + * @param string $name The name of the requirement + * @param bool $isMet Whether the requirement is met * - * @throws \LogicException If the set has already been registered + * @throws \LogicException If the requirement has already been registered */ - public function registerSet( $set, $isEnabled ) { - if ( array_key_exists( $set, $this->sets ) ) { - throw new \LogicException( "Set \"{$set}\" is already registered." ); + public function registerRequirement( $name, $isMet ) { + if ( array_key_exists( $name, $this->requirements ) ) { + throw new \LogicException( "The requirement \"{$name}\" is already registered." ); } - Assert::parameterType( 'boolean', $isEnabled, 'isEnabled' ); + Assert::parameterType( 'boolean', $isMet, 'isMet' ); - $this->sets[$set] = $isEnabled; + $this->requirements[$name] = $isMet; } /** - * Gets whether the set is enabled. + * Gets whether the requirement is met. * - * @param string $set The name of the set + * @param string $name The name of the requirement * @return bool * - * @throws \InvalidArgumentException If the set isn't registerd + * @throws \InvalidArgumentException If the requirement isn't registered */ - public function isSetEnabled( $set ) { - if ( !array_key_exists( $set, $this->sets ) ) { - throw new \InvalidArgumentException( "Set \"{$set}\" isn't registered." ); + public function isRequirementMet( $name ) { + if ( !array_key_exists( $name, $this->requirements ) ) { + throw new \InvalidArgumentException( "Requirement \"{$name}\" isn't registered." ); } - return $this->sets[$set]; + return $this->requirements[$name]; } } diff --git a/includes/FeatureManagement/TODO.md b/includes/FeatureManagement/TODO.md index 62b70195e..0279e9f9c 100644 --- a/includes/FeatureManagement/TODO.md +++ b/includes/FeatureManagement/TODO.md @@ -6,14 +6,14 @@ API and associated scaffolding classes (see https://phabricator.wikimedia.org/T2 https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/Vector/+/572323/). This document aims to list the steps required to get from this system to something as powerful as Piotr's. -1. Decide whether "set" is the correct name +1. ~~Decide whether "set" is the correct name~~ 2. Add support for sets that utilise contextual information that isn't available at boot time, e.g. ```php use Vector\Constants; use IContextSource; -$featureManager->registerSet( Constants::LOGGED_IN_SET, function( IContextSource $context ) { +$featureManager->registerRequirement( Constants::LOGGED_IN_REQ, function( IContextSource $context ) { $user = $context->getUser(); return $user @@ -21,13 +21,13 @@ $featureManager->registerSet( Constants::LOGGED_IN_SET, function( IContextSource && $user->isLoggedIn(); } ); -$featureManager->registerSet( Constants::MAINSPACE_SET, function ( IContextSource $context ) { +$featureManager->registerRequirement( Constants::MAINSPACE_REQ, function ( IContextSource $context ) { $title = $context->getTitle(); return $title && $title->inNamespace( NS_MAIN ); } ); ``` -3. Consider supporing memoization of those sets (see https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/Vector/+/573626/7/includes/FeatureManagement/FeatureManager.php@68) -4. Add support for getting all sets -5. Add support for getting all features enabled when a set is enabled/disabled +3. Consider supporing memoization of those requirements (see https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/Vector/+/573626/7/includes/FeatureManagement/FeatureManager.php@68) +4. Add support for getting all requirements +5. Add support for getting all features enabled when a requirement is enabled/disabled diff --git a/tests/phpunit/unit/FeatureManagement/FeatureManagerTest.php b/tests/phpunit/unit/FeatureManagement/FeatureManagerTest.php index c217a446d..22f142022 100644 --- a/tests/phpunit/unit/FeatureManagement/FeatureManagerTest.php +++ b/tests/phpunit/unit/FeatureManagement/FeatureManagerTest.php @@ -31,24 +31,24 @@ use Vector\FeatureManagement\FeatureManager; class FeatureManagerTest extends \MediaWikiUnitTestCase { /** - * @covers ::registerSet + * @covers ::registerRequirement */ - public function testRegisterSetThrowsWhenSetIsRegisteredTwice() { + public function testRegisterRequirementThrowsWhenRequirementIsRegisteredTwice() { $this->expectException( \LogicException::class ); $featureManager = new FeatureManager(); - $featureManager->registerSet( 'setA', true ); - $featureManager->registerSet( 'setA', true ); + $featureManager->registerRequirement( 'requirementA', true ); + $featureManager->registerRequirement( 'requirementA', true ); } /** - * @covers ::registerSet + * @covers ::registerRequirement */ - public function testRegisterSetValidatesIsEnabled() { + public function testRegisterRequirementValidatesIsEnabled() { $this->expectException( \Wikimedia\Assert\ParameterAssertionException::class ); $featureManager = new FeatureManager(); - $featureManager->registerSet( 'setA', 'foo' ); + $featureManager->registerRequirement( 'requirementA', 'foo' ); } public static function provideInvalidFeatureConfig() { @@ -60,7 +60,7 @@ class FeatureManagerTest extends \MediaWikiUnitTestCase { [ 1 ], ], - // The "bar" set hasn't been registered. + // The "bar" requirement hasn't been registered. [ \InvalidArgumentException::class, [ @@ -78,30 +78,30 @@ class FeatureManagerTest extends \MediaWikiUnitTestCase { $this->expectException( $expectedExceptionType ); $featureManager = new FeatureManager(); - $featureManager->registerSet( 'set', true ); + $featureManager->registerRequirement( 'requirement', true ); $featureManager->registerFeature( 'feature', $config ); } /** - * @covers ::isSetEnabled + * @covers ::isRequirementMet */ - public function testIsSetEnabled() { + public function testIsRequirementMet() { $featureManager = new FeatureManager(); - $featureManager->registerSet( 'enabled', true ); - $featureManager->registerSet( 'disabled', false ); + $featureManager->registerRequirement( 'enabled', true ); + $featureManager->registerRequirement( 'disabled', false ); - $this->assertTrue( $featureManager->isSetEnabled( 'enabled' ) ); - $this->assertFalse( $featureManager->isSetEnabled( 'disabled' ) ); + $this->assertTrue( $featureManager->isRequirementMet( 'enabled' ) ); + $this->assertFalse( $featureManager->isRequirementMet( 'disabled' ) ); } /** - * @covers ::isSetEnabled + * @covers ::isRequirementMet */ - public function testIsSetEnabledThrowsExceptionWhenSetIsntRegistered() { + public function testIsRequirementMetThrowsExceptionWhenRequirementIsntRegistered() { $this->expectException( \InvalidArgumentException::class ); $featureManager = new FeatureManager(); - $featureManager->isSetEnabled( 'foo' ); + $featureManager->isRequirementMet( 'foo' ); } /** @@ -120,25 +120,25 @@ class FeatureManagerTest extends \MediaWikiUnitTestCase { */ public function testIsFeatureEnabled() { $featureManager = new FeatureManager(); - $featureManager->registerSet( 'foo', false ); + $featureManager->registerRequirement( 'foo', false ); $featureManager->registerFeature( 'requiresFoo', 'foo' ); $this->assertFalse( $featureManager->isFeatureEnabled( 'requiresFoo' ), - 'A feature is disabled when the set that it requires is disabled.' + 'A feature is disabled when the requirement that it requires is disabled.' ); // --- - $featureManager->registerSet( 'bar', true ); - $featureManager->registerSet( 'baz', true ); + $featureManager->registerRequirement( 'bar', true ); + $featureManager->registerRequirement( 'baz', true ); $featureManager->registerFeature( 'requiresFooBar', [ 'foo', 'bar' ] ); $featureManager->registerFeature( 'requiresBarBaz', [ 'bar', 'baz' ] ); $this->assertFalse( $featureManager->isFeatureEnabled( 'requiresFooBar' ), - 'A feature is disabled when at least one set that it requires is disabled.' + 'A feature is disabled when at least one requirement that it requires is disabled.' ); $this->assertTrue( $featureManager->isFeatureEnabled( 'requiresBarBaz' ) );