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: new improved auto router #5889

Merged
merged 30 commits into from
May 3, 2022

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Apr 9, 2022

Needs to rebase after merging #5877

Description
An optional new more secure auto router.

  • add AutoRouterImproved class and AutoRouterInterface
  • add Config\Feature::$autoRoutesImproved (false by default)
  • use AutoRouterImproved in Router if enabled

AutoRouterImproved changes:

  • A controller method needs HTTP verb prefix like getIndex(), postCreate().
    • Developers always know HTTP method, so requests by unexpected HTTP method does not happen.
  • The Default Controller (home by default) and the Default Method (index by default) must be omitted in the URI.
    • Restrict one-to-one correspondence between controller methods and URIs.
    • By default, you can access /, but /home and /home/index are 404.
  • It checks method parameter count.
    • If there are more parameters in the URI than the method parameters, it results in 404.
  • It does not support _remap() method.
    • Restrict one-to-one correspondence between controller methods and URIs.
  • Can't access controllers in Defined Routes.
    • Completely separates controllers accessible via Auto Routes from those accessible via Defined Routes.

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 marked this pull request as draft April 9, 2022 10:18
@kenjis kenjis added the enhancement PRs that improve existing functionalities label Apr 9, 2022
@kenjis kenjis force-pushed the feat-new-auto-router branch 3 times, most recently from 80721ed to f6e101d Compare April 9, 2022 11:27
@kenjis kenjis added new feature PRs for new features and removed enhancement PRs that improve existing functionalities labels Apr 9, 2022
@MGatner
Copy link
Member

MGatner commented Apr 10, 2022

This might be a wildly-inappropriate idea but... in addition to the method-naming convention (get___()) what about using attributes to signify to the improved auto-router that "this is an expected route"? You could probably reuse some of your code from the routes-generating library.

We would need something like "if PHP version > 8", and I'm not even sure if you can handle attributes conditionally with valid syntax... but that's why it is wild. 😅

@kenjis
Copy link
Member Author

kenjis commented Apr 10, 2022

In my opinion or modeling, there are two routers.

  1. Defined Route Router
  2. Auto Route Router

Defined Route Router has RouteCollection.
Attributes for routes are related in (1).
CodeIgniter4 Attribute Routes is just a command to generate a part of RouteCollection (Config/Routes.php).

So if we agree with "if PHP version >= 8" and adding Attributes for routes, I could send another PR for it.

I'm not even sure if you can handle attributes conditionally with valid syntax...

No problem. Attributes are just PHP comments.
When using PHP7, new Reflection methods for attributes are just not supported.
Checking PHP version before processing Attributes is enough.

@kenjis kenjis changed the title feat: new auto router feat: new improved auto router Apr 10, 2022
@MGatner
Copy link
Member

MGatner commented Apr 11, 2022

I think for sure in version 5 we should have a Router interface or abstract class with multiple different handlers. They could cascade or whatever to support different definitions at once.

Let's see what Lonnie has to say on the other PR before you go doing a bunch more work on this one.

@kenjis
Copy link
Member Author

kenjis commented Apr 12, 2022

@samsonasik Do you know why?

Run vendor/bin/rector process --dry-run --no-progress-bar

Error: ] Could not process "tests/system/Router/AutoRouterImprovedTest.php"     
         file, due to:                                                          
         "System error: "Class 'App\Controllers\BaseController' not found"      
         Run Rector with "--debug" option and post the report here:             
         https://github.com/rectorphp/rector/issues/new". On line: 16           

Error: Process completed with exit code 1.

https://github.com/codeigniter4/CodeIgniter4/runs/5984021110?check_suite_focus=true

I ran it with --debug, but nothing showed.

$ vendor/bin/rector process --debug tests/system/Router/AutoRouterImprovedTest.php
 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
                                                                                                                        
 [ERROR] Could not process "tests/system/Router/AutoRouterImprovedTest.php" file, due to:                               
         "System error: "Class "App\Controllers\BaseController" not found"                                              
         Run Rector with "--debug" option and post the report here: https://github.com/rectorphp/rector/issues/new". On 
         line: 16                                                                                                       

