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

feat: [Auto Routing Improved] fallback to default controller's default method #7406

Merged

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Apr 4, 2023

Description
Related #7162

You can pass arguments to the default method of the default controller.

When you have News\Home and public function getIndex($id = null) in it:

  • GET /news/101
    • News not found
    • News\101 not found (invalid)
    • News\101\Home not found (invalid)
    • News\Home found
    • News\Home::getIndex() found
    • → run News\Home::getIndex('101')

This PR makes it possible that one controller one URI.

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 kenjis added enhancement PRs that improve existing functionalities 4.4 labels Apr 4, 2023
@kenjis kenjis marked this pull request as draft April 10, 2023 22:56
@kenjis kenjis force-pushed the feat-auto-routes-fallback-to-default-controller branch from 937cf28 to 18b3db1 Compare April 11, 2023 07:00
@kenjis kenjis changed the title feat: [Auto Routing Improved] fallback to default controller feat: [Auto Routing Improved] fallback to default controller's default method Apr 11, 2023
@kenjis kenjis marked this pull request as ready for review April 11, 2023 08:29
@kenjis kenjis force-pushed the feat-auto-routes-fallback-to-default-controller branch 2 times, most recently from df966fa to df04a9f Compare April 13, 2023 09:07
@kenjis kenjis force-pushed the feat-auto-routes-fallback-to-default-controller branch from df04a9f to cc6b2d9 Compare April 13, 2023 09:30
Cannot declare class CodeIgniter\Router\Controllers\Index
@kenjis
Copy link
Member Author

kenjis commented May 24, 2023

I would like to merge this. Can someone review?

@iRedds
Copy link
Collaborator

iRedds commented May 24, 2023

I think this autorouting is too auto.
For example, the News::getIndex() method returns a list of news.
If I didn't specify the /news/(:num) route, then I would expect a request to the URL /news/15 to return an error.
Now I have to check the argument in the getIndex() method in order to get a 404 error for the URL /news/15.

It seems to me that such a change will lead to incorrect operation of existing applications.

If I compared on a scale of breaking changes, then I would rate this PR as 10 out of 10, and for example, changing the interface in the core of the framework 6 out of 10.

@kenjis
Copy link
Member Author

kenjis commented May 24, 2023

If the getIndex() method has no parameter, /news/15 is not routed to the controller.
/news is routed to. Because Auto Routing Improved checks method parameter count.
If there are more parameters in the URI than the method parameters, it results in 404.

There is no way to specify the type of a parameter like /news/(:num) in Auto Routing.

I do not think this PR (or #7162) to be a breaking change.
Give use cases where existing apps would be broken.
I will consider it.

@iRedds
Copy link
Collaborator

iRedds commented May 24, 2023

I haven't looked at the Autorouter code because I think it's a waste of core team resources. Therefore, my comment was based only on the concept proposed in the PR. But after your answer, I saw that the code uses ReflectionClass.
While I still think the framework's support for autorouting is a waste of resources, I have no more questions about the concept of this PR.

@kenjis
Copy link
Member Author

kenjis commented May 26, 2023

Defining all routes is tedious.
Many sites do not need flexible routing that defined routes provide.
Even if needed, most routes can be handled by auto routing.
So I don't want to remove auto routing feature.

@kenjis kenjis merged commit 3565864 into codeigniter4:4.4 Jun 1, 2023
@kenjis kenjis deleted the feat-auto-routes-fallback-to-default-controller branch June 1, 2023 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.4 enhancement PRs that improve existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants