From 8977e56a6b71e10e6046537cd82a3e32aea12f86 Mon Sep 17 00:00:00 2001 From: Kunal Mehta Date: Sat, 10 May 2014 01:19:00 -0700 Subject: [PATCH] Make abstract Config class truly implementation-agnostic Follow up to I13baec0b6 ("Config: Add Config and GlobalConfig classes"): Config: * Rather than returning Status objects, Config::set will now throw an exception if an error is encountered * Config::factory was moved into it's own ConfigFactory class. * Since there are no more functions in it, Config was turned into an interface. GlobalConfig: * Remove $prefix args from Config::set and ::get. The idea of having an abstract Config class is to abstract some notion of configuration data from the particular way in which it is currently implemented (global variables). So the abstract base class has no business dealing with variable name prefixes. ** Instead GlobalVarConfig's implementations of get and set call getWithPrefix and setWithPrefix internally, which are now protected * Rename GlobalConfig to GlobalVarConfig, which makes it clearer that it isn't referring to the scope of the configuration value, but to the scope of the variable name which provides it. ConfigFactory: * ConfigFactory is where Config objects are registered, and later constructed. * Config objects are registered with a given name, and a callback factory function. This allows for implementations to construct the object with the parameters they want, and avoids the overhead of needing an entire class. ** The name 'main' is the default object returned by RequestContext::getConfig(), and is intended to be used by core. * This is a singleton class, the main instance can be obtained with: ConfigFactory::getDefaultInstance() In addition to the above: * $wgConfigClass was removed, and $wgConfigRegistry was introduced, which stores a name => callback. The name is to be what the Config instance is registered with, and the callback should return an implementation of Config. * Tests were written for the new ConfigFactory, and GlobalVarConfig's tests were improved. Co-Authored-By: Ori Livneh Co-Authored-By: Chad Horohoe Co-Authored-By: Mattflaschen Co-Authored-By: Parent5446 Co-Authored-By: Reedy Co-Authored-By: Daniel Kinzler Change-Id: I5a5857fcfa07598ba4ce9ae5bbb4ce54a567d31e --- includes/AutoLoader.php | 4 +- includes/DefaultSettings.php | 9 +- includes/config/Config.php | 38 +++----- .../{GlobalConfig.php => ConfigException.php} | 21 +---- includes/config/ConfigFactory.php | 94 +++++++++++++++++++ includes/config/GlobalVarConfig.php | 88 +++++++++++++++++ includes/context/RequestContext.php | 4 +- .../includes/config/ConfigFactoryTest.php | 46 +++++++++ .../includes/config/GlobalConfigTest.php | 38 -------- .../includes/config/GlobalVarConfigTest.php | 36 +++++++ 10 files changed, 292 insertions(+), 86 deletions(-) rename includes/config/{GlobalConfig.php => ConfigException.php} (69%) create mode 100644 includes/config/ConfigFactory.php create mode 100644 includes/config/GlobalVarConfig.php create mode 100644 tests/phpunit/includes/config/ConfigFactoryTest.php delete mode 100644 tests/phpunit/includes/config/GlobalConfigTest.php create mode 100644 tests/phpunit/includes/config/GlobalVarConfigTest.php diff --git a/includes/AutoLoader.php b/includes/AutoLoader.php index d3aabfe050..3ab2f009ac 100644 --- a/includes/AutoLoader.php +++ b/includes/AutoLoader.php @@ -391,7 +391,9 @@ $wgAutoloadLocalClasses = array( # includes/config 'Config' => 'includes/config/Config.php', - 'GlobalConfig' => 'includes/config/GlobalConfig.php', + 'ConfigException' => 'includes/config/ConfigException.php', + 'ConfigFactory' => 'includes/config/ConfigFactory.php', + 'GlobalVarConfig' => 'includes/config/GlobalVarConfig.php', # includes/content 'AbstractContent' => 'includes/content/AbstractContent.php', diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php index 87dc0255d3..4598952ed0 100644 --- a/includes/DefaultSettings.php +++ b/includes/DefaultSettings.php @@ -60,11 +60,14 @@ if ( !defined( 'MEDIAWIKI' ) ) { $wgConf = new SiteConfiguration; /** - * Class name to use for accessing Config. - * Currently only 'GlobalConfig' is available + * Registry of factory functions to create config objects: + * The 'main' key must be set, and the value should be a valid + * callable. * @since 1.23 */ -$wgConfigClass = 'GlobalConfig'; +$wgConfigRegistry = array( + 'main' => 'GlobalVarConfig::newInstance' +); /** * MediaWiki version number diff --git a/includes/config/Config.php b/includes/config/Config.php index 04afddad14..68e90b4582 100644 --- a/includes/config/Config.php +++ b/includes/config/Config.php @@ -21,37 +21,27 @@ */ /** - * Abstract class for get settings for + * Interface for configuration instances * * @since 1.23 */ -abstract class Config { - /** - * @param string $name configuration variable name without prefix - * @param string $prefix of the variable name - * @return mixed - */ - abstract public function get( $name, $prefix = 'wg' ); +interface Config { /** - * @param string $name configuration variable name without prefix - * @param mixed $value to set - * @param string $prefix of the variable name - * @return Status object indicating success or failure + * Get a configuration variable such as "Sitename" or "UploadMaintenance." + * + * @param string $name Name of configuration option + * @return mixed Value configured + * @throws ConfigException */ - abstract public function set( $name, $value, $prefix = 'wg' ); + public function get( $name ); /** - * @param string|null $type class name for Config object, - * uses $wgConfigClass if not provided - * @return Config + * Set a configuration variable such a "Sitename" to something like "My Wiki" + * + * @param string $name Name of configuration option + * @param mixed $value Value to set + * @throws ConfigException */ - public static function factory( $type = null ) { - if ( !$type ) { - global $wgConfigClass; - $type = $wgConfigClass; - } - - return new $type; - } + public function set( $name, $value ); } diff --git a/includes/config/GlobalConfig.php b/includes/config/ConfigException.php similarity index 69% rename from includes/config/GlobalConfig.php rename to includes/config/ConfigException.php index e16a9ee728..368ca5a465 100644 --- a/includes/config/GlobalConfig.php +++ b/includes/config/ConfigException.php @@ -21,25 +21,8 @@ */ /** - * Accesses configuration settings from $GLOBALS + * Exceptions for config failures * * @since 1.23 */ -class GlobalConfig extends Config { - - /** - * @see Config::get - */ - public function get( $name, $prefix = 'wg' ) { - return $GLOBALS[$prefix . $name]; - } - - /** - * @see Config::set - */ - public function set( $name, $value, $prefix = 'wg' ) { - $GLOBALS[$prefix . $name] = $value; - - return Status::newGood(); - } -} +class ConfigException extends MWException {} diff --git a/includes/config/ConfigFactory.php b/includes/config/ConfigFactory.php new file mode 100644 index 0000000000..b09316ba3a --- /dev/null +++ b/includes/config/ConfigFactory.php @@ -0,0 +1,94 @@ + callback + * @var array + */ + protected $factoryFunctions = array(); + + /** + * Config objects that have already been created + * name => Config object + * @var array + */ + protected $configs = array(); + + public static function getDefaultInstance() { + static $self = null; + if ( !$self ) { + $self = new self; + global $wgConfigRegistry; + foreach ( $wgConfigRegistry as $name => $callback ) { + $self->register( $name, $callback ); + } + } + return $self; + } + + /** + * Register a new config factory function + * Will override if it's already registered + * @param string $name + * @param callable $callback that takes this ConfigFactory as an argument + * @throws InvalidArgumentException if an invalid callback is provided + */ + public function register( $name, $callback ) { + if ( !is_callable( $callback ) ) { + throw new InvalidArgumentException( 'Invalid callback provided' ); + } + $this->factoryFunctions[$name] = $callback; + } + + /** + * Create a given Config using the registered callback for $name. + * If an object was already created, the same Config object is returned. + * @param string $name of the extension/component you want a Config object for + * 'main' is used for core + * @throws ConfigException if a factory function isn't registered for $name + * @throws UnexpectedValueException if the factory function returns a non-Config object + * @return Config + */ + public function makeConfig( $name ) { + if ( !isset( $this->configs[$name] ) ) { + if ( !isset( $this->factoryFunctions[$name] ) ) { + throw new ConfigException( "No registered builder available for $name." ); + } + $conf = call_user_func( $this->factoryFunctions[$name], $this ); + if ( $conf instanceof Config ) { + $this->configs[$name] = $conf; + } else { + throw new UnexpectedValueException( "The builder for $name returned a non-Config object." ); + } + } + + return $this->configs[$name]; + } +} diff --git a/includes/config/GlobalVarConfig.php b/includes/config/GlobalVarConfig.php new file mode 100644 index 0000000000..61a76b7617 --- /dev/null +++ b/includes/config/GlobalVarConfig.php @@ -0,0 +1,88 @@ +prefix = $prefix; + } + + /** + * @see Config::get + */ + public function get( $name ) { + return $this->getWithPrefix( $this->prefix, $name ); + } + + /** + * @see Config::set + */ + public function set( $name, $value ) { + $this->setWithPrefix( $this->prefix, $name, $value ); + } + + /** + * Get a variable with a given prefix, if not the defaults. + * + * @param string $prefix Prefix to use on the variable, if one. + * @param string $name Variable name without prefix + * @throws ConfigException + * @return mixed + */ + protected function getWithPrefix( $prefix, $name ) { + $var = $prefix . $name; + if ( !isset( $GLOBALS[ $var ] ) ) { + throw new ConfigException( __METHOD__ . ": undefined variable: '$var'" ); + } + return $GLOBALS[ $var ]; + } + + /** + * Get a variable with a given prefix, if not the defaults. + * + * @param string $prefix Prefix to use on the variable + * @param string $name Variable name without prefix + * @param mixed $value value to set + */ + protected function setWithPrefix( $prefix, $name, $value ) { + $GLOBALS[ $prefix . $name ] = $value; + } +} diff --git a/includes/context/RequestContext.php b/includes/context/RequestContext.php index 6f27a7ae82..e035b49194 100644 --- a/includes/context/RequestContext.php +++ b/includes/context/RequestContext.php @@ -84,7 +84,9 @@ class RequestContext implements IContextSource { */ public function getConfig() { if ( $this->config === null ) { - $this->config = Config::factory(); + // @todo In the future, we could move this to WebStart.php so + // the Config object is ready for when initialization happens + $this->config = ConfigFactory::getDefaultInstance()->makeConfig( 'main' ); } return $this->config; diff --git a/tests/phpunit/includes/config/ConfigFactoryTest.php b/tests/phpunit/includes/config/ConfigFactoryTest.php new file mode 100644 index 0000000000..0a6bf723f8 --- /dev/null +++ b/tests/phpunit/includes/config/ConfigFactoryTest.php @@ -0,0 +1,46 @@ +register( 'unittest', 'GlobalVarConfig::newInstance' ); + $this->assertTrue( True ); // No exception thrown + $this->setExpectedException( 'InvalidArgumentException' ); + $factory->register( 'invalid', 'Invalid callback' ); + } + + /** + * @covers ConfigFactory::makeConfig + */ + public function testMakeConfig() { + $factory = new ConfigFactory(); + $factory->register( 'unittest', 'GlobalVarConfig::newInstance' ); + $conf = $factory->makeConfig( 'unittest' ); + $this->assertInstanceOf( 'Config', $conf ); + } + + /** + * @covers ConfigFactory::makeConfig + */ + public function testMakeConfigWithNoBuilders() { + $factory = new ConfigFactory(); + $this->setExpectedException( 'ConfigException' ); + $factory->makeConfig( 'nobuilderregistered' ); + } + + /** + * @covers ConfigFactory::makeConfig + */ + public function testMakeConfigWithInvalidCallback() { + $factory = new ConfigFactory(); + $factory->register( 'unittest', function() { + return true; // Not a Config object + }); + $this->setExpectedException( 'UnexpectedValueException' ); + $factory->makeConfig( 'unittest' ); + } +} diff --git a/tests/phpunit/includes/config/GlobalConfigTest.php b/tests/phpunit/includes/config/GlobalConfigTest.php deleted file mode 100644 index b605a46eb2..0000000000 --- a/tests/phpunit/includes/config/GlobalConfigTest.php +++ /dev/null @@ -1,38 +0,0 @@ -config = new GlobalConfig; - } - - public static function provideGet() { - return array( - array( 'wgSitename', array( 'Sitename' ) ), - array( 'wgFoo', array( 'Foo' ) ), - array( 'efVariable', array( 'Variable', 'ef' ) ), - array( 'Foo', array( 'Foo', '' ) ), - ); - } - - /** - * @param string $name - * @param array $params - * @dataProvider provideGet - * @covers GlobalConfig::get - */ - public function testGet( $name, $params ) { - $rand = wfRandom(); - $old = isset( $GLOBALS[$name] ) ? $GLOBALS[$name] : null; - $GLOBALS[$name] = $rand; - $out = call_user_func_array( array( $this->config, 'get' ), $params ); - $this->assertEquals( $rand, $out ); - if ( $old ) { - $GLOBALS[$name] = $old; - } - } -} diff --git a/tests/phpunit/includes/config/GlobalVarConfigTest.php b/tests/phpunit/includes/config/GlobalVarConfigTest.php new file mode 100644 index 0000000000..7080b02335 --- /dev/null +++ b/tests/phpunit/includes/config/GlobalVarConfigTest.php @@ -0,0 +1,36 @@ + 'default1', + 'wgFoo' => 'default2', + 'efVariable' => 'default3', + 'BAR' => 'default4', + ); + + foreach ( $set as $var => $value ) { + $GLOBALS[$var] = $value; + } + + return array( + array( 'Something', 'wg', 'default1' ), + array( 'Foo', 'wg', 'default2' ), + array( 'Variable', 'ef', 'default3' ), + array( 'BAR', '', 'default4' ), + ); + } + + /** + * @param string $name + * @param string $prefix + * @param string $expected + * @dataProvider provideGet + * @covers GlobalVarConfig::get + */ + public function testGet( $name, $prefix, $expected ) { + $config = new GlobalVarConfig( $prefix ); + $this->assertEquals( $config->get( $name ), $expected ); + } +} -- 2.20.1