@samsonasik
Copy link
Member

@kenjis I am not sure, probably it tries to load App\Controllers\BaseController while autoload is overlapped in the AutoRouterImprovedTest, the possible solution is register to rector.php's autoload path or in composer.json files or classmap config ref https://github.com/rectorphp/rector/blob/main/docs/static_reflection_and_autoload.md#dealing-with-class--was-not-found-while-trying-to-analyse-it

@kenjis
Copy link
Member Author

kenjis commented Apr 13, 2022

@kenjis
Copy link
Member Author

kenjis commented Apr 13, 2022

@kenjis
Copy link
Member Author

kenjis commented Apr 13, 2022

--- a/rector.php
+++ b/rector.php
@@ -126,6 +126,10 @@ return static function (ContainerConfigurator $containerConfigurator): void {
     // auto import fully qualified class names
     $parameters->set(Option::AUTO_IMPORT_NAMES, true);
 
+    $parameters->set(Option::AUTOLOAD_PATHS, [
+        __DIR__.'/app/Controllers/BaseController.php',
+    ]);
+
     $services = $containerConfigurator->services();
     $services->set(UnderscoreToCamelCaseVariableNameRector::class);
     $services->set(SimplifyUselessVariableRector::class);

But nothing changes.

$ vendor/bin/rector process --debug tests/system/Router/AutoRouterImprovedTest.php 
 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
                                                                                                                        
 [ERROR] Could not process "tests/system/Router/AutoRouterImprovedTest.php" file, due to:                               
         "System error: "Class "App\Controllers\BaseController" not found"                                              
         Run Rector with "--debug" option and post the report here: https://github.com/rectorphp/rector/issues/new". On 
         line: 16

@kenjis
Copy link
Member Author

kenjis commented Apr 13, 2022

@samsonasik I solved the problem.

The controllers that test case use extended App\Controllers\BaseController.
But App is not autoloadable by Composer.
I replaced BaseController with CodeIgniter\Controller.

Thanks for the help.

@samsonasik
Copy link
Member

@kenjis RectorConfig is extended class of ContainerConfigurator, only in dev-main for now, the old usage still working, the new RectorConfig will use explicitly more method call usage on it instead, ref https://github.com/rectorphp/rector-src/blob/dd96863b8ef3b8a86d9268e9e2f7bb5632dfb375/packages/Config/RectorConfig.php#L24

app/Config/Feature.php Outdated Show resolved Hide resolved
app/Config/Feature.php Outdated Show resolved Hide resolved
@kenjis kenjis marked this pull request as ready for review April 20, 2022 08:56
@kenjis
Copy link
Member Author

kenjis commented Apr 20, 2022

If everything looks Okay, I will add the documentation.

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@kenjis
Copy link
Member Author

kenjis commented Apr 26, 2022

Added the user guide updates. Review, please.

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

Some comments. I like the direction this is heading and think it is very close. I haven't tested it myself in a real app yet but I would like to (or someone else to).

This is a fairly large change and will inevitably generate forum questions and issues so let's try to get the whole team to look at it. The new docs helped me understand it much better - very nice work on those.

app/Config/Feature.php Outdated Show resolved Hide resolved
system/Router/AutoRouter.php Show resolved Hide resolved
system/Router/AutoRouterImproved.php Outdated Show resolved Hide resolved
system/Router/AutoRouterImproved.php Show resolved Hide resolved
user_guide_src/source/changelogs/v4.2.0.rst Outdated Show resolved Hide resolved
user_guide_src/source/changelogs/v4.2.0.rst Show resolved Hide resolved
@kenjis kenjis merged commit ab63c8d into codeigniter4:develop May 3, 2022
@kenjis kenjis deleted the feat-new-auto-router branch May 3, 2022 11:17
@element-code
Copy link
Contributor

Why is the AutoRouterImproved marked as final and not extendable in any way?
It also doesn't use the controllerName/methodName of the router, and instead implements its own logic.

@kenjis
Copy link
Member Author

kenjis commented Mar 20, 2023

Because it is better to implement the interface, not extending the class.
But if you really need to extend it, please create an issue and explain what you extend and why.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature PRs for new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants