flightphp / core

An extensible micro-framework for PHP
https://docs.flightphp.com
MIT License
2.64k stars 408 forks source link

Suggestion: Adding route filters #276

Closed magnus-eriksson closed 10 months ago

magnus-eriksson commented 8 years ago

It would be really helpful to be able to add filters on specific routes, like most other routers, and not just globally on the "route"-method.

Something like this:

Flight::route('/some-url', function() {
    // Some code that won't be executed if the 
    // "before" filter returns false
}, false, ['before' => 'authRequired']);

Flight::filter('authRequired', function($route) {
    // Check or do some stuff
});

It's easier to read if you can see on each route what the filters are instead checking all the controllers (especially if you're using controller classes).

mikecao commented 8 years ago

Note sure about the ['before' => 'authRequired'] concept though. Wouldn't all these filter run before the route? Maybe accepting an array of callbacks could work.

magnus-eriksson commented 8 years ago

The idea with a "before"-filter is that it runs before the callback only when that specific route is matched. The "authRequired" is just an example. That filter would be triggered before the route callback and you can, for example, redirect the user to the log in page if the user isn't authenticated, instead of having it trigger the callback/controller.

This, and having route groups (which also could have a "before"-filter), is a pretty common pattern for routers.

For a better explanation, you can check out Laravel's documentation about route filters. It's the same concept.

kouts commented 8 years ago

That would be a great addition and especially an 'after' route filter.

n0nag0n commented 10 months ago

I think I have part of this solved with route grouping, step 2 would be adding the middleware/filters to routes and groups.https://github.com/flightphp/core/tree/route-grouping

cdsaenz commented 10 months ago

I think I have part of this solved with route grouping, step 2 would be adding the middleware/filters to routes and groups.https://github.com/flightphp/core/tree/route-grouping

You certainly have! I was just testing the new Flight 3.0, works like a charm and specially grouping is an awesome addition. Only a few months ago I moved to Codeigniter 4 and I'm using routes grouping and filters a lot. There I do something like this below, where api-auth is a FilterInterface descendant class with before and after methods. But the original suggestion here seems even easier, with 'before' and 'after' as optional callbacks. I think Laravel might make it even easier than Codeigniter with Route::filter() declarations. Note: I'm not sure that either Laravel or Codeigniter allow filters in routes, I always use them in groups.

// Codeigniter 4 
$routes->group('api', ['filter' => 'api-auth'], static function ($routes) {
});
vic4key commented 10 months ago

My simple way. Have a look. :)

Step 1. https://github.com/vic4key/sample-v3/blob/9bf1e55d59c652fabdcfffff4125f8b90975d2d3/index.php#L18 Step 2. https://github.com/vic4key/sample-v3/blob/9bf1e55d59c652fabdcfffff4125f8b90975d2d3/core/flight.php#L39C20-L39C20 Step 3. https://github.com/vic4key/sample-v3/blob/9bf1e55d59c652fabdcfffff4125f8b90975d2d3/core/security.php#L6C7-L6C7 Step 4. https://github.com/vic4key/sample-v3/blob/master/features/account/apis.php

magnus-eriksson commented 10 months ago

I think I have part of this solved with route grouping, step 2 would be adding the middleware/filters to routes and groups.https://github.com/flightphp/core/tree/route-grouping

Wow! This is dedication! I've always had a soft spot for this framework for it's easy to use, but stopped using it because of some limitations like these, so I'm excited! :-) ❤️

I'm not sure that either Laravel or Codeigniter allow filters in routes, I always use them in groups.

I'm not sure how they work today, but I know that you could add filters on specific routes back in Laravel 4.2 at leasr. It's not uncommon you want to add some specific checks (auth levels, request content type, ip-based access to certain features or similar) on multiple routes that otherwise don't have anything in common (different prefixes).

Alternative:

An alternative way of adding filters on specific routes (and to add more things like aliases that I see you have been working on) is to return the route object when creating a route, and add methods for adding filters on that directly:

Flight::route('GET /', [Foo::class, 'home'])
    ->addFilter([SomeFilter::class, 'filter'])
    ->setAlias('home');

If addFilter() also returns itself, you can chain how many filters you want on each route:

Flight::route('GET /lorem', [Foo::class, 'home'])
    ->addFilter([SomeFilter::class, 'foo'])
    ->addFilter([SomeOtherFilter::class, 'bar'])
    ->setAlias('lorem'); 

You can add more things in the future by only adding methods to the route instance.

cdsaenz commented 10 months ago

I'm not sure that either Laravel or Codeigniter allow filters in routes, I always use them in groups.

I'm not sure how they work today, but I know that you could add filters on specific routes back in Laravel 4.2 at leasr. It's not uncommon you want to add some specific checks (auth levels, request content type, ip-based access to certain features or similar) on multiple routes that otherwise don't have anything in common (different prefixes).

Well you're totally correct there. I was forcing things to work in the groups, because I didn't know (or forgot!) but in fact Codeigniter 4 can do that! This'll facilitate my work already in there :) (I think Laravel uses middleware for this)

$routes->get('admin', ' AdminController::index', ['filter' => 'admin-auth']);

Alternative:

