Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: routes registration bug #6644

Merged
merged 5 commits into from
Oct 9, 2022
Merged

fix: routes registration bug #6644

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
255fbe5
fix: bug in route registration
kenjis Oct 7, 2022
8ac5198
test: fix incorrect test
kenjis Oct 7, 2022
cfeb6f1
test: add test cases
kenjis Oct 7, 2022
596b2e9
refactor: run rector
kenjis Oct 7, 2022
daa1b7c
fix: incorrect test
kenjis Oct 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions system/Router/RouteCollection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ class RouteCollection implements RouteCollectionInterface
* verb => [
* routeName => [
* 'route' => [
* routeKey => handler,
* routeKey(or from) => handler,
* ]
* ]
* ],
Expand All @@ -133,6 +133,14 @@ class RouteCollection implements RouteCollectionInterface
* Array of routes options
*
* @var array
*
* [
* verb => [
* routeKey(or from) => [
* key => value,
* ]
* ],
* ]
*/
protected $routesOptions = [];

Expand Down Expand Up @@ -1239,12 +1247,15 @@ 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;
if ((isset($this->routes[$verb][$name]) || $fromExists) && ! $overwrite) {
return;
}

Expand Down
4 changes: 2 additions & 2 deletions tests/system/HTTP/RedirectResponseTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
74 changes: 70 additions & 4 deletions tests/system/Router/RouteCollectionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@

namespace CodeIgniter\Router;

use App\Controllers\Home;
use CodeIgniter\Config\Services;
use CodeIgniter\Test\CIUnitTestCase;
use Config\Modules;
Expand Down Expand Up @@ -1189,6 +1188,73 @@ static function () {},
$this->assertSame($options, ['as' => 'admin', 'foo' => 'baz']);
}

/**
* @dataProvider optionsProvider
*/
public function testRoutesOptionsWithSameFromTwoRoutes(array $options1, array $options2)
{
$routes = $this->getCollector();

// This is the first route for `administrator`.
$routes->get(
'administrator',
static function () {},
$options1
);
// The second route for `administrator` should be ignored.
$routes->get(
'administrator',
static function () {},
$options2
);

$options = $routes->getRoutesOptions('administrator');

$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();
Expand Down Expand Up @@ -1460,12 +1526,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('/'));
}

Expand Down