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

Route filters #500

Open
happyDemon opened this issue May 24, 2014 · 13 comments
Open

Route filters #500

happyDemon opened this issue May 24, 2014 · 13 comments
Milestone

Comments

@happyDemon
Copy link

This is a feature I found handy while using Laravel.
Instead of having to inject the before method of controllers it could be nice to define route filters somewhere and assign them to the routes that need them (including a global route filter).

There's an added bonus for module developer that could bundle them, leaving them optional to use by other developer (instead of hardcoding them in a base controller).

http://laravel.com/docs/routing#route-filters for details

***edit
This would be for milestone 3.4 obvisously (forgot to add that to this ticket)

@WinterSilence
Copy link

better to pass a string with the function body :

$filter = 'return $request->method() != Request::POST ? FALSE : $params';

and use create_function:

$this->filters[] = create_function('$route, $params, $request', $filter);

firstly it will avoid problems with the serialization of caching, and secondly less resources.

@happyDemon
Copy link
Author

It would be the point to store the filters somewhere, e.g. in a module's init.php:
Route::filter('name_of_filter', function($route, $request){ //do some stuff });

And call the filter when needed e.g.

Route::set('default', '(<controller>(/<action>(/<id>)))')
        ->filter('name_of_filter')
    ->defaults(array(
        'controller' => 'welcome',
        'action'     => 'index',
    ));

There's no need to provide the $params variable since you can get that from the request object.

The filters themselves wouldn't need to be serialized, only the name that's stored with the route. There's no need to call create_function. I don't see a single app using dozens of filters. Unless I'm missing what you mean.

Also there's no need to return anything. It would be wiser to Throw a HTTP exception if something went wrong or redirect when needed.

@WinterSilence
Copy link

@happyDemon mistakes:

The filters themselves wouldn't need to be serialized, only the name that's stored with the route.

Routes serialized at caching https://github.com/kohana/core/blob/3.4/develop/classes/Kohana/Route.php#L165

Also there's no need to return anything. It would be wiser to Throw a HTTP exception if something went wrong or redirect when needed.

It's true only if route used single filter, see https://github.com/kohana/core/blob/3.4/develop/classes/Kohana/Route.php#L460

There's no need to provide the $params variable since you can get that from the request object.

this request not contain params yet, also see previous comment

@kemo
Copy link
Member

kemo commented May 25, 2014

@WinterSilence routes can be cached, which doesn't mean filters have to be cached with them as well. There's virtually no overhead defining an anonymous function (and remember, the filter doesn't have to be a 'lambda', can be defined as a call to a class method).

One thing I don't like about Laravel's "filters" is that they don't only filter, they do more than that. Maybe there should be a different name for this?

@WinterSilence
Copy link

  1. Need to split the class code to classes: Router (collection of routes) and Route.
  2. Need fix: values ​​passed to Route::defaults() and 'Route::uri()' are not checked for compliance with Route::$_regex
  3. Does it make sense to store routes as objects? Would not it be faster to store data in an array and return the object in Router::get()

https://gist.github.com/WinterSilence/437395f886177017d4bc my earlier version

@kemo

routes can be cached, which doesn't mean filters have to be cached with them as well.

Then caching will not make sense. Add example, maybe I did not understand you.

the filter doesn't have to be a 'lambda'

I suggest using a string containing the function body (and use create_function()) instead of closure, then will not have problems with caching: at serializing closures replaced by the original strings (they stored in special property)

One thing I don't like about Laravel's "filters" is that they don't only filter, they do more than that.

Filters are rarely used, so no need to expand their opportunities.

@WinterSilence
Copy link

Create class wrapper for create_function: https://gist.github.com/WinterSilence/604e1cd86e242e592795

@shadowhand
Copy link
Contributor

Using create_function is not an acceptable solution for anything.

@WinterSilence
Copy link

@shadowhand Why do you think so?

@kemo
Copy link
Member

kemo commented May 27, 2014

@WinterSilence because it's a hack, because you write PHP code in a string and because it's simply not the way Kohana does things. + There is no need to cache methods.

@neo22s
Copy link
Member

neo22s commented May 27, 2014

Seems also that garbage collector doesnt work there.
On May 27, 2014 11:28 PM, "Kemal Delalic" notifications@github.com wrote:

@WinterSilence https://github.com/WinterSilence because it's a hack,
because you write PHP code in a string and because it's simply not the way
Kohana does things. There is no need to cache methods.


Reply to this email directly or view it on GitHubhttps://github.com//issues/500#issuecomment-44337947
.

@happyDemon
Copy link
Author

I'm willing to have a go at this and have a pull request ready in a few weeks.

The main reason I'd like this is because I always seem to be hacking controllers' before method to serve specific needs for several of its actions (when these specific functionality actually should be defined on route level).

@lenton
Copy link
Contributor

lenton commented Jun 14, 2014

@happyDemon Is the problem you're trying to solve: not being able to use different before/after methods for specific actions and/or specifying which actions should/shouldn't run the before/after methods?

@enov enov added this to the 3.4.0 milestone Nov 28, 2014
@moon0326
Copy link

moon0326 commented Feb 6, 2015

I used to use my own version of route module that supports filters. I used it only for just a month or so then stopped using it since I've moved on to a new framework.

if you're interested in, checkout https://github.com/moon0326/Kohana-Route-Extended

@neo22s neo22s modified the milestones: 5.0.0, 3.4.0 Mar 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants