FIX Only and Except rules in Configs not working

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']);
+			$matches = true;
-			$matchesexcept = isset($fragment['except']) && $this->matchesPrefilterVariantRules($fragment['except']);
 
-			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']);
+			$matches = true;
-			$matchesexcept = isset($fragment['except']) && $this->matchesVariantRules($fragment['except']);
 
-			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;
+					$matches = $matches && isset($_ENV[$v]);
-					return false;
+					break;
 
 				case 'constantdefined':
-					if (defined($k)) break;
+					$matches = $matches && defined($v);
-					return false;
+					break;
 
 				default:
-					if (isset($_ENV[$k]) && $_ENV[$k] == $v) break;
+					$matches = $matches && (
-					if (defined($k) && constant($k) == $v) break;
+						(isset($_ENV[$k]) && $_ENV[$k] == $v) ||
-					return false;
+						(defined($k) && constant($k) == $v)
-			}
+					);
+					break;
 			}
 
-		return true;
+			if ($matches === false) return $matches;
+		}
+
+		return $matches;
 	}
 
 	/**

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);
 

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

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

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

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

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