From 255fbe5ca623ca3a7dc5b3cd06e19777c3daecf8 Mon Sep 17 00:00:00 2001 From: kenjis Date: Fri, 7 Oct 2022 14:49:53 +0900 Subject: [PATCH 1/5] fix: bug in route registration $routes and $routesOptions are not the same structure. $routesOptions don't have route names. If there are two routes having the same `from` but not the same name, the route and the option were not connected correctly. --- system/Router/RouteCollection.php | 16 ++++++++++-- tests/system/Router/RouteCollectionTest.php | 29 +++++++++++++++++++++ 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/system/Router/RouteCollection.php b/system/Router/RouteCollection.php index 66a950e72fee..844dfdb11f7a 100644 --- a/system/Router/RouteCollection.php +++ b/system/Router/RouteCollection.php @@ -110,7 +110,7 @@ class RouteCollection implements RouteCollectionInterface * verb => [ * routeName => [ * 'route' => [ - * routeKey => handler, + * routeKey(or from) => handler, * ] * ] * ], @@ -133,6 +133,14 @@ class RouteCollection implements RouteCollectionInterface * Array of routes options * * @var array + * + * [ + * verb => [ + * routeKey(or from) => [ + * key => value, + * ] + * ], + * ] */ protected $routesOptions = []; @@ -1239,12 +1247,16 @@ protected function create(string $verb, string $from, $to, ?array $options = nul $name = $options['as'] ?? $from; + helper('array'); + // Don't overwrite any existing 'froms' so that auto-discovered routes // do not overwrite any app/Config/Routes settings. The app // routes should always be the "source of truth". // this works only because discovered routes are added just prior // to attempting to route the request. - if (isset($this->routes[$verb][$name]) && ! $overwrite) { + $fromExists = (dot_array_search('*.route.' . $from, $this->routes[$verb] ?? []) === null) + ? false : true; + if ((isset($this->routes[$verb][$name]) || $fromExists) && ! $overwrite) { return; } diff --git a/tests/system/Router/RouteCollectionTest.php b/tests/system/Router/RouteCollectionTest.php index cac8d4d4d336..2db20898e20c 100644 --- a/tests/system/Router/RouteCollectionTest.php +++ b/tests/system/Router/RouteCollectionTest.php @@ -1189,6 +1189,35 @@ static function () {}, $this->assertSame($options, ['as' => 'admin', 'foo' => 'baz']); } + public function testRoutesOptionsWithSameFromTwoRoutes() + { + $routes = $this->getCollector(); + + // This is the first route for `administrator`. + $options1 = [ + 'as' => 'admin', + 'foo' => 'options1', + ]; + $routes->get( + 'administrator', + static function () {}, + $options1 + ); + // The second route for `administrator` should be ignored. + $options2 = [ + 'foo' => 'options2', + ]; + $routes->get( + 'administrator', + static function () {}, + $options2 + ); + + $options = $routes->getRoutesOptions('administrator'); + + $this->assertSame($options, $options1); + } + public function testRoutesOptionsForDifferentVerbs() { $routes = $this->getCollector(); From 8ac5198bcf7bd712ab7d9a32a39b2742d5bde360 Mon Sep 17 00:00:00 2001 From: kenjis Date: Fri, 7 Oct 2022 14:56:14 +0900 Subject: [PATCH 2/5] test: fix incorrect test --- tests/system/Router/RouteCollectionTest.php | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/system/Router/RouteCollectionTest.php b/tests/system/Router/RouteCollectionTest.php index 2db20898e20c..b8b6f9ddbfc0 100644 --- a/tests/system/Router/RouteCollectionTest.php +++ b/tests/system/Router/RouteCollectionTest.php @@ -11,7 +11,6 @@ namespace CodeIgniter\Router; -use App\Controllers\Home; use CodeIgniter\Config\Services; use CodeIgniter\Test\CIUnitTestCase; use Config\Modules; @@ -1489,12 +1488,12 @@ public function testRouteOverwritingTwoRules() $routes->setDefaultController('Home'); $routes->setDefaultMethod('index'); + // The subdomain of the current URL is `doc`, so this route is registered. $routes->get('/', '\App\Controllers\Site\CDoc::index', ['subdomain' => 'doc', 'as' => 'doc_index']); + // The subdomain route is already registered, so this route is not registered. $routes->get('/', 'Home::index'); - // the second rule applies, so overwrites the first - $expects = '\\' . Home::class; - + $expects = '\App\Controllers\Site\CDoc'; $this->assertSame($expects, $router->handle('/')); } From cfeb6f1e2eac4bc97fb89a4ebe815f3a5d23cf8e Mon Sep 17 00:00:00 2001 From: kenjis Date: Fri, 7 Oct 2022 15:03:56 +0900 Subject: [PATCH 3/5] test: add test cases --- tests/system/Router/RouteCollectionTest.php | 54 ++++++++++++++++++--- 1 file changed, 46 insertions(+), 8 deletions(-) diff --git a/tests/system/Router/RouteCollectionTest.php b/tests/system/Router/RouteCollectionTest.php index b8b6f9ddbfc0..c41ae4ec64ba 100644 --- a/tests/system/Router/RouteCollectionTest.php +++ b/tests/system/Router/RouteCollectionTest.php @@ -1188,24 +1188,20 @@ static function () {}, $this->assertSame($options, ['as' => 'admin', 'foo' => 'baz']); } - public function testRoutesOptionsWithSameFromTwoRoutes() + /** + * @dataProvider optionsProvider + */ + public function testRoutesOptionsWithSameFromTwoRoutes(array $options1, array $options2) { $routes = $this->getCollector(); // This is the first route for `administrator`. - $options1 = [ - 'as' => 'admin', - 'foo' => 'options1', - ]; $routes->get( 'administrator', static function () {}, $options1 ); // The second route for `administrator` should be ignored. - $options2 = [ - 'foo' => 'options2', - ]; $routes->get( 'administrator', static function () {}, @@ -1217,6 +1213,48 @@ static function () {}, $this->assertSame($options, $options1); } + public function optionsProvider() + { + yield from [ + [ + [ + 'foo' => 'options1', + ], + [ + 'foo' => 'options2', + ], + ], + [ + [ + 'as' => 'admin', + 'foo' => 'options1', + ], + [ + 'foo' => 'options2', + ], + ], + [ + [ + 'foo' => 'options1', + ], + [ + 'as' => 'admin', + 'foo' => 'options2', + ], + ], + [ + [ + 'as' => 'admin', + 'foo' => 'options1', + ], + [ + 'as' => 'admin', + 'foo' => 'options2', + ], + ], + ]; + } + public function testRoutesOptionsForDifferentVerbs() { $routes = $this->getCollector(); From 596b2e949d7ccdd9838a382075344a7ce90ba716 Mon Sep 17 00:00:00 2001 From: kenjis Date: Fri, 7 Oct 2022 15:24:54 +0900 Subject: [PATCH 4/5] refactor: run rector --- system/Router/RouteCollection.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/system/Router/RouteCollection.php b/system/Router/RouteCollection.php index 844dfdb11f7a..70a4fdc22311 100644 --- a/system/Router/RouteCollection.php +++ b/system/Router/RouteCollection.php @@ -1254,8 +1254,7 @@ protected function create(string $verb, string $from, $to, ?array $options = nul // routes should always be the "source of truth". // this works only because discovered routes are added just prior // to attempting to route the request. - $fromExists = (dot_array_search('*.route.' . $from, $this->routes[$verb] ?? []) === null) - ? false : true; + $fromExists = dot_array_search('*.route.' . $from, $this->routes[$verb] ?? []) !== null; if ((isset($this->routes[$verb][$name]) || $fromExists) && ! $overwrite) { return; } From daa1b7c250c05b0f9741978ea858fd0f56f3dab6 Mon Sep 17 00:00:00 2001 From: kenjis Date: Fri, 7 Oct 2022 15:37:39 +0900 Subject: [PATCH 5/5] fix: incorrect test The second call for add `exampleRoute` would be ignored. --- tests/system/HTTP/RedirectResponseTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/system/HTTP/RedirectResponseTest.php b/tests/system/HTTP/RedirectResponseTest.php index 1b8fe62f49d9..a3736f891a3d 100644 --- a/tests/system/HTTP/RedirectResponseTest.php +++ b/tests/system/HTTP/RedirectResponseTest.php @@ -73,12 +73,12 @@ public function testRedirectRoute() $this->assertTrue($response->hasHeader('Location')); $this->assertSame('http://example.com/index.php/exampleRoute', $response->getHeaderLine('Location')); - $this->routes->add('exampleRoute', 'Home::index', ['as' => 'home']); + $this->routes->add('exampleRoute2', 'Home::index', ['as' => 'home']); $response->route('home'); $this->assertTrue($response->hasHeader('Location')); - $this->assertSame('http://example.com/index.php/exampleRoute', $response->getHeaderLine('Location')); + $this->assertSame('http://example.com/index.php/exampleRoute2', $response->getHeaderLine('Location')); } public function testRedirectRouteBadNamedRoute()