From e74c002647e6302618c880ab653bd48894692ec5 Mon Sep 17 00:00:00 2001 From: Hamish Friedlander Date: Fri, 28 Jun 2013 13:46:32 +1200 Subject: [PATCH] FIX Only and Except rules in Configs not working --- core/manifest/ConfigManifest.php | 64 +++++++++---- tests/core/manifest/ConfigManifestTest.php | 94 +++++++++++++++++++ .../mysite/_config/classrules.yml | 28 ++++++ .../mysite/_config/constantdefinedrules.yml | 28 ++++++ .../mysite/_config/envvarsetrules.yml | 28 ++++++ .../mysite/_config/modulerules.yml | 28 ++++++ .../mysite/_config/variablerules.yml | 70 ++++++++++++++ 7 files changed, 320 insertions(+), 20 deletions(-) create mode 100644 tests/core/manifest/fixtures/configmanifest/mysite/_config/classrules.yml create mode 100644 tests/core/manifest/fixtures/configmanifest/mysite/_config/constantdefinedrules.yml create mode 100644 tests/core/manifest/fixtures/configmanifest/mysite/_config/envvarsetrules.yml create mode 100644 tests/core/manifest/fixtures/configmanifest/mysite/_config/modulerules.yml create mode 100644 tests/core/manifest/fixtures/configmanifest/mysite/_config/variablerules.yml diff --git a/core/manifest/ConfigManifest.php b/core/manifest/ConfigManifest.php index b8cffb2c2..332f838c7 100644 --- a/core/manifest/ConfigManifest.php +++ b/core/manifest/ConfigManifest.php @@ -396,10 +396,17 @@ class SS_ConfigManifest { $matchingFragments = array(); foreach ($this->yamlConfigFragments as $i => $fragment) { - $failsonly = isset($fragment['only']) && !$this->matchesPrefilterVariantRules($fragment['only']); - $matchesexcept = isset($fragment['except']) && $this->matchesPrefilterVariantRules($fragment['except']); + $matches = true; - if (!$failsonly && !$matchesexcept) $matchingFragments[] = $fragment; + if (isset($fragment['only'])) { + $matches = $matches && ($this->matchesPrefilterVariantRules($fragment['only']) !== false); + } + + if (isset($fragment['except'])) { + $matches = $matches && ($this->matchesPrefilterVariantRules($fragment['except']) !== true); + } + + if ($matches) $matchingFragments[] = $fragment; } $this->yamlConfigFragments = $matchingFragments; @@ -414,22 +421,26 @@ class SS_ConfigManifest { * which values means accept or reject a fragment */ public function matchesPrefilterVariantRules($rules) { + $matches = "undefined"; // Needs to be truthy, but not true + foreach ($rules as $k => $v) { switch (strtolower($k)) { case 'classexists': - if (!ClassInfo::exists($v)) return false; + $matches = $matches && ClassInfo::exists($v); break; case 'moduleexists': - if (!$this->moduleExists($v)) return false; + $matches = $matches && $this->moduleExists($v); break; default: // NOP } + + if ($matches === false) return $matches; } - return true; + return $matches; } /** @@ -487,10 +498,17 @@ class SS_ConfigManifest { $this->yamlConfig = array(); foreach ($this->yamlConfigFragments as $i => $fragment) { - $failsonly = isset($fragment['only']) && !$this->matchesVariantRules($fragment['only']); - $matchesexcept = isset($fragment['except']) && $this->matchesVariantRules($fragment['except']); + $matches = true; - if (!$failsonly && !$matchesexcept) $this->mergeInYamlFragment($this->yamlConfig, $fragment['fragment']); + if (isset($fragment['only'])) { + $matches = $matches && ($this->matchesVariantRules($fragment['only']) !== false); + } + + if (isset($fragment['except'])) { + $matches = $matches && ($this->matchesVariantRules($fragment['except']) !== true); + } + + if ($matches) $this->mergeInYamlFragment($this->yamlConfig, $fragment['fragment']); } if ($cache) { @@ -502,6 +520,8 @@ class SS_ConfigManifest { * Returns false if the non-prefilterable parts of the rule aren't met, and true if they are */ public function matchesVariantRules($rules) { + $matches = "undefined"; // Needs to be truthy, but not true + foreach ($rules as $k => $v) { switch (strtolower($k)) { case 'classexists': @@ -511,13 +531,13 @@ class SS_ConfigManifest { case 'environment': switch (strtolower($v)) { case 'live': - if (!Director::isLive()) return false; + $matches = $matches && Director::isLive(); break; case 'test': - if (!Director::isTest()) return false; + $matches = $matches && Director::isTest(); break; case 'dev': - if (!Director::isDev()) return false; + $matches = $matches && Director::isDev(); break; default: user_error('Unknown environment '.$v.' in config fragment', E_USER_ERROR); @@ -525,21 +545,25 @@ class SS_ConfigManifest { break; case 'envvarset': - if (isset($_ENV[$k])) break; - return false; + $matches = $matches && isset($_ENV[$v]); + break; case 'constantdefined': - if (defined($k)) break; - return false; + $matches = $matches && defined($v); + break; default: - if (isset($_ENV[$k]) && $_ENV[$k] == $v) break; - if (defined($k) && constant($k) == $v) break; - return false; + $matches = $matches && ( + (isset($_ENV[$k]) && $_ENV[$k] == $v) || + (defined($k) && constant($k) == $v) + ); + break; } + + if ($matches === false) return $matches; } - return true; + return $matches; } /** diff --git a/tests/core/manifest/ConfigManifestTest.php b/tests/core/manifest/ConfigManifestTest.php index 85800ad0e..1250adbdc 100644 --- a/tests/core/manifest/ConfigManifestTest.php +++ b/tests/core/manifest/ConfigManifestTest.php @@ -8,6 +8,100 @@ class ConfigManifestTest_ConfigManifestAccess extends SS_ConfigManifest { class ConfigManifestTest extends SapphireTest { + protected function getConfigFixture() { + $manifest = new SS_ConfigManifest(dirname(__FILE__).'/fixtures/configmanifest', true, true); + return $manifest->yamlConfig; + } + + public function testClassRules() { + $config = $this->getConfigFixture(); + + $this->assertEquals( + 'Yes', @$config['ConfigManifestTest']['Class']['DirectorExists'], + 'Only rule correctly detects existing class' + ); + + $this->assertEquals( + 'No', @$config['ConfigManifestTest']['Class']['NoSuchClassExists'], + 'Except rule correctly detects missing class' + ); + } + + public function testModuleRules() { + $config = $this->getConfigFixture(); + + $this->assertEquals( + 'Yes', @$config['ConfigManifestTest']['Module']['MysiteExists'], + 'Only rule correctly detects existing module' + ); + + $this->assertEquals( + 'No', @$config['ConfigManifestTest']['Module']['NoSuchModuleExists'], + 'Except rule correctly detects missing module' + ); + } + + public function testEnvVarSetRules() { + $_ENV['EnvVarSet_Foo'] = 1; + $config = $this->getConfigFixture(); + + $this->assertEquals( + 'Yes', @$config['ConfigManifestTest']['EnvVarSet']['FooSet'], + 'Only rule correctly detects set environment variable' + ); + + $this->assertEquals( + 'No', @$config['ConfigManifestTest']['EnvVarSet']['BarSet'], + 'Except rule correctly detects unset environment variable' + ); + } + + public function testConstantDefinedRules() { + define('ConstantDefined_Foo', 1); + $config = $this->getConfigFixture(); + + $this->assertEquals( + 'Yes', @$config['ConfigManifestTest']['ConstantDefined']['FooDefined'], + 'Only rule correctly detects defined constant' + ); + + $this->assertEquals( + 'No', @$config['ConfigManifestTest']['ConstantDefined']['BarDefined'], + 'Except rule correctly detects undefined constant' + ); + } + + public function testEnvOrConstantMatchesValueRules() { + $_ENV['EnvOrConstantMatchesValue_Foo'] = 'Foo'; + define('EnvOrConstantMatchesValue_Bar', 'Bar'); + $config = $this->getConfigFixture(); + + $this->assertEquals( + 'Yes', @$config['ConfigManifestTest']['EnvOrConstantMatchesValue']['FooIsFoo'], + 'Only rule correctly detects environment variable matches specified value' + ); + + $this->assertEquals( + 'Yes', @$config['ConfigManifestTest']['EnvOrConstantMatchesValue']['BarIsBar'], + 'Only rule correctly detects constant matches specified value' + ); + + $this->assertEquals( + 'No', @$config['ConfigManifestTest']['EnvOrConstantMatchesValue']['FooIsQux'], + 'Except rule correctly detects environment variable that doesn\'t match specified value' + ); + + $this->assertEquals( + 'No', @$config['ConfigManifestTest']['EnvOrConstantMatchesValue']['BarIsQux'], + 'Except rule correctly detects environment variable that doesn\'t match specified value' + ); + + $this->assertEquals( + 'No', @$config['ConfigManifestTest']['EnvOrConstantMatchesValue']['BazIsBaz'], + 'Except rule correctly detects undefined variable' + ); + } + public function testRelativeOrder() { $accessor = new ConfigManifestTest_ConfigManifestAccess(BASE_PATH, true, false); diff --git a/tests/core/manifest/fixtures/configmanifest/mysite/_config/classrules.yml b/tests/core/manifest/fixtures/configmanifest/mysite/_config/classrules.yml new file mode 100644 index 000000000..eaadf2c7a --- /dev/null +++ b/tests/core/manifest/fixtures/configmanifest/mysite/_config/classrules.yml @@ -0,0 +1,28 @@ +--- +Only: + ClassExists: Director +--- +ConfigManifestTest: + Class: + DirectorExists: Yes +--- +Only: + ClassExists: NoSuchClass +--- +ConfigManifestTest: + Class: + NoSuchClassExists: Yes +--- +Except: + ClassExists: Director +--- +ConfigManifestTest: + Class: + DirectorExists: No +--- +Except: + ClassExists: NoSuchClass +--- +ConfigManifestTest: + Class: + NoSuchClassExists: No diff --git a/tests/core/manifest/fixtures/configmanifest/mysite/_config/constantdefinedrules.yml b/tests/core/manifest/fixtures/configmanifest/mysite/_config/constantdefinedrules.yml new file mode 100644 index 000000000..2833cb592 --- /dev/null +++ b/tests/core/manifest/fixtures/configmanifest/mysite/_config/constantdefinedrules.yml @@ -0,0 +1,28 @@ +--- +Only: + ConstantDefined: ConstantDefined_Foo +--- +ConfigManifestTest: + ConstantDefined: + FooDefined: Yes +--- +Only: + ConstantDefined: ConstantDefined_Bar +--- +ConfigManifestTest: + ConstantDefined: + BarDefined: Yes +--- +Except: + ConstantDefined: ConstantDefined_Foo +--- +ConfigManifestTest: + ConstantDefined: + FooDefined: No +--- +Except: + ConstantDefined: ConstantDefined_Bar +--- +ConfigManifestTest: + ConstantDefined: + BarDefined: No diff --git a/tests/core/manifest/fixtures/configmanifest/mysite/_config/envvarsetrules.yml b/tests/core/manifest/fixtures/configmanifest/mysite/_config/envvarsetrules.yml new file mode 100644 index 000000000..45368aa00 --- /dev/null +++ b/tests/core/manifest/fixtures/configmanifest/mysite/_config/envvarsetrules.yml @@ -0,0 +1,28 @@ +--- +Only: + EnvVarSet: EnvVarSet_Foo +--- +ConfigManifestTest: + EnvVarSet: + FooSet: Yes +--- +Only: + EnvVarSet: EnvVarSet_Bar +--- +ConfigManifestTest: + EnvVarSet: + BarSet: Yes +--- +Except: + EnvVarSet: EnvVarSet_Foo +--- +ConfigManifestTest: + EnvVarSet: + FooSet: No +--- +Except: + EnvVarSet: EnvVarSet_Bar +--- +ConfigManifestTest: + EnvVarSet: + BarSet: No diff --git a/tests/core/manifest/fixtures/configmanifest/mysite/_config/modulerules.yml b/tests/core/manifest/fixtures/configmanifest/mysite/_config/modulerules.yml new file mode 100644 index 000000000..1cb9232f6 --- /dev/null +++ b/tests/core/manifest/fixtures/configmanifest/mysite/_config/modulerules.yml @@ -0,0 +1,28 @@ +--- +Only: + ModuleExists: mysite +--- +ConfigManifestTest: + Module: + MysiteExists: Yes +--- +Only: + ModuleExists: nosuchmodule +--- +ConfigManifestTest: + Module: + NoSuchModuleExists: Yes +--- +Except: + ModuleExists: mysite +--- +ConfigManifestTest: + Module: + MysiteExists: No +--- +Except: + ModuleExists: nosuchmodule +--- +ConfigManifestTest: + Module: + NoSuchModuleExists: No diff --git a/tests/core/manifest/fixtures/configmanifest/mysite/_config/variablerules.yml b/tests/core/manifest/fixtures/configmanifest/mysite/_config/variablerules.yml new file mode 100644 index 000000000..ebb7c61a1 --- /dev/null +++ b/tests/core/manifest/fixtures/configmanifest/mysite/_config/variablerules.yml @@ -0,0 +1,70 @@ +--- +Only: + EnvOrConstantMatchesValue_Foo: Foo +--- +ConfigManifestTest: + EnvOrConstantMatchesValue: + FooIsFoo: Yes +--- +Only: + EnvOrConstantMatchesValue_Foo: Qux +--- +ConfigManifestTest: + EnvOrConstantMatchesValue: + FooIsQux: Yes +--- +Only: + EnvOrConstantMatchesValue_Bar: Bar +--- +ConfigManifestTest: + EnvOrConstantMatchesValue: + BarIsBar: Yes +--- +Only: + EnvOrConstantMatchesValue_Bar: Qux +--- +ConfigManifestTest: + EnvOrConstantMatchesValue: + BarIsQux: Yes +--- +Only: + EnvOrConstantMatchesValue_Baz: Baz +--- +ConfigManifestTest: + EnvOrConstantMatchesValue: + BazIsBaz: Yes +--- +Except: + EnvOrConstantMatchesValue_Foo: Foo +--- +ConfigManifestTest: + EnvOrConstantMatchesValue: + FooIsFoo: No +--- +Except: + EnvOrConstantMatchesValue_Foo: Qux +--- +ConfigManifestTest: + EnvOrConstantMatchesValue: + FooIsQux: No +--- +Except: + EnvOrConstantMatchesValue_Bar: Bar +--- +ConfigManifestTest: + EnvOrConstantMatchesValue: + BarIsBar: No +--- +Except: + EnvOrConstantMatchesValue_Bar: Qux +--- +ConfigManifestTest: + EnvOrConstantMatchesValue: + BarIsQux: No +--- +Except: + EnvOrConstantMatchesValue_Baz: Baz +--- +ConfigManifestTest: + EnvOrConstantMatchesValue: + BazIsBaz: No