From d14caf2f110d36c6a5926cec84da501cd9204482 Mon Sep 17 00:00:00 2001 From: Sam Smith Date: Mon, 2 Mar 2020 12:39:47 +0000 Subject: [PATCH] featureManager: Add Requirement interface FeatureManager::registerRequirement established the interface for a requirement: its name and whether it's met. However, the Feature Manager also needs to handle scenarios where a requirement needs additional context before it can be considered met. That context may not be available when the application is booting, e.g. checking if the user is logged in; or the logic is complicated enough that it should be under test. Changes: - Add the Requirement interface and update FeatureManager to work with implementations of it - Maintain B/C by constructing an instance of a the SimpleRequirement DTO Bug: T244481 Change-Id: Id95d9e5d7125492968d0e15515224aadbc3075f8 --- includes/FeatureManagement/FeatureManager.php | 51 ++++++++++---- includes/FeatureManagement/Requirement.php | 45 ++++++++++++ .../FeatureManagement/SimpleRequirement.php | 68 +++++++++++++++++++ .../FeatureManagement/FeatureManagerTest.php | 1 + 4 files changed, 150 insertions(+), 15 deletions(-) create mode 100644 includes/FeatureManagement/Requirement.php create mode 100644 includes/FeatureManagement/SimpleRequirement.php diff --git a/includes/FeatureManagement/FeatureManager.php b/includes/FeatureManagement/FeatureManager.php index 0f250de..7fc5517 100644 --- a/includes/FeatureManagement/FeatureManager.php +++ b/includes/FeatureManagement/FeatureManager.php @@ -38,8 +38,8 @@ use Wikimedia\Assert\Assert; final class FeatureManager { /** - * A map of feature name to the array of requirements. A feature is only considered enabled when - * all of its requirements are met. + * A map of feature name to the array of requirements (referenced by name). A feature is only + * considered enabled when all of its requirements are met. * * See FeatureManager::registerFeature for additional detail. * @@ -48,9 +48,12 @@ final class FeatureManager { private $features = []; /** - * A map of requirement name to whether the requirement is met. + * A map of requirement name to the Requirement instance that represents it. * - * @var Array + * The names of requirements are assumed to be static for the lifetime of the request. Therefore + * we can use them to look up Requirement instances quickly. + * + * @var Array */ private $requirements = []; @@ -138,7 +141,7 @@ final class FeatureManager { $requirements = $this->features[$feature]; foreach ( $requirements as $name ) { - if ( !$this->requirements[$name] ) { + if ( !$this->requirements[$name]->isMet() ) { return false; } } @@ -146,16 +149,38 @@ final class FeatureManager { return true; } + /** + * Register a complex requirement. + * + * A complex requirement is one that depends on object that may or may not be fully loaded + * while the application is booting, e.g. see `User::isSafeToLoad`. + * + * Such requirements are expected to be registered during a hook that is run early on in the + * application lifecycle, e.g. the `BeforePerformAction` and `APIBeforeMain` hooks. + * + * @see FeatureManager::registerRequirement + * + * @param Requirement $requirement + * + * @throws \LogicException If the requirement has already been registered + */ + public function registerComplexRequirement( Requirement $requirement ) { + $name = $requirement->getName(); + + if ( array_key_exists( $name, $this->requirements ) ) { + throw new \LogicException( "The requirement \"{$name}\" is already registered." ); + } + + $this->requirements[$name] = $requirement; + } + /** * Register a requirement. * * 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 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. + * @see FeatureManager::registerComplexRequirement * * @param string $name The name of the requirement * @param bool $isMet Whether the requirement is met @@ -163,11 +188,7 @@ final class FeatureManager { * @throws \LogicException If the requirement has already been registered */ public function registerRequirement( string $name, bool $isMet ) { - if ( array_key_exists( $name, $this->requirements ) ) { - throw new \LogicException( "The requirement \"{$name}\" is already registered." ); - } - - $this->requirements[$name] = $isMet; + $this->registerComplexRequirement( new SimpleRequirement( $name, $isMet ) ); } /** @@ -183,6 +204,6 @@ final class FeatureManager { throw new \InvalidArgumentException( "Requirement \"{$name}\" isn't registered." ); } - return $this->requirements[$name]; + return $this->requirements[$name]->isMet(); } } diff --git a/includes/FeatureManagement/Requirement.php b/includes/FeatureManagement/Requirement.php new file mode 100644 index 0000000..281ae66 --- /dev/null +++ b/includes/FeatureManagement/Requirement.php @@ -0,0 +1,45 @@ +name = $name; + $this->isMet = $isMet; + } + + /** + * @inheritDoc + */ + public function getName() : string { + return $this->name; + } + + /** + * @inheritDoc + */ + public function isMet() : bool { + return $this->isMet; + } +} diff --git a/tests/phpunit/unit/FeatureManagement/FeatureManagerTest.php b/tests/phpunit/unit/FeatureManagement/FeatureManagerTest.php index ff613d3..d97b5bc 100644 --- a/tests/phpunit/unit/FeatureManagement/FeatureManagerTest.php +++ b/tests/phpunit/unit/FeatureManagement/FeatureManagerTest.php @@ -32,6 +32,7 @@ class FeatureManagerTest extends \MediaWikiUnitTestCase { /** * @covers ::registerRequirement + * @covers ::registerComplexRequirement */ public function testRegisterRequirementThrowsWhenRequirementIsRegisteredTwice() { $this->expectException( \LogicException::class );