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.

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

Clean router config #7380

Merged
merged 22 commits into from
Apr 10, 2023
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
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
47 changes: 3 additions & 44 deletions app/Config/Routes.php
Original file line number Diff line number Diff line change
@@ -1,49 +1,8 @@
<?php

namespace Config;
use CodeIgniter\Router\RouteCollection;

// Create a new instance of our RouteCollection class.
$routes = Services::routes();

/*
* --------------------------------------------------------------------
* Router Setup
* --------------------------------------------------------------------
*/
$routes->setDefaultNamespace('App\Controllers');
$routes->setDefaultController('Home');
$routes->setDefaultMethod('index');
$routes->setTranslateURIDashes(false);
$routes->set404Override();
// The Auto Routing (Legacy) is very dangerous. It is easy to create vulnerable apps
// where controller filters or CSRF protection are bypassed.
// If you don't want to define all routes, please use the Auto Routing (Improved).
// Set `$autoRoutesImproved` to true in `app/Config/Feature.php` and set the following to true.
// $routes->setAutoRoute(false);

/*
* --------------------------------------------------------------------
* Route Definitions
* --------------------------------------------------------------------
/**
* @var RouteCollection $routes
*/

// We get a performance increase by specifying the default
// route since we don't have to scan directories.
$routes->get('/', 'Home::index');

/*
* --------------------------------------------------------------------
* Additional Routing
* --------------------------------------------------------------------
*
* There will often be times that you need additional routing and you
* need it to be able to override any defaults in this file. Environment
* based routes is one such time. require() additional route files here
* to make that happen.
*
* You will have access to the $routes object within that file without
* needing to reload it.
*/
if (is_file(APPPATH . 'Config/' . ENVIRONMENT . '/Routes.php')) {
require APPPATH . 'Config/' . ENVIRONMENT . '/Routes.php';
}
100 changes: 100 additions & 0 deletions app/Config/Routing.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
<?php

/**
* This file is part of CodeIgniter 4 framework.
*
* (c) CodeIgniter Foundation <[email protected]>
*
* For the full copyright and license information, please view
* the LICENSE file that was distributed with this source code.
*/

namespace Config;

use CodeIgniter\Config\Routing as BaseRouting;

/**
* Routing configuration
*/
class Routing extends BaseRouting
{
/**
* An array of files that contain route definitions.
* Route files are read in order, with the first match
* found taking precedence.
*
* Default: APPPATH . 'Config/Routes.php'
*/
public array $routeFiles = [
APPPATH . 'Config/Routes.php',
];

/**
* The default namespace to use for Controllers when no other
* namespace has been specified.
*
* Default: 'App\Controllers'
*/
public string $defaultNamespace = 'App\Controllers';

/**
* The default controller to use when no other controller has been
* specified.
*
* Default: 'Home'
*/
public string $defaultController = 'Home';

/**
* The default method to call on the controller when no other
* method has been set in the route.
*
* Default: 'index'
*/
public string $defaultMethod = 'index';

/**
* Whether to translate dashes in URIs to underscores.
* Primarily useful when using the auto-routing.
*
* Default: false
*/
public bool $translateURIDashes = false;

/**
* Sets the class/method that should be called if routing doesn't
* find a match. It can be either a closure or the controller/method
* name exactly like a route is defined: Users::index
*
* This setting is passed to the Router class and handled there.
*
* If you want to use a closure, you will have to set it in the
* class constructor or the routes file by calling:
*
* $routes->set404Override(function() {
* // Do something here
* });
*
* Example:
* public $override404 = 'App\Errors::show404';
*/
public $override404;

/**
* If TRUE, the system will attempt to match the URI against
* Controllers by matching each segment against folders/files
* in APPPATH/Controllers, when a match wasn't found against
* defined routes.
*
* If FALSE, will stop searching and do NO automatic routing.
*/
public bool $autoRoute = false;

/**
* If TRUE, will enable the use of the 'prioritize' option
* when defining routes.
*
* Default: false
*/
public bool $prioritize = false;
}
98 changes: 98 additions & 0 deletions system/Config/Routing.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
<?php

/**
* This file is part of CodeIgniter 4 framework.
*
* (c) CodeIgniter Foundation <[email protected]>
*
* For the full copyright and license information, please view
* the LICENSE file that was distributed with this source code.
*/

namespace CodeIgniter\Config;

/**
* Routing configuration
*/
class Routing extends BaseConfig
{
/**
* An array of files that contain route definitions.
* Route files are read in order, with the first match
* found taking precedence.
*
* Default: APPPATH . 'Config/Routes.php'
*/
public array $routeFiles = [
APPPATH . 'Routes.php',
];

/**
* The default namespace to use for Controllers when no other
* namespace has been specified.
*
* Default: 'App\Controllers'
*/
public string $defaultNamespace = 'App\Controllers';

/**
* The default controller to use when no other controller has been
* specified.
*
* Default: 'Home'
*/
public string $defaultController = 'Home';

/**
* The default method to call on the controller when no other
* method has been set in the route.
*
* Default: 'index'
*/
public string $defaultMethod = 'index';

/**
* Whether to translate dashes in URIs to underscores.
* Primarily useful when using the auto-routing.
*
* Default: false
*/
public bool $translateURIDashes = false;

/**
* Sets the class/method that should be called if routing doesn't
* find a match. It can be either a closure or the controller/method
* name exactly like a route is defined: Users::index
*
* This setting is passed to the Router class and handled there.
*
* If you want to use a closure, you will have to set it in the
* class constructor or the routes file by calling:
*
* $routes->set404Override(function() {
* // Do something here
* });
*
* Example:
* public $override404 = 'App\Errors::show404';
*/
public $override404;

/**
* If TRUE, the system will attempt to match the URI against
* Controllers by matching each segment against folders/files
* in APPPATH/Controllers, when a match wasn't found against
* defined routes.
*
* If FALSE, will stop searching and do NO automatic routing.
*/
public bool $autoRoute = false;

/**
* If TRUE, will enable the use of the 'prioritize' option
* when defining routes.
*
* Default: false
*/
public bool $prioritize = false;
}
2 changes: 1 addition & 1 deletion system/Config/Services.php
Original file line number Diff line number Diff line change
Expand Up @@ -596,7 +596,7 @@ public static function routes(bool $getShared = true)
return static::getSharedInstance('routes');
}

