ice / framework

Source code of Ice framework
https://www.iceframework.org
BSD 3-Clause "New" or "Revised" License
341 stars 45 forks source link

Router, refactor #216

Closed Yahasana closed 5 years ago

Yahasana commented 5 years ago

make it faster, robust and full features.

one more thing: the old router was renamed to FastRouter. feel free to inject it into di by hand.

Yahasana commented 5 years ago

@mruz new implement for route done. do you have script to test it performance with other routers?

mruz commented 5 years ago

A good piece of work, thanks! I don't have, but we could use the ones form tests in the 1000 loop.

mruz commented 5 years ago

@Yahasana is it possible to have a static route the same as placeholder?

['GET', '/contact', ['controller' => 'index', 'action' => 'contact']],
['GET', '/{controller}', ['controller' => '[a-z]+[/]?']],

I tired with {action} too, but it's not working.

Yahasana commented 5 years ago

please check comments in Router::setRoutes L164

// the rule format: ['name' => ["httpMethod", "URI pattern", "regex", "defaults"]]

// there is no placeholder for static route, so regex is null
['GET', '/contact', null, ['controller' => 'index', 'action' => 'contact']]
mruz commented 5 years ago

It works, thanks. Is it a bug that

['GET', '/{action}', ['action' => '[aboutus|policy|terms|contact]+']],

matches /user too?

Yahasana commented 5 years ago

nope, do you add other route rules?

['GET', '/{action}', ['action' => '[aboutus|policy|terms|contact]']],

mruz commented 5 years ago

Yes, /{controller} as I sent before.

Yahasana commented 5 years ago

so /user will match the rule


['GET', '/{controller}', ['controller' => '[a-z]+[/]?']],
mruz commented 5 years ago

Routes

            ['GET', '/{module}', ['module' => '[admin|doc]+']],
            ['GET', '/{action}', ['action' => '[aboutus|policy|terms|contact]+'], ['controller' => 'index']],
            ['GET', '/{controller}', ['controller' => '[a-z]+[/]?']],

Tests

            ['/contact', ['frontend', 'index', 'contact', []]],
            ['/admin', ['admin', 'index', 'index', []]],
            ['/user', ['frontend', 'user', 'index', []]],

Result

1) Tests\RoutesTest::testUniversalGET with data set #2 ('/user', array('frontend', 'user', 'index', array()))
/user
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     0 => 'frontend'
-    1 => 'user'
-    2 => 'index'
+    1 => 'index'
+    2 => 'user'
     3 => Array ()
 )
mruz commented 5 years ago

ok, it should be:

['GET', '/{module}', ['module' => 'admin|doc']],
['GET', '/{action}', ['action' => 'aboutus|policy|terms|contact']],
['GET', '/{controller}', ['controller' => '[a-z]+[/]?']],

thanks!

Yahasana commented 5 years ago

controller should NOT contain /

['GET', '/{controller}', ['controller' => '[a-z]+']],

['GET', '/{controller}[/]', ['controller' => '[a-z]+']],