From 6b6bed86edea1d3a9609cf688370cd2e56410c95 Mon Sep 17 00:00:00 2001
From: Stephen Niedzielski <stephen@niedzielski.com>
Date: Wed, 25 Mar 2020 14:36:44 -0600
Subject: [PATCH] [docs] [dev] [PHP] [FeatureManager] revise docs and add
 DynamicConfigRequirement test

Address some feedback from I7a2cdc2dfdf20d78e4548f07cf53994563b234b3:

- Miscellaneous documentation improvements.
- Add a false case test to `DynamicConfigRequirement->isMet()`.

Bug: T244481
Change-Id: Ic5637f42da755f871c5a6d545e14effd3ac8c670
---
 includes/Constants.php                        |  1 +
 .../Requirements/DynamicConfigRequirement.php | 24 +++++++++++++++----
 includes/ServiceWiring.php                    | 12 +++++-----
 .../DynamicConfigRequirementTest.php          | 17 +++++++------
 4 files changed, 36 insertions(+), 18 deletions(-)

diff --git a/includes/Constants.php b/includes/Constants.php
index 4800b466..e1b0c228 100644
--- a/includes/Constants.php
+++ b/includes/Constants.php
@@ -62,6 +62,7 @@ final class Constants {
 
 	// These are used in the Feature Management System.
 	/**
+	 * Also known as `$wgFullyInitialised`. Set to true in core/includes/Setup.php.
 	 * @var string
 	 */
 	public const CONFIG_KEY_FULLY_INITIALISED = 'FullyInitialised';
diff --git a/includes/FeatureManagement/Requirements/DynamicConfigRequirement.php b/includes/FeatureManagement/Requirements/DynamicConfigRequirement.php
index 827aa76d..cc958104 100644
--- a/includes/FeatureManagement/Requirements/DynamicConfigRequirement.php
+++ b/includes/FeatureManagement/Requirements/DynamicConfigRequirement.php
@@ -25,7 +25,7 @@ namespace Vector\FeatureManagement\Requirements;
 use Vector\FeatureManagement\Requirement;
 
 /**
- * Some application state changes throughout the lifetime of the application, e.g.
+ * Some application state changes throughout the lifetime of the application, e.g. `wgSitename` or
  * `wgFullyInitialised`, which signals whether the application boot process has finished and
  * critical resources like database connections are available.
  *
@@ -36,14 +36,25 @@ use Vector\FeatureManagement\Requirement;
  * $featureManager->registerComplexRequirement(
  *   new DynamicConfigRequirement(
  *     $config,
- *     'FullyInitialised',
+ *     'Sitename',
  *     'Foo'
  *   )
  * );
  * ```
  *
  * registers a requirement that will evaluate to true only when `mediawiki/includes/Setup.php` has
- * finished executing (after all service wiring has executed).
+ * finished executing (after all service wiring has executed). I.e., every call to
+ * `Requirement->isMet()` reinterrogates the Config object for the current state and returns it.
+ * Contrast to
+ *
+ * ```lang=php
+ * $featureManager->registerRequirement(
+ *   'Foo',
+ *   $config->get( 'Sitename' )
+ * );
+ * ```
+ *
+ * wherein state is evaluated only once at registration time and permanently cached.
  *
  * 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
@@ -72,8 +83,11 @@ final class DynamicConfigRequirement implements Requirement {
 
 	/**
 	 * @param \Config $config
-	 * @param string $configName
-	 * @param string $requirementName The name of the requirement presented to the Feature Manager
+	 * @param string $configName Any `Config` key. This name is used to query `$config` state. E.g.,
+	 *   `'DBname'`. See https://www.mediawiki.org/wiki/Manual:Configuration_settings
+	 * @param string $requirementName The name of the requirement presented to FeatureManager.
+	 *   This name _usually_ matches the `$configName` parameter for simplicity but allows for
+	 *   abstraction as needed. See `Requirement->getName()`.
 	 */
 	public function __construct( \Config $config, string $configName, string $requirementName ) {
 		$this->config = $config;
diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php
index ba18933c..ea9b7f02 100644
--- a/includes/ServiceWiring.php
+++ b/includes/ServiceWiring.php
@@ -32,13 +32,13 @@ return [
 		return $services->getService( 'ConfigFactory' )->makeConfig( Constants::SKIN_NAME );
 	},
 	Constants::SERVICE_FEATURE_MANAGER => function ( MediaWikiServices $services ) {
-		$requirement = new DynamicConfigRequirement(
-			$services->getMainConfig(),
-			Constants::CONFIG_KEY_FULLY_INITIALISED,
-			Constants::REQUIREMENT_FULLY_INITIALISED
-		);
-
 		$featureManager = new FeatureManager();
+
+		$requirement = new DynamicConfigRequirement(
+				$services->getMainConfig(),
+				Constants::CONFIG_KEY_FULLY_INITIALISED,
+				Constants::REQUIREMENT_FULLY_INITIALISED
+			);
 		$featureManager->registerComplexRequirement( $requirement );
 
 		return $featureManager;
diff --git a/tests/phpunit/unit/FeatureManagement/Requirements/DynamicConfigRequirementTest.php b/tests/phpunit/unit/FeatureManagement/Requirements/DynamicConfigRequirementTest.php
index 154ebbe4..19d82c38 100644
--- a/tests/phpunit/unit/FeatureManagement/Requirements/DynamicConfigRequirementTest.php
+++ b/tests/phpunit/unit/FeatureManagement/Requirements/DynamicConfigRequirementTest.php
@@ -31,19 +31,22 @@ use Vector\FeatureManagement\Requirements\DynamicConfigRequirement;
  */
 class DynamicConfigRequirementTest extends \MediaWikiUnitTestCase {
 
+	public static function providesBooleanStates(): array {
+		return [ [ false ], [ true ] ];
+	}
+
 	/**
+	 * @dataProvider providesBooleanStates
 	 * @covers ::isMet
 	 */
-	public function testItFetchesAndReturnsConfigValue() {
-		$config = $this->createMock( \Config::class );
-		$config->expects( $this->once() )
-			->method( 'get' )
-			->with( 'Foo' )
-			->willReturn( true );
+	public function testItFetchesAndReturnsConfigValue( bool $configValue ) {
+		$config = new \HashConfig( [
+			'Foo' => $configValue,
+		] );
 
 		$requirement = new DynamicConfigRequirement( $config, 'Foo', 'Bar' );
 
-		$this->assertTrue( $requirement->isMet() );
+		$this->assertEquals( $requirement->isMet(), $configValue );
 	}
 
 	/**