Merge pull request #741 from silverstripe-rebelalliance/trac/7765

FIX several issues with the config system

_config/routes.yml

@@ -7,8 +7,10 @@ Director:
     '': 'Controller'
 ---
 Name: coreroutes
-After: cms/routes#modelascontrollerroutes
 Before: '*'
+After:
+  - framework/routes#rootroutes
+  - cms/routes#modelascontrollerroutes
 ---
 Director:
   rules:
@@ -21,7 +23,11 @@ Director:
     'interactive': 'SapphireREPL'
 ---
 Name: adminroutes
-After: framework/routes#coreroutes
+Before: '*'
+After:
+  - framework/routes#rootroutes
+  - framework/routes#coreroutes
+  - cms/routes#modelascontrollerroutes
 ---
 Director:
   rules:

core/DAG.php

@@ -4,7 +4,7 @@
  * A Directed Acyclic Graph - used for doing topological sorts on dependencies, such as the before/after conditions
  * in config yaml fragments
  */
-class SS_DAG {
+class SS_DAG implements IteratorAggregate {
 	/** @var array|null - The nodes/vertices in the graph. Should be a numeric sequence of items (no string keys, no gaps). */
 	protected $data;
 
@@ -68,7 +68,68 @@ class SS_DAG {
 			$dag = $withedges;
 		}
 
-		if ($dag) throw new Exception("DAG has cyclic requirements");
+		if ($dag) {
+			$remainder = new SS_DAG($data); $remainder->dag = $dag;
+			throw new SS_DAG_CyclicException("DAG has cyclic requirements", $remainder);
+		}
 		return $sorted;
 	}
+
+	function getIterator() {
+		return new SS_DAG_Iterator($this->data, $this->dag);
+	}
+}
+
+class SS_DAG_CyclicException extends Exception {
+
+	public $dag;
+
+	function __construct($message, $dag) {
+		$this->dag = $dag;
+		parent::__construct($message);
+	}
+
+}
+
+class SS_DAG_Iterator implements Iterator {
+
+	protected $data;
+	protected $dag;
+
+	protected $dagkeys;
+	protected $i;
+
+	function __construct($data, $dag) {
+		$this->data = $data;
+		$this->dag = $dag;
+		$this->rewind();
+	}
+
+	function key() {
+		return $this->i;
+	}
+
+	function current() {
+		$res = array();
+
+		$res['from'] = $this->data[$this->i];
+
+		$res['to'] = array();
+		foreach ($this->dag[$this->i] as $to) $res['to'][] = $this->data[$to];
+
+		return $res;
+	}
+
+	function next() {
+		$this->i = array_shift($this->dagkeys);
+	}
+
+	function rewind() {
+		$this->dagkeys = array_keys($this->dag);
+		$this->next();
+	}
+
+	function valid() {
+		return $this->i !== null;
+	}
 }

core/manifest/ConfigManifest.php