An alternative way of adding filters on specific routes (and to add more things like aliases that I see you have been working on) is to return the route object when creating a route, and add methods for adding filters on that directly:

Flight::route('GET /', [Foo::class, 'home'])
    ->addFilter([SomeFilter::class, 'filter'])
    ->setAlias('home');

Indeed I like this syntax quite a bit more! I tested the alias (named route) option and it works fine but that 'false' in the middle makes me cringe a bit :)

$controller = new Product();
Flight::group('/products',function() use ($controller) {
Flight::route('/list', [$controller,'index'],false,'ProductsList');
Flight::route('/new', [$controller,'new'],false,'ProductsNew');
});
n0nag0n commented 10 months ago

Wow, lots of fun discussion here :)

@vic4key I think your repo is private cause it's 404's for me :grimacing:

@cdsaenz Yeah I was gonna do something similar to that. In the internal framework we created at my company we did something similar to the following that I feel flows really nicely.


// per route basis
$Router->get('/something', [ $controller, 'method' ], [ Middleware_1::class, Middleware_2::class ]);

// group a set of routes with a middleware
$Router->group('/some-prefix', function($Router) {
    $Router->get('/something', [ $controller, 'method' ], [ Middleware_3::class ]); // can still have another middleware
}, [ Middleware_1::class, Middleware_2::class ]); // applies to all routes in this group

// Middleware_1.php
class Middleware_1 {
     public function before($app) {
        // do stuff
     }

    public function after($app) {
        // do stuff
    }
}

Indeed I like this syntax quite a bit more! I tested the alias (named route) option and it works fine but that 'false' in the middle makes me cringe a bit :)

Yeah I don't love the false, but I had to remind myself this framework is an API first framework, then a do it all type templating framework second. My guess is that while aliasing is cool, it won't be used as much as it would be on another framework.

@magnus-eriksson I actually like your implementation. That might be the way around the weird $pass_route = false issue that @cdsaenz was mentioning. I thought about just overriding $pass_route and if bool do this, if string consider it an alias. Unfortunately experience has taught me that implying stuff causes so many problems; being explicit is MUCH better.

cdsaenz commented 10 months ago

@cdsaenz Yeah I was gonna do something similar to that. In the internal framework we created at my company we did something similar to the following that I feel flows really nicely.

Yeah looks good!

Yeah I don't love the false, but I had to remind myself this framework is an API first framework, then a do it all type templating framework second. My guess is that while aliasing is cool, it won't be used as much as it would be on another framework.

Yeah it's good to remind ourselves the end goal and the use case for Flight. My view is that it should be an stellar Router. YET, easy to implement, and thin: I don't think we need niceties like a 'Class::method' string as route endpoint, for example. Beyond that, the set/get are useful, the json response (yeah API first framework makes sense) and a simple view/render method. Then just the map/register powerful utilities, all of which I'm using to plug-in Twig.

About named routes/aliases, I use them a lot but you're right, perhaps it's because of Codeigniter habits, and the CRUD stuff I do there. But it's so nice to have. In fact In twig I created a filter for "url_to". It's all an analysis of the possibilities of course :) I've even added Eloquent and it's fun.. But not sure I'd ever use such a monster or even Twig. But what a test it is!

        // route url
        $twig->addFilter(new \Twig\TwigFilter('url_to', function ($alias, array $parameters = []) {            
            return ROOTURL . Flight::router()->getUrlByAlias($alias,$parameters);
        }));

@magnus-eriksson I actually like your implementation. That might be the way around the weird $pass_route = false issue that > @cdsaenz was mentioning. I thought about just overriding $pass_route and if bool do this, if string consider it an alias. Unfortunately experience has taught me that implying stuff causes so many problems; being explicit is MUCH better.

Yeah totally, I don't think $pass_route as bool or string is clear enough, though used in other frameworks, and it's valid. Implying means more guessing, and thus, a more steep learning curve. I'd suggest you keep it as it is now and eventually do what @magnus-eriksson suggested, and let both methods work. We all love chaining I think, it's clear and elegant.

n0nag0n commented 10 months ago

Here's an attempt at middleware. I think I want to look at how the dispatcher->filter stuff works more before I commit to this setup. It has some basic tests that are running. I will need to do a lot more testing, esp with nested and group middleware.

One thing to point out, if you use a function() { } for your middleware, you can only execute the before method (unless you guys feel both should be executed somehow). The other thing to be aware, if you do this as a group middleware and you need to pull params from the route, you likely are going to have to do something like func_get_vars() to pull out the params as the order of the params may change.

https://github.com/flightphp/core/pull/514

eydun commented 10 months ago

@cdsaenz

Yeah it's good to remind ourselves the end goal and the use case for Flight.

That is a very important question. It is not a good idea to build without a clear vision.

@magnus-eriksson

Wow! This is dedication! I've always had a soft spot for this framework for it's easy to use, but stopped using it because of some limitations like these, so I'm excited! :-) ❤️

I am here on the same page as @magnus-eriksson. For me Flight was the framework with the most shallow learning curve.

In regards to Routes and Filters we should ask, what is the simplest and most logical syntax?

n0nag0n commented 10 months ago

Middleware is now on 3.1.0. Give it a try!