From 5484283a25d666fa1fb658da1a2bcfe6db3b8730 Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Mon, 1 Jul 2013 15:25:21 +1200 Subject: [PATCH] FIX changing environment in config.php changes matched yaml rules --- core/Config.php | 13 ++- core/manifest/ConfigManifest.php | 94 ++++++++++++++++--- tests/core/manifest/ConfigManifestTest.php | 79 ++++++++++++---- .../mysite/_config/envrules.yml | 42 +++++++++ .../mysite/_config.php | 4 + .../mysite/_config/environment.yml | 18 ++++ 6 files changed, 215 insertions(+), 35 deletions(-) create mode 100644 tests/core/manifest/fixtures/configmanifest/mysite/_config/envrules.yml create mode 100644 tests/core/manifest/fixtures/configmanifest_dynamicenv/mysite/_config.php create mode 100644 tests/core/manifest/fixtures/configmanifest_dynamicenv/mysite/_config/environment.yml diff --git a/core/Config.php b/core/Config.php index 10c00ed3c..752e73c27 100644 --- a/core/Config.php +++ b/core/Config.php @@ -295,8 +295,12 @@ class Config { * instead, the last manifest to be added always wins */ public function pushConfigYamlManifest(SS_ConfigManifest $manifest) { - array_unshift($this->manifests, $manifest->yamlConfig); + array_unshift($this->manifests, $manifest); + + // Now that we've got another yaml config manifest we need to clean the cache $this->cache->clean(); + // We also need to clean the cache if the manifest's calculated config values change + $manifest->registerChangeCallback(array($this->cache, 'clean')); // @todo: Do anything with these. They're for caching after config.php has executed $this->collectConfigPHPSettings = true; @@ -479,10 +483,13 @@ class Config { } } + $value = $nothing = null; + // Then the manifest values foreach($this->manifests as $manifest) { - if (isset($manifest[$class][$name])) { - self::merge_low_into_high($result, $manifest[$class][$name], $suppress); + $value = $manifest->get($class, $name, $nothing); + if ($value !== $nothing) { + self::merge_low_into_high($result, $value, $suppress); if ($result !== null && !is_array($result)) return $result; } } diff --git a/core/manifest/ConfigManifest.php b/core/manifest/ConfigManifest.php index 332f838c7..1398518aa 100644 --- a/core/manifest/ConfigManifest.php +++ b/core/manifest/ConfigManifest.php @@ -9,7 +9,16 @@ */ class SS_ConfigManifest { - /** + /** @var string - The base path used when building the manifest */ + protected $base; + + /** @var string - A string to prepend to all cache keys to ensure all keys are unique to just this $base */ + protected $key; + + /** @var bool - Whether `test` directories should be searched when searching for configuration */ + protected $includeTests; + + /** * All the values needed to be collected to determine the correct combination of fragements for * the current environment. * @var array @@ -34,6 +43,17 @@ class SS_ConfigManifest { */ public $yamlConfig = array(); + /** + * The variant key state as when yamlConfig was loaded + * @var string + */ + protected $yamlConfigVariantKey = null; + + /** + * @var [callback] A list of callbacks to be called whenever the content of yamlConfig changes + */ + protected $configChangeCallbacks = array(); + /** * A side-effect of collecting the _config fragments is the calculation of all module directories, since * the definition of a module is "a directory that contains either a _config.php file or a _config directory @@ -65,6 +85,7 @@ class SS_ConfigManifest { public function __construct($base, $includeTests = false, $forceRegen = false ) { $this->base = $base; $this->key = sha1($base).'_'; + $this->includeTests = $includeTests; // Get the Zend Cache to load/store cache into $this->cache = SS_Cache::factory('SS_Configuration', 'Core', array( @@ -78,21 +99,28 @@ class SS_ConfigManifest { $this->phpConfigSources = $this->cache->load($this->key.'php_config_sources'); // Get the variant key spec $this->variantKeySpec = $this->cache->load($this->key.'variant_key_spec'); - // Try getting the pre-filtered & merged config for this variant - if (!($this->yamlConfig = $this->cache->load($this->key.'yaml_config_'.$this->variantKey()))) { - // Otherwise, if we do have the yaml config fragments (and we should since we have a variant key spec) - // work out the config for this variant - if ($this->yamlConfigFragments = $this->cache->load($this->key.'yaml_config_fragments')) { - $this->buildYamlConfigVariant(); - } - } } - // If we don't have a config yet, we need to do a full regen to get it - if (!$this->yamlConfig) { + // If we don't have a variantKeySpec (because we're forcing regen, or it just wasn't in the cache), generate it + if (!$this->variantKeySpec) { $this->regenerate($includeTests); - $this->buildYamlConfigVariant(); } + + // At this point $this->variantKeySpec will always contain something valid, so we can build the variant + $this->buildYamlConfigVariant(); + } + + /** + * Register a callback to be called whenever the calculated merged config changes + * + * In some situations the merged config can change - for instance, code in _config.php can cause which Only + * and Except fragments match. Registering a callback with this function allows code to be called when + * this happens. + * + * @param callback $callback + */ + public function registerChangeCallback($callback) { + $this->configChangeCallbacks[] = $callback; } /** @@ -103,6 +131,22 @@ class SS_ConfigManifest { foreach ($this->phpConfigSources as $config) { require_once $config; } + + if ($this->variantKey() != $this->yamlConfigVariantKey) $this->buildYamlConfigVariant(); + } + + /** + * Gets the (merged) config value for the given class and config property name + * + * @param string $class - The class to get the config property value for + * @param string $name - The config property to get the value for + * @param any $default - What to return if no value was contained in any YAML file for the passed $class and $name + * @return any - The merged set of all values contained in all the YAML configuration files for the passed + * $class and $name, or $default if there are none + */ + public function get($class, $name, $default=null) { + if (isset($this->yamlConfig[$class][$name])) return $this->yamlConfig[$class][$name]; + else return $default; } /** @@ -493,9 +537,32 @@ class SS_ConfigManifest { /** * Calculates which yaml config fragments are applicable in this variant, and merge those all together into * the $this->yamlConfig propperty + * + * Checks cache and takes care of loading yamlConfigFragments if they aren't already present, but expects + * $variantKeySpec to already be set */ public function buildYamlConfigVariant($cache = true) { + // Only try loading from cache if we don't have the fragments already loaded, as there's no way to know if a + // given variant is stale compared to the complete set of fragments + if (!$this->yamlConfigFragments) { + // First try and just load the exact variant + if ($this->yamlConfig = $this->cache->load($this->key.'yaml_config_'.$this->variantKey())) { + $this->yamlConfigVariantKey = $this->variantKey(); + return; + } + // Otherwise try and load the fragments so we can build the variant + else { + $this->yamlConfigFragments = $this->cache->load($this->key.'yaml_config_fragments'); + } + } + + // If we still don't have any fragments we have to build them + if (!$this->yamlConfigFragments) { + $this->regenerate($this->includeTests, $cache); + } + $this->yamlConfig = array(); + $this->yamlConfigVariantKey = $this->variantKey(); foreach ($this->yamlConfigFragments as $i => $fragment) { $matches = true; @@ -514,6 +581,9 @@ class SS_ConfigManifest { if ($cache) { $this->cache->save($this->yamlConfig, $this->key.'yaml_config_'.$this->variantKey()); } + + // Since yamlConfig has changed, call any callbacks that are interested + foreach ($this->configChangeCallbacks as $callback) call_user_func($callback); } /** diff --git a/tests/core/manifest/ConfigManifestTest.php b/tests/core/manifest/ConfigManifestTest.php index 1250adbdc..f11c022d7 100644 --- a/tests/core/manifest/ConfigManifestTest.php +++ b/tests/core/manifest/ConfigManifestTest.php @@ -8,65 +8,65 @@ class ConfigManifestTest_ConfigManifestAccess extends SS_ConfigManifest { class ConfigManifestTest extends SapphireTest { - protected function getConfigFixture() { + protected function getConfigFixtureValue($name) { $manifest = new SS_ConfigManifest(dirname(__FILE__).'/fixtures/configmanifest', true, true); - return $manifest->yamlConfig; + return $manifest->get('ConfigManifestTest', $name); } public function testClassRules() { - $config = $this->getConfigFixture(); + $config = $this->getConfigFixtureValue('Class'); $this->assertEquals( - 'Yes', @$config['ConfigManifestTest']['Class']['DirectorExists'], + 'Yes', @$config['DirectorExists'], 'Only rule correctly detects existing class' ); $this->assertEquals( - 'No', @$config['ConfigManifestTest']['Class']['NoSuchClassExists'], + 'No', @$config['NoSuchClassExists'], 'Except rule correctly detects missing class' ); } public function testModuleRules() { - $config = $this->getConfigFixture(); + $config = $this->getConfigFixtureValue('Module'); $this->assertEquals( - 'Yes', @$config['ConfigManifestTest']['Module']['MysiteExists'], + 'Yes', @$config['MysiteExists'], 'Only rule correctly detects existing module' ); $this->assertEquals( - 'No', @$config['ConfigManifestTest']['Module']['NoSuchModuleExists'], + 'No', @$config['NoSuchModuleExists'], 'Except rule correctly detects missing module' ); } public function testEnvVarSetRules() { $_ENV['EnvVarSet_Foo'] = 1; - $config = $this->getConfigFixture(); + $config = $this->getConfigFixtureValue('EnvVarSet'); $this->assertEquals( - 'Yes', @$config['ConfigManifestTest']['EnvVarSet']['FooSet'], + 'Yes', @$config['FooSet'], 'Only rule correctly detects set environment variable' ); $this->assertEquals( - 'No', @$config['ConfigManifestTest']['EnvVarSet']['BarSet'], + 'No', @$config['BarSet'], 'Except rule correctly detects unset environment variable' ); } public function testConstantDefinedRules() { define('ConstantDefined_Foo', 1); - $config = $this->getConfigFixture(); + $config = $this->getConfigFixtureValue('ConstantDefined'); $this->assertEquals( - 'Yes', @$config['ConfigManifestTest']['ConstantDefined']['FooDefined'], + 'Yes', @$config['FooDefined'], 'Only rule correctly detects defined constant' ); $this->assertEquals( - 'No', @$config['ConfigManifestTest']['ConstantDefined']['BarDefined'], + 'No', @$config['BarDefined'], 'Except rule correctly detects undefined constant' ); } @@ -74,34 +74,73 @@ class ConfigManifestTest extends SapphireTest { public function testEnvOrConstantMatchesValueRules() { $_ENV['EnvOrConstantMatchesValue_Foo'] = 'Foo'; define('EnvOrConstantMatchesValue_Bar', 'Bar'); - $config = $this->getConfigFixture(); + $config = $this->getConfigFixtureValue('EnvOrConstantMatchesValue'); $this->assertEquals( - 'Yes', @$config['ConfigManifestTest']['EnvOrConstantMatchesValue']['FooIsFoo'], + 'Yes', @$config['FooIsFoo'], 'Only rule correctly detects environment variable matches specified value' ); $this->assertEquals( - 'Yes', @$config['ConfigManifestTest']['EnvOrConstantMatchesValue']['BarIsBar'], + 'Yes', @$config['BarIsBar'], 'Only rule correctly detects constant matches specified value' ); $this->assertEquals( - 'No', @$config['ConfigManifestTest']['EnvOrConstantMatchesValue']['FooIsQux'], + 'No', @$config['FooIsQux'], 'Except rule correctly detects environment variable that doesn\'t match specified value' ); $this->assertEquals( - 'No', @$config['ConfigManifestTest']['EnvOrConstantMatchesValue']['BarIsQux'], + 'No', @$config['BarIsQux'], 'Except rule correctly detects environment variable that doesn\'t match specified value' ); $this->assertEquals( - 'No', @$config['ConfigManifestTest']['EnvOrConstantMatchesValue']['BazIsBaz'], + 'No', @$config['BazIsBaz'], 'Except rule correctly detects undefined variable' ); } + public function testEnvironmentRules() { + foreach (array('dev', 'test', 'live') as $env) { + Config::inst()->nest(); + + Config::inst()->update('Director', 'environment_type', $env); + $config = $this->getConfigFixtureValue('Environment'); + + foreach (array('dev', 'test', 'live') as $check) { + $this->assertEquals( + $env == $check ? $check : 'not'.$check, @$config[ucfirst($check).'Environment'], + 'Only & except rules correctly detect environment' + ); + } + + Config::inst()->unnest(); + } + } + + public function testDynamicEnvironmentRules() { + Config::inst()->nest(); + + // First, make sure environment_type is live + Config::inst()->update('Director', 'environment_type', 'live'); + $this->assertEquals('live', Config::inst()->get('Director', 'environment_type')); + + // Then, load in a new manifest, which includes a _config.php that sets environment_type to dev + $manifest = new SS_ConfigManifest(dirname(__FILE__).'/fixtures/configmanifest_dynamicenv', true, true); + Config::inst()->pushConfigYamlManifest($manifest); + + // Make sure that stuck + $this->assertEquals('dev', Config::inst()->get('Director', 'environment_type')); + + // And that the dynamic rule was calculated correctly + $this->assertEquals('dev', Config::inst()->get('ConfigManifestTest', 'DynamicEnvironment')); + + Config::inst()->unnest(); + } + + public function testRelativeOrder() { $accessor = new ConfigManifestTest_ConfigManifestAccess(BASE_PATH, true, false); diff --git a/tests/core/manifest/fixtures/configmanifest/mysite/_config/envrules.yml b/tests/core/manifest/fixtures/configmanifest/mysite/_config/envrules.yml new file mode 100644 index 000000000..6573ac565 --- /dev/null +++ b/tests/core/manifest/fixtures/configmanifest/mysite/_config/envrules.yml @@ -0,0 +1,42 @@ +--- +Only: + Environment: live +--- +ConfigManifestTest: + Environment: + LiveEnvironment: live +--- +Only: + Environment: dev +--- +ConfigManifestTest: + Environment: + DevEnvironment: dev +--- +Only: + Environment: test +--- +ConfigManifestTest: + Environment: + TestEnvironment: test +--- +Except: + Environment: live +--- +ConfigManifestTest: + Environment: + LiveEnvironment: notlive +--- +Except: + Environment: dev +--- +ConfigManifestTest: + Environment: + DevEnvironment: notdev +--- +Except: + Environment: test +--- +ConfigManifestTest: + Environment: + TestEnvironment: nottest diff --git a/tests/core/manifest/fixtures/configmanifest_dynamicenv/mysite/_config.php b/tests/core/manifest/fixtures/configmanifest_dynamicenv/mysite/_config.php new file mode 100644 index 000000000..337294dec --- /dev/null +++ b/tests/core/manifest/fixtures/configmanifest_dynamicenv/mysite/_config.php @@ -0,0 +1,4 @@ +update('Director', 'environment_type', 'dev'); diff --git a/tests/core/manifest/fixtures/configmanifest_dynamicenv/mysite/_config/environment.yml b/tests/core/manifest/fixtures/configmanifest_dynamicenv/mysite/_config/environment.yml new file mode 100644 index 000000000..fb5e190e3 --- /dev/null +++ b/tests/core/manifest/fixtures/configmanifest_dynamicenv/mysite/_config/environment.yml @@ -0,0 +1,18 @@ +--- +Only: + Environment: live +--- +ConfigManifestTest: + DynamicEnvironment: live +--- +Only: + Environment: dev +--- +ConfigManifestTest: + DynamicEnvironment: dev +--- +Only: + Environment: test +--- +ConfigManifestTest: + DynamicEnvironment: test