From 84226b41b67f93fb2a1898ed140ebc97b6a3b5b9 Mon Sep 17 00:00:00 2001 From: Sam Smith Date: Fri, 13 Mar 2020 17:22:59 +0000 Subject: [PATCH] featureManager: Add DynamicConfigRequirement requirement We expect the vast majority of requirements and features to be defined in services as possible. However, there are some "complex" requirements that require additional application/HTTP request state. Unfortunately, service wiring is done before some of that state is available. I65702426 attempted to work around this by requiring clients of the Feature Manager to pass that additional state on every interaction with the system. Those complex requirements would then select the parts of the state that they required when it was required. However implementations of \IContextSource are God objects and their use should be limited. Whilst reviewing I65702426, Stephen Niedzielski mentioned that the application state being available is a requirement. This remarkably simple solution: - Keeps the Requirement interface and FeatureManager API free of God objects; - Is true to the nature of the Feature Manager - it makes clear and centralizes the various checks for application state being available across the codebase; and - Inject a Requirement implementations' dependencies at construction time It just so happens that the $wgFullyInitialised variable flags whether the application state is available... Changes: - Add the the FeatureManager\Requirements\DynamicConfigRequirement class and tests. The DynamicConfigRequirement lazily evaluates a single configuration value whenever ::isMet is invoked - Register an DynamicConfigRequirement instance, configured to evaluate $wgFullyInitialised while constructing the Vector.FeatureManager service Bug: T244481 Change-Id: I7a2cdc2dfdf20d78e4548f07cf53994563b234b3 --- includes/Constants.php | 11 +++ .../Requirements/DynamicConfigRequirement.php | 97 +++++++++++++++++++ includes/FeatureManagement/TODO.md | 28 +----- includes/ServiceWiring.php | 12 ++- .../DynamicConfigRequirementTest.php | 74 ++++++++++++++ 5 files changed, 196 insertions(+), 26 deletions(-) create mode 100644 includes/FeatureManagement/Requirements/DynamicConfigRequirement.php create mode 100644 tests/phpunit/unit/FeatureManagement/Requirements/DynamicConfigRequirementTest.php diff --git a/includes/Constants.php b/includes/Constants.php index 10bb7ccc..4800b466 100644 --- a/includes/Constants.php +++ b/includes/Constants.php @@ -60,6 +60,17 @@ final class Constants { */ public const PREF_KEY_SKIN_VERSION = 'VectorSkinVersion'; + // These are used in the Feature Management System. + /** + * @var string + */ + public const CONFIG_KEY_FULLY_INITIALISED = 'FullyInitialised'; + + /** + * @var string + */ + public const REQUIREMENT_FULLY_INITIALISED = 'FullyInitialised'; + /** * This class is for namespacing constants only. Forbid construction. * @throws FatalError diff --git a/includes/FeatureManagement/Requirements/DynamicConfigRequirement.php b/includes/FeatureManagement/Requirements/DynamicConfigRequirement.php new file mode 100644 index 00000000..827aa76d --- /dev/null +++ b/includes/FeatureManagement/Requirements/DynamicConfigRequirement.php @@ -0,0 +1,97 @@ +registerComplexRequirement( + * new DynamicConfigRequirement( + * $config, + * 'FullyInitialised', + * 'Foo' + * ) + * ); + * ``` + * + * registers a requirement that will evaluate to true only when `mediawiki/includes/Setup.php` has + * finished executing (after all service wiring has executed). + * + * NOTE: This API hasn't settled. It may change at any time without warning. Please don't bind to + * it unless you absolutely need to + * + * @unstable + * + * @package FeatureManagement + * @internal + */ +final class DynamicConfigRequirement implements Requirement { + + /** + * @var \Config + */ + private $config; + + /** + * @var string + */ + private $configName; + + /** + * @var string + */ + private $requirementName; + + /** + * @param \Config $config + * @param string $configName + * @param string $requirementName The name of the requirement presented to the Feature Manager + */ + public function __construct( \Config $config, string $configName, string $requirementName ) { + $this->config = $config; + $this->configName = $configName; + $this->requirementName = $requirementName; + } + + /** + * @inheritDoc + */ + public function getName() : string { + return $this->requirementName; + } + + /** + * @inheritDoc + */ + public function isMet() : bool { + return (bool)$this->config->get( $this->configName ); + } +} diff --git a/includes/FeatureManagement/TODO.md b/includes/FeatureManagement/TODO.md index 0279e9f9..017407d2 100644 --- a/includes/FeatureManagement/TODO.md +++ b/includes/FeatureManagement/TODO.md @@ -6,28 +6,6 @@ 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~~ -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->registerRequirement( Constants::LOGGED_IN_REQ, function( IContextSource $context ) { - $user = $context->getUser(); - - return $user - && $user->isSafeToLoad() - && $user->isLoggedIn(); -} ); - -$featureManager->registerRequirement( Constants::MAINSPACE_REQ, function ( IContextSource $context ) { - $title = $context->getTitle(); - - return $title && $title->inNamespace( NS_MAIN ); -} ); -``` - -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 +1. Consider supporing memoization of those requirements (see https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/Vector/+/573626/7/includes/FeatureManagement/FeatureManager.php@68) +2. Add support for getting all requirements +3. Add support for getting all features enabled when a requirement is enabled/disabled diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index 0366cbeb..ba18933c 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -25,12 +25,22 @@ use MediaWiki\MediaWikiServices; use Vector\Constants; use Vector\FeatureManagement\FeatureManager; +use Vector\FeatureManagement\Requirements\DynamicConfigRequirement; return [ Constants::SERVICE_CONFIG => function ( MediaWikiServices $services ) { return $services->getService( 'ConfigFactory' )->makeConfig( Constants::SKIN_NAME ); }, Constants::SERVICE_FEATURE_MANAGER => function ( MediaWikiServices $services ) { - return new FeatureManager(); + $requirement = new DynamicConfigRequirement( + $services->getMainConfig(), + Constants::CONFIG_KEY_FULLY_INITIALISED, + Constants::REQUIREMENT_FULLY_INITIALISED + ); + + $featureManager = new FeatureManager(); + $featureManager->registerComplexRequirement( $requirement ); + + return $featureManager; } ]; diff --git a/tests/phpunit/unit/FeatureManagement/Requirements/DynamicConfigRequirementTest.php b/tests/phpunit/unit/FeatureManagement/Requirements/DynamicConfigRequirementTest.php new file mode 100644 index 00000000..154ebbe4 --- /dev/null +++ b/tests/phpunit/unit/FeatureManagement/Requirements/DynamicConfigRequirementTest.php @@ -0,0 +1,74 @@ +createMock( \Config::class ); + $config->expects( $this->once() ) + ->method( 'get' ) + ->with( 'Foo' ) + ->willReturn( true ); + + $requirement = new DynamicConfigRequirement( $config, 'Foo', 'Bar' ); + + $this->assertTrue( $requirement->isMet() ); + } + + /** + * @covers ::isMet + */ + public function testItCastsConfigValue() { + $config = new \HashConfig( [ + 'Foo' => new \stdClass(), + ] ); + + $requirement = new DynamicConfigRequirement( $config, 'Foo', 'Bar' ); + + $this->assertTrue( $requirement->isMet() ); + } + + /** + * @covers ::getName + */ + public function testItReturnsName() { + $requirement = new DynamicConfigRequirement( + new \HashConfig(), + 'Foo', + 'Bar' + ); + + $this->assertEquals( 'Bar', $requirement->getName() ); + } +}