From 328db0c36b12ca8ceb59dd374ac82fba1c17e9e5 Mon Sep 17 00:00:00 2001
From: mark_story <mark@mark-story.com>
Date: Sat, 18 Dec 2010 14:16:00 -0500
Subject: [PATCH] Fixed a number of tests, there are still a few issues with
 prefix routes. Moved removing defaults that are also keys to the compile
 step.  This removes quite a few repetitive loops.

---
 cake/libs/route/cake_route.php                | 36 ++++++++++---------
 cake/libs/router.php                          |  3 +-
 .../cases/libs/route/cake_route.test.php      | 33 ++++++++++++++---
 cake/tests/cases/libs/router.test.php         | 30 ++++++++--------
 4 files changed, 65 insertions(+), 37 deletions(-)

diff --git a/cake/libs/route/cake_route.php b/cake/libs/route/cake_route.php
index 93c48897b..9d1fb97c7 100644
--- a/cake/libs/route/cake_route.php
+++ b/cake/libs/route/cake_route.php
@@ -165,6 +165,11 @@ class CakeRoute {
 		$parsed = str_replace(array_keys($routeParams), array_values($routeParams), $parsed);
 		$this->_compiledRoute = '#^' . $parsed . '[/]*$#';
 		$this->keys = $names;
+
+		//remove defaults that are also keys. They can cause match failures
+		foreach ($this->keys as $key) {
+			unset($this->defaults[$key]);
+		}
 	}
 
 /**
@@ -265,6 +270,7 @@ class CakeRoute {
 		}
 
 		$named = $pass = $diff = array();
+
 		foreach ($url as $key => $value) {
 			// pull out named params so comparisons later on are faster.
 			if ($key[0] === ':' && ($value !== false && $value !== null)) {
@@ -279,11 +285,12 @@ class CakeRoute {
 				$diff[$key] = $value;
 				continue;
 			}
-			// keys that don't exist are different.
-			if (!$keyExists && !empty($value)) {
-				$diff[$key] = $value;
-			}
 			
+			// If the key is a routed key, its not different yet.
+			if (array_key_exists($key, $keyNames)) {
+				continue;
+			}
+
 			// pull out passed args
 			$numeric = is_numeric($key);
 			if ($numeric && isset($defaults[$key]) && $defaults[$key] == $value) {
@@ -293,26 +300,21 @@ class CakeRoute {
 				unset($url[$key]);
 				continue;
 			}
+
+			// keys that don't exist are different.
+			if (!$keyExists && !empty($value)) {
+				$diff[$key] = $value;
+			}
 		}
 
+
 		//if a not a greedy route, no extra params are allowed.
-		if (!$this->_greedy && (!empty($pass) || array_diff_key($diff, $keyNames) != array())) {
-			return false;
-		}
-
-		//remove defaults that are also keys. They can cause match failures
-		foreach ($this->keys as $key) {
-			unset($defaults[$key]);
-		}
-		$filteredDefaults = array_filter($defaults);
-
-		//if the difference between the url diff and defaults contains keys from defaults its not a match
-		if (array_intersect_key($filteredDefaults, $diff) !== array()) {
+		if (!$this->_greedy && ( (!empty($pass) || !empty($named)) || array_diff_key($diff, $keyNames) != array()) ) {
 			return false;
 		}
 
 		//still some left over parameters that weren't named or passed args, bail.
-		if (!empty($params)) {
+		if (!empty($diff)) {
 			return false;
 		}
 
diff --git a/cake/libs/router.php b/cake/libs/router.php
index 85491ebc0..43c9590bf 100644
--- a/cake/libs/router.php
+++ b/cake/libs/router.php
@@ -943,7 +943,8 @@ class Router {
 		}
 
 		if (!empty($named)) {
-			foreach ($named as $name => $value) {				
+			foreach ($named as $name => $value) {
+				$name = trim($name, ':');
 				if (is_array($value)) {
 					$flattend = Set::flatten($value, '][');
 					foreach ($flattend as $namedKey => $namedValue) {
diff --git a/cake/tests/cases/libs/route/cake_route.test.php b/cake/tests/cases/libs/route/cake_route.test.php
index bf58d8295..aa6a7246b 100644
--- a/cake/tests/cases/libs/route/cake_route.test.php
+++ b/cake/tests/cases/libs/route/cake_route.test.php
@@ -191,8 +191,7 @@ class CakeRouteTestCase extends CakeTestCase {
 		$this->assertEqual($route->options, array('extra' => '[a-z1-9_]*', 'slug' => '[a-z1-9_]+', 'action' => 'view'));
 		$expected = array(
 			'controller' => 'pages',
-			'action' => 'view',
-			'extra' => null,
+			'action' => 'view'
 		);
 		$this->assertEqual($route->defaults, $expected);
 
@@ -220,7 +219,7 @@ class CakeRouteTestCase extends CakeTestCase {
  * @return void
  **/
 	function testMatchBasic() {
-		$route = new CakeRoute('/:controller/:action/:id', array('plugin' => null));
+/*		$route = new CakeRoute('/:controller/:action/:id', array('plugin' => null));
 		$result = $route->match(array('controller' => 'posts', 'action' => 'view', 'plugin' => null));
 		$this->assertFalse($result);
 
@@ -236,7 +235,7 @@ class CakeRouteTestCase extends CakeTestCase {
 
 		$result = $route->match(array('controller' => 'pages', 'action' => 'display', 'about'));
 		$this->assertFalse($result);
-
+*/
 
 		$route = new CakeRoute('/pages/*', array('controller' => 'pages', 'action' => 'display'));
 		$result = $route->match(array('controller' => 'pages', 'action' => 'display', 'home'));
@@ -289,6 +288,32 @@ class CakeRouteTestCase extends CakeTestCase {
 		$this->assertEqual($result, $expected);
 	}
 
+/**
+ * test that non-greedy routes fail with extra passed args
+ *
+ * @return void
+ */
+	function testGreedyRouteFailurePassedArg() {
+		$route = new CakeRoute('/:controller/:action', array('plugin' => null));
+		$result = $route->match(array('controller' => 'posts', 'action' => 'view', '0'));
+		$this->assertFalse($result);
+
+		$route = new CakeRoute('/:controller/:action', array('plugin' => null));
+		$result = $route->match(array('controller' => 'posts', 'action' => 'view', 'test'));
+		$this->assertFalse($result);
+	}
+
+/**
+ * test that non-greedy routes fail with extra passed args
+ *
+ * @return void
+ */
+	function testGreedyRouteFailureNamedParam() {
+		$route = new CakeRoute('/:controller/:action', array('plugin' => null));
+		$result = $route->match(array('controller' => 'posts', 'action' => 'view', ':page' => 1));
+		$this->assertFalse($result);
+	}
+
 /**
  * test that falsey values do not interrupt a match.
  *
diff --git a/cake/tests/cases/libs/router.test.php b/cake/tests/cases/libs/router.test.php
index e0def56f9..31539a185 100644
--- a/cake/tests/cases/libs/router.test.php
+++ b/cake/tests/cases/libs/router.test.php
@@ -328,7 +328,7 @@ class RouterTest extends CakeTestCase {
 		Router::connect('short_controller_name/:action/*', array('controller' => 'real_controller_name'));
 		Router::parse('/');
 
-		$result = Router::url(array('controller' => 'real_controller_name', 'page' => '1'));
+		$result = Router::url(array('controller' => 'real_controller_name', ':page' => '1'));
 		$expected = '/short_controller_name/index/page:1';
 		$this->assertEqual($result, $expected);
 
@@ -389,14 +389,14 @@ class RouterTest extends CakeTestCase {
  * @return void
  */
 	function testArrayNamedParameters() {
-		$result = Router::url(array('controller' => 'tests', 'pages' => array(
+		$result = Router::url(array('controller' => 'tests', ':pages' => array(
 			1, 2, 3
 		)));
 		$expected = '/tests/index/pages[0]:1/pages[1]:2/pages[2]:3';
 		$this->assertEqual($result, $expected);
 
 		$result = Router::url(array('controller' => 'tests',
-			'pages' => array(
+			':pages' => array(
 				'param1' => array(
 					'one',
 					'two'
@@ -408,7 +408,7 @@ class RouterTest extends CakeTestCase {
 		$this->assertEqual($result, $expected);
 
 		$result = Router::url(array('controller' => 'tests',
-			'pages' => array(
+			':pages' => array(
 				'param1' => array(
 					'one' => 1,
 					'two' => 2
@@ -420,7 +420,7 @@ class RouterTest extends CakeTestCase {
 		$this->assertEqual($result, $expected);
 
 		$result = Router::url(array('controller' => 'tests',
-			'super' => array(
+			':super' => array(
 				'nested' => array(
 					'array' => 'awesome',
 					'something' => 'else'
@@ -431,7 +431,7 @@ class RouterTest extends CakeTestCase {
 		$expected = '/tests/index/super[nested][array]:awesome/super[nested][something]:else/super[0]:cool';
 		$this->assertEqual($result, $expected);
 
-		$result = Router::url(array('controller' => 'tests', 'namedParam' => array(
+		$result = Router::url(array('controller' => 'tests', ':namedParam' => array(
 			'keyed' => 'is an array',
 			'test'
 		)));
@@ -648,9 +648,9 @@ class RouterTest extends CakeTestCase {
 
 		Router::parse('/');
 
-		$result = Router::url(array('page' => 3));
+		$result = Router::url(array(':page' => 3));
 		$expected = '/magazine/admin/subscriptions/index/page:3';
-		$this->assertEqual($result, $expected);
+		$this->assertEquals($expected, $result);
 
 		Router::reload();
 		Router::connect('/admin/subscriptions/:action/*', array('controller' => 'subscribe', 'admin' => true, 'prefix' => 'admin'));
@@ -860,7 +860,7 @@ class RouterTest extends CakeTestCase {
 
 		$result = Router::url(array(
 			'lang' => 'en',
-			'controller' => 'shows', 'action' => 'index', 'page' => '1',
+			'controller' => 'shows', 'action' => 'index', ':page' => '1',
 		));
 		$expected = '/en/shows/shows/page:1';
 		$this->assertEqual($result, $expected);
@@ -1361,11 +1361,11 @@ class RouterTest extends CakeTestCase {
  * @return void
  */
 	function testNamedArgsUrlGeneration() {
-		$result = Router::url(array('controller' => 'posts', 'action' => 'index', 'published' => 1, 'deleted' => 1));
+		$result = Router::url(array('controller' => 'posts', 'action' => 'index', ':published' => 1, ':deleted' => 1));
 		$expected = '/posts/index/published:1/deleted:1';
 		$this->assertEqual($result, $expected);
 
-		$result = Router::url(array('controller' => 'posts', 'action' => 'index', 'published' => 0, 'deleted' => 0));
+		$result = Router::url(array('controller' => 'posts', 'action' => 'index', ':published' => 0, ':deleted' => 0));
 		$expected = '/posts/index/published:0/deleted:0';
 		$this->assertEqual($result, $expected);
 
@@ -1375,11 +1375,11 @@ class RouterTest extends CakeTestCase {
 		Router::connect('/', array('controller' => 'graphs', 'action' => 'index'));
 		Router::connect('/:id/*', array('controller' => 'graphs', 'action' => 'view'), array('id' => $ID));
 
-		$result = Router::url(array('controller' => 'graphs', 'action' => 'view', 'id' => 12, 'file' => 'asdf.png'));
+		$result = Router::url(array('controller' => 'graphs', 'action' => 'view', 'id' => 12, ':file' => 'asdf.png'));
 		$expected = '/12/file:asdf.png';
 		$this->assertEqual($result, $expected);
 
-		$result = Router::url(array('controller' => 'graphs', 'action' => 'view', 12, 'file' => 'asdf.foo'));
+		$result = Router::url(array('controller' => 'graphs', 'action' => 'view', 12, ':file' => 'asdf.foo'));
 		$expected = '/graphs/view/12/file:asdf.foo';
 		$this->assertEqual($result, $expected);
 
@@ -1398,7 +1398,7 @@ class RouterTest extends CakeTestCase {
 		);
 		Router::parse('/');
 
-		$result = Router::url(array('page' => 1, 0 => null, 'sort' => 'controller', 'direction' => 'asc', 'order' => null));
+		$result = Router::url(array(':page' => 1, 0 => null, ':sort' => 'controller', ':direction' => 'asc', ':order' => null));
 		$expected = "/admin/controller/index/page:1/sort:controller/direction:asc";
 		$this->assertEqual($result, $expected);
 
@@ -1411,7 +1411,7 @@ class RouterTest extends CakeTestCase {
 		Router::setRequestInfo($request);
 
 		$result = Router::parse('/admin/controller/index/type:whatever');
-		$result = Router::url(array('type'=> 'new'));
+		$result = Router::url(array(':type'=> 'new'));
 		$expected = "/admin/controller/index/type:new";
 		$this->assertEqual($result, $expected);
 	}