Clean router config

[lonnieezell]

The router file had way too much information in it, from walls of comments with descriptions of settings, to settings, and including additional files. This PR cleans this up so that the Routes.php file is only for adding routes. It does this by:

1. Creating a new config file, app/Config/Routing.php. All of the settings that were set by default within the old Routes file has been moved to this config file.
2. The Routes file has been moved to app/Routes.php to make it more prominent, and reduce confusion by having two closely-named files.
3. Routes.php files within modules are now expected to be at the root of the module directory, though this is easily changed.
4. A new setting within the Routing config file allows existing applications to simply point to the old Routes file if they don't want to move it. This also allows the user to specify multiple routes files to include if they want to separate routes logically, like into an api and web files, etc.
5. A new setting within the Routing config file allows setting the path to module config files. This now also defaults to {module}/Routes.php.

UPGRADING EXISTING PROJECTS:

The process for upgrading an existing project should be simple:

1. copy the Routing.php config file into the project folder.
2. edit the $routeFiles to point to the existing file (or move the existing file into the app folder)
3. edit the $modulePath to be Config/Routes.php (or move any existing files up to the module root folder)

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[kenjis]

This is not a trivial change. It should go to 4.4 branch.

@lonnieezell Don't use wip as a commit message, please.
It does not tell anything.

[iRedds]

I don't think this PR is a good decision.

About configs.
In CodeIgniter, configs are not cached, which means that with each request, each loaded config inherited from CodeIgniter\Config will try to find the class properties in .env.
+1 to overhead

Almost all configs used in class dependencies are used as DTO, overriding class properties.
The meaning of using the config-class is lost. The same can be obtained using an array.

About routes.
All settings from the routes file can be removed, since they duplicate the state of the properties of the RouteCollection class. Except for the namespace, but it can be defined in the class by default.
The framework has documentation, so extra comments can be removed from the routes file.

Also, the presence of two files associated with routing can cause confusion.
Especially in modules, the position of the file with routes remains the same.

[kenjis]

Almost all configs used in class dependencies are used as DTO, overriding class properties.
The meaning of using the config-class is lost. The same can be obtained using an array.

What is the meaning of using the config-class?

And I know you loves array config, but we have been using config-class already.
Changing to an array would be a breaking change. Is it worth the change?

[kenjis]

Especially in modules, the position of the file with routes remains the same.

This is a good point.
If it's in Acme/Config/Routes.php in a module, but in app/Routes.php in the app, that's odd.

[lonnieezell]

It should go to 4.4 branch.

Good point! I'll fix that up.

All settings from the routes file can be removed...

That's true, I could just remove those settings all together and document how to call the methods within the Routes file. I have two issues with that, though.

First - the current Routes file isn't what I would consider a config file, honestly. It was left in that folder for purely historical reasons during the rewrite.

Second - this is the first of a two-step process to make the Router more flexible, converting it to use Handlers for the 2 distinct routing systems we have, but expandable with dev-provided routers. I've heard devs searching for a way to get routes dynamically from the database for years, and this would allow them to easily handle that if they needed, as well as allow others to create other routing if they needed. Knowing this, we need a place to configure the handlers that are in use. This could be handled in the Services class, I guess, though that doesn't feel right when you look at other portions that use handlers, like the Cache, Session, or Database.

Especially in modules, the position of the file with routes remains the same.

Good point. I missed that. I see three options:

1. Move Routes file back to the Config folder
2. Look for module Routes file in the main module
3. Add a separate config item that specifies where they should exist in Modules, allowing a move to the module root, but an easy setting during an upgrade for existing projects.

Thoughts?

[iRedds]

@kenjis You're exaggerating when you say I love arrays. I believe that arrays are the most optimal format for configuration. A class has only one advantage - it can be specified as a dependency.

Of course, in this version of the framework, changing classes to arrays is unacceptable, but I still hope that a discussion about switching to arrays is possible in the next version.

If we talk about the use of classes, then in my opinion a very strange approach is used.

// instead of
$this->a = $config->a;
$this->b = $config->b;

// better
$this->config = $config;

Since the config is in dependencies, without it we will not be able to create an instance of the class. Then it makes no sense to duplicate class properties in the config and we can use the config directly.

[iRedds]

That's true, I could just remove those settings all together and document how to call the methods within the Routes file. I have two issues with that, though.

First - the current Routes file isn't what I would consider a config file, honestly. It was left in that folder for purely historical reasons during the rewrite.

Precisely because they tried to make CI4 more like CI3 and used its code base, we are now in a lot of trouble.
Let's take the config out of the application directory, and the routes into a separate directory.

Second - this is the first of a two-step process to make the Router more flexible, converting it to use Handlers for the 2

If you are talking about flexibility, then it already exists. Services! Though I still think ServiceContainer and Dependency Autowiring would be the best solution.

If the developer wants to change the behavior of the router, then he can replace the service with his own implementation.
When there is a lot of flexibility where it is not needed, this is a problem.

Take for example the Model. It serves arrays, stdClass and Entity. Everything seems to be fine, but when adding features, you have to service all return types. Which increases the code base and complicates maintenance.

Especially in modules, the position of the file with routes remains the same.

Good point. I missed that. I see three options:

I think a single configuration point should be used for modules. Something similar to Registrars and Laravel ServiceProvider.

namespace Module;

class Config
{
    public function boot()
    {
        // This is where we define routes, filters, and other available settings.
        // Instead of separate files that repeat the names of the application configs.
    }
}

[kenjis]

@iRedds
The following code is needed because the $config has items that the class does not need.

$this->a = $config->a