return new RouteCollection(AppServices::locator(), config('Modules'));
return new RouteCollection(AppServices::locator(), config('Modules'), config('Routing'));
}

/**
Expand Down
45 changes: 37 additions & 8 deletions system/Router/RouteCollection.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use CodeIgniter\Router\Exceptions\RouterException;
use Config\App;
use Config\Modules;
use Config\Routing;
use Config\Services;
use InvalidArgumentException;
use Locale;
Expand Down Expand Up @@ -88,6 +89,11 @@ class RouteCollection implements RouteCollectionInterface
*/
protected $override404;

/**
* An array of files that would contain route definitions.
*/
protected array $routeFiles = [];

/**
* Defined placeholders that can be used
* within the
Expand Down Expand Up @@ -242,12 +248,22 @@ class RouteCollection implements RouteCollectionInterface
/**
* Constructor
*/
public function __construct(FileLocator $locator, Modules $moduleConfig)
public function __construct(FileLocator $locator, Modules $moduleConfig, Routing $routing)
Copy link
Member

@kenjis kenjis Apr 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you really need this breaking change?
What if using config('Routing') in the __construct()?

If you really need it, I recommend you use refactoring feature in IDE
for updating all test code.

Copy link
Member Author

@lonnieezell lonnieezell Apr 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the same way the module config is sent it, I sent the Routing config in, so - seems like that's the proper way to do it....

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think injection is the better way than config(), but changing the method signature breaks a lot of client code...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would only break code for those that have extended the RouteCollection class, which I can't imagine is that many. If you think this particular BC is too much, we can scrap this and save it for 5. Seemed like a smaller BC possibility than a number of our other breaks, unless I'm completely missing something here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, then go with injection.

{
$this->fileLocator = $locator;
$this->moduleConfig = $moduleConfig;

$this->httpHost = Services::request()->getServer('HTTP_HOST');

// Setup based on config file. Let routes file override.
$this->defaultNamespace = $routing->defaultNamespace;
$this->defaultController = $routing->defaultController;
$this->defaultMethod = $routing->defaultMethod;
$this->translateURIDashes = $routing->translateURIDashes;
$this->override404 = $routing->override404;
$this->autoRoute = $routing->autoRoute;
$this->routeFiles = $routing->routeFiles;
$this->prioritize = $routing->prioritize;
}

/**
Expand All @@ -263,8 +279,26 @@ public function loadRoutes(string $routesFile = APPPATH . 'Config/Routes.php')
return $this;
}

// Include the passed in routesFile if it doesn't exist.
// Only keeping that around for BC purposes for now.
$routeFiles = $this->routeFiles;
if (! in_array($routesFile, $routeFiles, true)) {
$routeFiles[] = $routesFile;
}

// We need this var in local scope
// so route files can access it.
$routes = $this;
require $routesFile;

foreach ($routeFiles as $routesFile) {
if (! is_file($routesFile)) {
log_message('warning', 'Routes file not found: ' . $routesFile . '.');
samsonasik marked this conversation as resolved.
Show resolved Hide resolved

continue;
}

require $routesFile;
}

$this->discoverRoutes();

Expand All @@ -288,14 +322,9 @@ protected function discoverRoutes()
if ($this->moduleConfig->shouldDiscover('routes')) {
$files = $this->fileLocator->search('Config/Routes.php');

$excludes = [
APPPATH . 'Config' . DIRECTORY_SEPARATOR . 'Routes.php',
SYSTEMPATH . 'Config' . DIRECTORY_SEPARATOR . 'Routes.php',
];

foreach ($files as $file) {
// Don't include our main file again...
if (in_array($file, $excludes, true)) {
if (in_array($file, $this->routeFiles, true)) {
continue;
}

Expand Down
3 changes: 2 additions & 1 deletion tests/system/CodeIgniterTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
use Config\Cache;
use Config\Filters as FiltersConfig;
use Config\Modules;
use Config\Routing;
use Tests\Support\Filters\Customfilter;

/**
Expand Down Expand Up @@ -165,7 +166,7 @@ public function testRun404OverrideByClosure()
$_SERVER['argc'] = 2;

// Inject mock router.
$routes = new RouteCollection(Services::locator(), new Modules());
$routes = new RouteCollection(Services::locator(), new Modules(), new Routing());
$routes->setAutoRoute(false);
$routes->set404Override(static function () {
echo '404 Override by Closure.';
Expand Down
3 changes: 2 additions & 1 deletion tests/system/Commands/Utilities/Routes/FilterFinderTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
use CodeIgniter\Test\ConfigFromArrayTrait;
use Config\Filters as FiltersConfig;
use Config\Modules;
use Config\Routing;

/**
* @internal
Expand Down Expand Up @@ -52,7 +53,7 @@ protected function setUp(): void

private function createRouteCollection(array $routes = []): RouteCollection
{
$collection = new RouteCollection(Services::locator(), $this->moduleConfig);
$collection = new RouteCollection(Services::locator(), $this->moduleConfig, new Routing());

$routes = ($routes !== []) ? $routes : [
'users' => 'Users::index',
Expand Down
Loading