klein / klein.php

A fast & flexible router
MIT License
2.66k stars 290 forks source link

routes with named parameters #331

Closed funkytaco closed 8 years ago

funkytaco commented 8 years ago

In #326 I mentioned how I was matching routes before I started using named parameters.

If I remove the if statement below, my app is slowed down by about 1200 ms.

The following used to prevent unmatched routes from being instantiated.

        if (stristr($_SERVER['REQUEST_URI'],$uri)) {
            $router->respond($method, $uri, [$injector->make($class), $classMethod]);
        }

But that means I can't use dynamic URI's, like /api/1.0/tickets/id/[*:id] because they won't ever match.

I'm thinking right now maybe I could strip out anything between square brackets before passing to the if statement but I was wondering if there was a better way to make sure all routes are matched.

Right now, all of my routes are in routes.yml and look something like this:

- [{ method: GET }, { uri: "/api/1.0/inventory/legacy/hostname/[*:hostname]" }, { class: [Main\Controllers\FooController, legacy_hostname_read] }]
Rican7 commented 8 years ago

I'm confused on what's not working for you here. Klein isn't matching or your if statement isn't working?

I'd suggest against your method of controller instantiation here anyway. You're doing it eagerly, which will be slow as it'll instantiate a new controller for each and every route that you define.

Instead, I'd have the instantiation happen lazily:

$router->respond($method, $uri, function () use ($injector, $class, $classMethod) {
    $callback = [$injector->make($class), $classMethod];

    return call_user_func_array($callback, func_get_args());
});

There's a potentially even better way of doing this that's been discussed in previous issues, for example the pattern discussed in #279.

funkytaco commented 8 years ago

on every Bootstrap it's loading all routes.

Rican7 commented 8 years ago

on every Bootstrap it's loading all routes.

Sorry, but you're going to have to be more specific. If you mean that it's creating all of the controllers, then that makes sense... you're telling it to. You're instantiating the controller inline (eagerly) not as a response callback (lazily).

funkytaco commented 8 years ago

Yep, that's the problem. Before I was using the "dynamic" uri's (e.g. [*:hostname]) it worked fine because the "if" statement was enough to keep from matching all of the URI's

I'm actually the one who asked about this in #279 :). I tried implementing that controller factory, but I couldn't get it to work.

I just tried the method you mentioned above and it seems to work! Thanks!

Rican7 commented 8 years ago

Oh wow, I didn't even notice that it was you in #279. Haha.

Well, glad it worked for you! 😃

funkytaco commented 8 years ago

Anyway I can buy you a beer or something. Thanks again!

Rican7 commented 8 years ago

Haha, sure if you'd like. Beer is always good 🍺 😄

You can do so at either https://gratipay.com/~Rican7/ or https://venmo.com/metrogeek7.

Thanks! I appreciate it! 😃