A Config class should be a dependency for one class. If it is used in more than one class, it is not good.
If one Config class is for one class, we can use:

$this->config = $config;

I did it in #7255 Why don't you approve it?

[kenjis]

Look for module Routes file in the main module

What is the main module?

I feel app/Routes.php is strange. Why no directory?
What if we have multiple routes files?

[iRedds]

@kenjis To be honest, I don't quite understand what happened. That is, in CI3 the session settings were in the main config and everything worked, but in CI4 it does not work.

[kenjis]

@iRedds That is because CI4 uses Config classes.

In the current implementation, the Session class constructor requires Config\App object. If you want to use Session object in Config\App to set a property, It will be an infinite loop.
See this error: #7308 (comment)

This issue does not occur in array config because there is no instantiation.
It may be one advantage of array config.

[iRedds]

@kenjis Forbid using the constructor in configs.
Otherwise, any class called in Config\App::__construct() that references Config\App in its constructor will cause an infinite loop.

[kenjis]

@iRedds I created #7388

[lonnieezell]

A class has only one advantage - it can be specified as a dependency.

The original reason Narf and I discussed using classes as config files years ago was that it could be loaded instantly from anywhere. Obviously, the framework has expanded and the Factory class now handles that to ensure it's only ever loaded once for performance, so it might be valid to rethink for v5. But that's not relevant to this PR.

I think a single configuration point should be used for modules.

Interesting idea, but it feels more relevant to a plug-in system for a CMS or other app. This would warrant other discussion, but is a huge breaking change and not something we can handle within this PR.

I feel app/Routes.php is strange. Why no directory?

I was hesitant to create a new directory for a single file. If we had logic around the handling of different specific route file types, like Laravel has bulit-in logic around web.php and api.php route files, than it would make sense. As it stands, it is only a single file.

What if we have multiple routes files?

Their locations can all be specified within the config file. And the current one can be moved to join them just by changing its path in the config file.

If the developer wants to change the behavior of the router, then he can replace the service with his own implementation. When there is a lot of flexibility where it is not needed, this is a problem.

To some extent I agree with this direction. The three problems I'm struggling with are:

1. It's not consistent with other portions of the framework. Lack of consistency is also a problem. And I'm sure you'll now point out areas that aren't quite consistent, which I'm sure we'd all love to fix at some point, but I'd rather not create MORE inconsistencies.
2. Like I mentioned, a large part of the reason for doing this (though not the only one) was to split the main routing and the auto-routing into different handlers. I would hate to replace the current functionality of "setAutoRoute(true)" to become one of "copy the Service over to the App space, and modify it to add in these two handlers". That's too large of a BC break for a point release.
3. I would really love for the Routes file itself to just be route definitions and not have config settings in it also. I know you mentioned removing the docs would clean the file up, and it would to some extent, but I think it is mixing concerns right now that it shouldn't be.

@MGatner @michalsn what are your thoughts on these changes?

[iRedds]

Interesting idea, but it feels more relevant to a plug-in system for a CMS or other app. This would warrant other discussion, but is a huge breaking change and not something we can handle within this PR.

Tell me, how is the single point for the configuration different from the current state? In the technical part.
To add routes in a module, you need a Config/Routes file, for services Config/Services, for filters Config/Filters, etc.

That is, in order for the framework to process these files, they must comply with certain rules. How is this different from a plug-in system for a CMS?

You are talking about splitting routing handlers.

For example, for predefined routes, there is no point in a default route.
And for autorouting there is no point in namespace.
It turns out that these settings in the config will be redundant for different handlers.

On the other hand, based on the experience of CI3, even with autorouting, it is necessary to be able to define routes manually. Is it possible to separate handlers in this case?

[kenjis]

It turns out that these settings in the config will be redundant for different handlers.

Each route handler has its own settings.

On the other hand, based on the experience of CI3, even with autorouting, it is necessary to be able to define routes manually. Is it possible to separate handlers in this case?

In CI3 auto-routing is primary routing system, and there is no way to turn it off.
And it is not flexible as much as defined routes, so defined routes are necessary.

But if you define all routes, you don't need to use auto-routing at all.
CI4 can disable auto-routing.
If your URI design is very simple, you may not need to use defined routes at all.
If a site does not need defined routes, I would like to disable it.

[lonnieezell]

Ok, so without re-inventing the core of the framework here, what are people leaning toward? Please think solutions that are consistent within the framework as it exists now. Or should we just close this PR? I'm fine with that if it's too controversial, though it would have been nice to get input from other core team members.

[samsonasik]

I prefer go to 5.0 for this change, except, deprecate existing functionality to keep working while suggesting new feature.

[michalsn]

I'm a bit worried about the BC break part this PR brings when upgrading the framework, but overall I like the idea.

Routes inside the modules should be auto-discovered as before, though I'm unsure how we should specify the default location after these changes.

If we make a clear path of how to upgrade, I'm okay with including this in the v4.4 release.

[kenjis]

I would like to get this change into 4.4 with no breaking changes.

[lonnieezell]

I've rebased to 4.4 branch. I also made the path within any "modules" configurable. This migration should be pretty painless - see the instructions I added to the PR description for details).

[kenjis]

Defaulting to {module}/Routes.php will break all existing Composer packages with routes.

Wouldn't it be better to leave the default settings under Config?

• app/Config/Routes.php
• {module}/Config/Routes.php

It doesn't seem worth breaking all the existing packages and changing to {module}/Routes.php.

[kenjis]

Rebased and updated failed tests, and added documentation a bit.

[kenjis]

I think this is ready to merge. @codeigniter4/core-team

[michalsn]

It seems ready to me.