docs: Add description of conflicts between named and unnamed routes

[Elias-Black]

It fixes the case described me in the forum topic https://forum.codeigniter.com/showthread.php?tid=92672

If we have two routes and the name of the second route is the same as the path of the first route, they conflict, and the second route is not added to the list of all routes.

$routes->match(['GET', 'POST'], 'edit', 'PostController::edit');
$routes->match(['GET', 'POST'], 'edit/(:num)', 'PostController::edit/$1', ['as' => 'edit']); 

So, with this route configuration, we get a 404 on /edit/10.

[neznaika0]

It is worth adding tests if there is no case described. Add a note to changelog

[Elias-Black]

@neznaika0 yes, thank you for the advice! I was planning to add the test later, after receiving feedback, but I’ll do it now.

And am I correct in understanding that the changelog should just be left here as a message, rather than editing the CHANGELOG.md file?

If so, here it is:
fix: [Routing] a route with the same name is not blocked by a route with this path

[Elias-Black]

I also fixed DefinedRouteCollector. Previously, it would remove a route's name when it matched the route's URI.

[neznaika0]

No.Edit file https://github.com/codeigniter4/CodeIgniter4/blob/develop/user_guide_src/source/changelogs/v4.6.1.rst
I see you've fixed the tests. It may be BC and will need to be sent to the 4.7 branch. Let's wait for the team. Read it https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md

[michalsn]

I wasn't aware that a route's name is always filled. While this change may fix the bug, it may also introduce a BC break for users who took advantage of this fact in their apps.

@yllumi, I know you have a project that involves a more advanced use of Routes. Would this change affect you? Do you use route names even if they're not defined?

It would be nice to get an opinion on this from users who use Routes in more advanced ways.

Anyway, I think this should rather go into the 4.7 branch.

[yllumi]

@yllumi, I know you have a project that involves a more advanced use of Routes. Would this change affect you? Do you use route names even if they're not defined?

No @michalsn, in the projects and packages I've developed related to routing, I neither use nor modify the named route mechanism. So it seems the change above will not affect my packages.

[michalsn]

@yllumi Ok, thank you.

[neznaika0]

I always name each route. For example, according to the action: user.list user.edit user.delete. I didn't get any collisions

[michalsn]

I'm worried about cases where developers might not realize they're relying on named routes without explicitly defining them.

For example:

// Route is defined without a name:
$routes->get('edit', 'PostController::edit');

// But this still works:
d(url_to('edit'), route_to('edit'));

If we change this behavior and stop auto-filling route names, it could break code like this - especially if people have been unknowingly depending on it.

I'm not sure how common this pattern is, but it could potentially cause problems.

[Elias-Black]

// Route is defined without a name:
$routes->get('edit', 'PostController::edit');

// But this still works:
d(url_to('edit'), route_to('edit'));

Hmm, yes, this does seem dangerous.

However, keeping the current mechanics introduces a bug that causes inconsistent behavior with flawed logic, as I described in the issue.

It's a tough question🤔

[neznaika0]

There is a practice in the framework to use temporary Config/Feature.php flags. This does not mean that everything can be added there. But it allows us to experiment.

In the best case, you can replace the routes service with your own.

[michalsn]

After thinking about it, I can't decide if this should be changed. The named routes have been working this way since the beginning.

Looking at the code, it was supposed to work this way. We should work on the user guide to describe it correctly. Now, it sounds like only using the as parameter makes named routes available, which is not true.

The impact of the potential change seems too big. Not sure what others will think about the feature flag for this, but I'm not a fan.

[samsonasik]

This still needs to go to 4.7 branch, as changed behavior

[michalsn]

This still needs to go to 4.7 branch, as changed behavior

This is true, but even then - the impact of this change could be quite large - I'm not sure we should go for it.

That's why I'm trying to gather more opinions.

[paulbalandan]

Please correct me if I'm wrong. This PR introduces two changes:

1. The name of route will not be automatically filled unless as is provided.
2. Same route names for different routes are allowed.

For item 1, I can approve as it will force users to provide a route name. However, since it is a change in behavior it should go to 4.7.
For item 2, I am not a fan of name collisions. It can bring confusion to users.

[michalsn]

1. Yes.
2. No. I don't think this is possible.

I'm particularly worried about the scenario described here: #9499 (comment). Catching things like this in the existing apps may be a headache. This makes updating particularly difficult.

[Elias-Black]

@michalsn Actually, this fix will not affect the existing behavior:

// Route is defined without a name:
$routes->get('edit', 'PostController::edit');

// But this still works:
d(url_to('edit'), route_to('edit'));

This fix will only impact the following behavior:

// Route is defined without a name:
$routes->get('edit', 'PostController::edit');

// Route is defined with the name like the path above:
$routes->get('user/edit', 'UserController::edit', ['as' => 'edit']);

// But this still works:
d(url_to('edit'), route_to('edit'));

And yes, it seems like the fix of system/Router/DefinedRouteCollector.php is incorrect

[michalsn]

@Elias-Black

That's true - sorry everyone. I didn't notice that normal routes are also checked in the RouteCollection::reverseRoute().

Please change the branch to 4.7 and rebase. We will need a changelog entry here: https://github.com/codeigniter4/CodeIgniter4/blob/4.7/user_guide_src/source/changelogs/v4.7.0.rst

You should list changes under the sections: breaking changes (since name will be null for normal routes) and under the bug fixes. Please see the previous minor changelog for style reference: https://github.com/codeigniter4/CodeIgniter4/blob/4.7/user_guide_src/source/changelogs/v4.6.0.rst We will suggest changes if something is missing.

[Elias-Black]

@michalsn Oh, I discovered that my fix is order-dependent. If you declare a named route first, and then declare a route without a name but with a path that matches the name of the previous route, the latter won't work:

$routes->match(['GET', 'POST'], 'edit/(:num)', 'PostController::edit/$1', ['as' => 'edit']);
$routes->match(['GET', 'POST'], 'edit', 'PostController::edit');

// request of /edit returns 404

Sorry about that!

Actually, I can't think of a solution to this problem that wouldn't break the current incorrect behavior, where routes without a clearly defined name work as if they have a name.

Perhaps, in this case, it would be best to keep the current behavior but document it as transparently as possible.

[michalsn]

@Elias-Black Fair enough. Would you like to prepare the clarification for that in the user guide?

[michalsn]

This sounds good to me. Thank you.

[Elias-Black]

Thanks @michalsn and everyone else for the time spent!

This was important to me because I spent some time experimenting and going through the documentation due to this bug or undocumented functionality. I wouldn't want anyone else to run into the same issue.

[michalsn]

@Elias-Black We also appreciate the improvements to the user guide, as they may help many developers in the future. Thank you one more time.