@@ -217,7 +217,8 @@ class SS_ConfigManifest {
 				foreach (array('before', 'after') as $order) {
 					if (isset($header[$order])) {
 						// First, splice into parts (multiple before or after parts are allowed, comma separated)
-						$orderparts = preg_split('/\s+,\s+/', $header[$order], PREG_SPLIT_NO_EMPTY);
+						if (is_array($header[$order])) $orderparts = $header[$order];
+						else $orderparts = preg_split('/\s*,\s*/', $header[$order], -1, PREG_SPLIT_NO_EMPTY);
 
 						// For each, parse out into module/file#name, and set any missing to "*"
 						$header[$order] = array();
@@ -267,7 +268,30 @@ class SS_ConfigManifest {
 			}
 		}
 
-		$this->yamlConfigFragments = $dag->sort();
+		try {
+			$this->yamlConfigFragments = $dag->sort();
+		}
+		catch (SS_DAG_CyclicException $e) {
+
+			if (!Director::isLive() && isset($_REQUEST['debug'])) {
+				$res = '<h1>Remaining config fragment graph</h1>';
+				$res .= '<dl>';
+
+				foreach ($e->dag as $node) {
+					$res .= "<dt>{$node['from']['module']}/{$node['from']['file']}#{$node['from']['name']} marked to come after</dt><dd><ul>";
+					foreach ($node['to'] as $to) {
+						$res .= "<li>{$to['module']}/{$to['file']}#{$to['name']}</li>";
+					}
+					$res .= "</ul></dd>";
+				}
+
+				$res .= '</dl>';
+				echo $res;
+			}
+
+			throw $e;
+		}
+
 	}
 	
 	/**
@@ -279,46 +303,55 @@ class SS_ConfigManifest {
 	 * @return string "after", "before" or "undefined"
 	 */
 	protected function relativeOrder($a, $b) {
-		$matchesSomeRule = array();
-		
+		$matches = array();
+
 		// Do the same thing for after and before
-		foreach (array('after'=>'before', 'before'=>'after') as $rulename => $opposite) {
-			$matchesSomeRule[$rulename] = false;
-			
-			// If no rule specified, we don't match it
-			if (isset($a[$rulename])) {
-				
-				foreach ($a[$rulename] as $rule) {
-					$matchesRule = true;
-					
-					foreach(array('module', 'file', 'name') as $part) {
-						$partMatches = true;
-						
-						// If part is *, we match _unless_ the opposite rule has a non-* matcher than also matches $b
-						if ($rule[$part] == '*') {
-							if (isset($a[$opposite])) foreach($a[$opposite] as $oppositeRule) {
-								if ($oppositeRule[$part] == $b[$part]) { $partMatches = false; break; }
-							}
-						}
-						else {
-							$partMatches = ($rule[$part] == $b[$part]);
-						}
-						
-						$matchesRule = $matchesRule && $partMatches;
-						if (!$matchesRule) break;
+		foreach (array('before', 'after') as $rulename) {
+			$matches[$rulename] = array();
+
+			// Figure out for each rule, which part matches
+			if (isset($a[$rulename])) foreach ($a[$rulename] as $rule) {
+				$match = array();
+
+				foreach(array('module', 'file', 'name') as $part) {
+					// If part is *, we match _unless_ the opposite rule has a non-* matcher than also matches $b
+					if ($rule[$part] == '*') {
+						$match[$part] = 'wild';
+					}
+					else {
+						$match[$part] = ($rule[$part] == $b[$part]);
 					}
-					
-					$matchesSomeRule[$rulename] = $matchesSomeRule[$rulename] || $matchesRule;
 				}
+
+				$matches[$rulename][] = $match;
 			}
 		}
-		
-		// Check if it matches both rules - problem if so
-		if ($matchesSomeRule['before'] && $matchesSomeRule['after']) {
+
+		// Figure out the specificness of each match. 1 an actual match, 0 for a wildcard match, remove if no match
+		$matchlevel = array('before' => -1, 'after' => -1);
+
+		foreach (array('before', 'after') as $rulename) {
+			foreach ($matches[$rulename] as $i => $rule) {
+				$level = 0;
+
+				foreach ($rule as $part => $partmatches) {
+					if ($partmatches === false) continue 2;
+					if ($partmatches === true) $level += 1;
+				}
+
+				if ($matchlevel[$rulename] === false || $level > $matchlevel[$rulename]) $matchlevel[$rulename] = $level;
+			}
+		}
+
+		if ($matchlevel['before'] === -1 && $matchlevel['after'] === -1) {
+			return 'undefined';
+		}
+		else if ($matchlevel['before'] === $matchlevel['after']) {
 			user_error('Config fragment requires itself to be both before _and_ after another fragment', E_USER_ERROR);
 		}
-		
-		return $matchesSomeRule['before'] ? 'before' : ($matchesSomeRule['after'] ? 'after' : 'undefined');
+		else {
+			return ($matchlevel['before'] > $matchlevel['after']) ? 'before' : 'after';
+		}
 	}
 
 	/**

tests/core/manifest/ConfigManifestTest.php

@@ -0,0 +1,89 @@
+<?php
+
+class ConfigManifestTest_ConfigManifestAccess extends SS_ConfigManifest {
+	public function relativeOrder($a, $b) {
+		return parent::relativeOrder($a, $b);
+	}
+}
+
+class ConfigManifestTest extends SapphireTest {
+
+	function testRelativeOrder() {
+		$accessor = new ConfigManifestTest_ConfigManifestAccess(BASE_PATH, true, false);
+
+		// A fragment with a fully wildcard before rule
+		$beforeWildcarded = array(
+			'module' => 'foo', 'file' => 'alpha', 'name' => '1',
+			'before' => array(array('module' => '*', 'file' => '*', 'name' => '*'))
+		);
+		// A fragment with a fully wildcard before rule and a fully explicit after rule
+		$beforeWildcardedAfterExplicit = array_merge($beforeWildcarded, array(
+			'after' => array(array('module' => 'bar', 'file' => 'beta', 'name' => '2'))
+		));
+		// A fragment with a fully wildcard before rule and two fully explicit after rules
+		$beforeWildcardedAfterTwoExplicitRules = array_merge($beforeWildcarded, array(
+			'after' => array(
+				array('module' => 'bar', 'file' => 'beta', 'name' => '2'),
+				array('module' => 'baz', 'file' => 'gamma', 'name' => '3')
+			)
+		));
+		// A fragment with a fully wildcard before rule and a partially explicit after rule
+		$beforeWildcardedAfterPartialWildcarded = array_merge($beforeWildcarded, array(
+			'after' => array(array('module' => 'bar', 'file' => 'beta', 'name' => '*'))
+		));
+
+		// Wildcard should match any module
+		$this->assertEquals($accessor->relativeOrder(
+			$beforeWildcarded,
+			array('module' => 'qux', 'file' => 'delta', 'name' => '4')
+		), 'before');
+
+		// Wildcard should match any module even if there is an opposing rule, if opposing rule doesn't match
+		$this->assertEquals($accessor->relativeOrder(
+			$beforeWildcardedAfterExplicit,
+			array('module' => 'qux', 'file' => 'delta', 'name' => '4')
+		), 'before');
+
+		// Wildcard should match any module even if there is an opposing rule, if opposing rule doesn't match, no matter how many opposing rules
+		$this->assertEquals($accessor->relativeOrder(
+			$beforeWildcardedAfterExplicit,
+			array('module' => 'qux', 'file' => 'delta', 'name' => '4')
+		), 'before');
+
+		// Wildcard should match any module even if there is an opposing rule, if opposing rule doesn't match (even if some portions do)
+		$this->assertEquals($accessor->relativeOrder(
+			$beforeWildcardedAfterExplicit,
+			array('module' => 'bar', 'file' => 'beta', 'name' => 'nomatchy')
+		), 'before');
+
+		// When opposing rule matches, wildcard should be ignored
+		$this->assertEquals($accessor->relativeOrder(
+			$beforeWildcardedAfterExplicit,
+			array('module' => 'bar', 'file' => 'beta', 'name' => '2')
+		), 'after');
+
+		// When any one of mutiple opposing rule exists, wildcard should be ignored
+		$this->assertEquals($accessor->relativeOrder(
+			$beforeWildcardedAfterTwoExplicitRules,
+			array('module' => 'bar', 'file' => 'beta', 'name' => '2')
+		), 'after');
+
+		$this->assertEquals($accessor->relativeOrder(
+			$beforeWildcardedAfterTwoExplicitRules,
+			array('module' => 'baz', 'file' => 'gamma', 'name' => '3')
+		), 'after');
+
+		// When two opposed wildcard rules, and more specific one doesn't match, other should win
+		$this->assertEquals($accessor->relativeOrder(
+			$beforeWildcardedAfterPartialWildcarded,
+			array('module' => 'qux', 'file' => 'delta', 'name' => '4')
+		), 'before');
+
+		// When two opposed wildcard rules, and more specific one does match, more specific one should win
+		$this->assertEquals($accessor->relativeOrder(
+			$beforeWildcardedAfterPartialWildcarded,
+			array('module' => 'bar', 'file' => 'beta', 'name' => 'wildcardmatchy')
+		), 'after');
+	}
+
+}