-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
fix: route options are not merged (outer filter is merged with inner filter) #8033
Conversation
In this case, there is no change. $routes->group('', ['filter' => 'csrf'], function ($routes) {
$routes->get('dashboard', function (){
echo "CSRF is working";
});
$routes->group('profile', function($routes){
$routes->get('/', function (){
echo "CSRF is working";
});
});
}); Before:
After:
|
In this case, the outer filter will be applied to the inner route. $routes->group('', ['filter' => 'csrf'], function ($routes) {
$routes->get('dashboard', function (){
echo "CSRF is working";
});
$routes->group('profile', ['namespace' => 'Foo\Controller'], function($routes){
$routes->get('/', function (){
echo "CSRF is working";
});
});
}); Before:
After:
|
In this case, the outer filter will be merged with the inner filter. $routes->group('', ['filter' => 'csrf'], function ($routes) {
$routes->get('dashboard', function (){
echo "CSRF is working";
});
$routes->group('profile', ['filter' => 'honeypot'], function($routes){
$routes->get('/', function (){
echo "CSRF is working";
});
});
}); Before:
After:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was leaning towards the "none" approach but this is probably more logical for people who don't want to dig.
Co-authored-by: MGatner <[email protected]>
Namespace for inner groups ignored. You merged full |
@neznaika0 What do you mean by "Namespace for inner groups ignored"? $routes->group('', ['filter' => 'csrf'], function ($routes) {
$routes->get('dashboard', 'Outer::index');
$routes->group('profile', ['namespace' => 'Foo\Controller'], function($routes) {
$routes->get('/', 'Inner::index');
});
}); +--------+-----------+------+-------------------------------+----------------+---------------+
| Method | Route | Name | Handler | Before Filters | After Filters |
+--------+-----------+------+-------------------------------+----------------+---------------+
| GET | / | » | \App\Controllers\Home::index | | toolbar |
| GET | dashboard | » | \App\Controllers\Outer::index | csrf | csrf toolbar |
| GET | profile | » | \Foo\Controller\Inner::index | csrf | csrf toolbar |
+--------+-----------+------+-------------------------------+----------------+---------------+ |
I didn't see |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems more logical than the alternative or current behavior. Thank you.
Yes, this is logical, understandable, and less of a surprise to developers. |
Description
Supersedes #7981
Fixes #7963
If routes are grouped, the outer filter should be applied to all routes within.
If there is a filter that should be excluded on an inner route, that route should not be defined within the group.
group()
options with innergroup()
optionsChecklist: