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

Bug: route group filter bug #7963

Closed
sajibAdhi opened this issue Sep 23, 2023 · 13 comments
Closed

Bug: route group filter bug #7963

sajibAdhi opened this issue Sep 23, 2023 · 13 comments
Labels
bug Verified issues on the current code behavior or pull requests that will fix them

Comments

@sajibAdhi
Copy link

sajibAdhi commented Sep 23, 2023

PHP Version

8.2

CodeIgniter4 Version

4.4.1

CodeIgniter4 Installation Method

Composer (using codeigniter4/appstarter)

Which operating systems have you tested for this bug?

Windows, Linux

Which server did you use?

apache

Database

MySQL 5.6

What happened?

I was using filters in Grouping Routes which was working properly in nested routes and nested grouping routes. But when I used a namespace in nested grouping routes the filter stopped working for nested grouping routes.

Steps to Reproduce

$routes->group('', ['filter' => AuthenticationFilter::class], function ($routes) {
    $routes->get('dashboard', function (){
        echo "AuthenticationFilter is working";
    });
    
    $routes->group('profile', ['namespace' => 'App\Controllers\Profile'], function($routes){
        $routes->get('/', function (){
            echo "AuthenticationFilter is not working";
        });
    });
});

Expected Output

I am expecting the auth filter to be called before the profile request.

Anything else?

No response

@sajibAdhi sajibAdhi added the bug Verified issues on the current code behavior or pull requests that will fix them label Sep 23, 2023
@kenjis
Copy link
Member

kenjis commented Sep 23, 2023

Note
Options passed to the outer group() (for example namespace and filter) are not merged with the inner group() options.
https://codeigniter4.github.io/CodeIgniter4/incoming/routing.html#nesting-groups

This note seems to explain if you specify a filter in the outer group, the filter is not merged into the filters in the inner group.
See #3985

@kenjis kenjis removed the bug Verified issues on the current code behavior or pull requests that will fix them label Sep 23, 2023
@sajibAdhi
Copy link
Author

Note
Options passed to the outer group() (for example namespace and filter) are not merged with the inner group() options.
https://codeigniter4.github.io/CodeIgniter4/incoming/routing.html#nesting-groups

Oh!! 😭 😭 It's kind of pain.

@kenjis
Copy link
Member

kenjis commented Sep 23, 2023

But the current behavior is confusing.

If we don't set options in the inner group, the outer group options are applied.

$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";
        });
    });
});
+--------+-----------+------+------------------------------+----------------+---------------+
| Method | Route     | Name | Handler                      | Before Filters | After Filters |
+--------+-----------+------+------------------------------+----------------+---------------+
| GET    | /         | »    | \App\Controllers\Home::index |                | toolbar       |
| GET    | dashboard | »    | (Closure)                    | csrf           | csrf toolbar  |
| GET    | profile   | »    | (Closure)                    | csrf           | csrf toolbar  |
+--------+-----------+------+------------------------------+----------------+---------------+

If we set options array in the inner group, the outer group options are not applied.

$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 not working";
        });
    });
});
+--------+-----------+------+------------------------------+----------------+---------------+
| Method | Route     | Name | Handler                      | Before Filters | After Filters |
+--------+-----------+------+------------------------------+----------------+---------------+
| GET    | /         | »    | \App\Controllers\Home::index |                | toolbar       |
| GET    | dashboard | »    | (Closure)                    | csrf           | csrf toolbar  |
| GET    | profile   | »    | (Closure)                    |                | toolbar       |
+--------+-----------+------+------------------------------+----------------+---------------+

@kenjis kenjis reopened this Sep 23, 2023
@kenjis kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Sep 23, 2023
@kenjis
Copy link
Member

kenjis commented Sep 23, 2023

@codeigniter4/core-team
Why the options array passed to the outer group() is not merged with the inner group() options array?

$this->currentOptions = array_shift($params);

@michalsn
Copy link
Member

Dunno, sounds like a bug... even though it's documented.

@kenjis
Copy link
Member

kenjis commented Sep 26, 2023

I would like to fix as a bug, but the impact may be big.
So I sent PR to 4.5 branch. See #7981

@MGatner
Copy link
Member

MGatner commented Sep 27, 2023

Followed up on the PR.

@neznaika0
Copy link
Contributor

A small question on the topic. Is it normal that the "filter" option is applied before and after the request?

$routes->group('', ['filter' => 'auth'], static function ($routes) {
    $routes->get('/expenses', 'Expense::list', ['as' => 'expenses.list']);
});
+--------+--------------------+--------------------+-------------------------------------+----------------+---------------+
| Method | Route              | Name               | Handler                             | Before Filters | After Filters |
+--------+--------------------+--------------------+-------------------------------------+----------------+---------------+
| GET    | expenses           | »                  | \App\Controllers\Expense::list      | auth csrf      | auth          |

@kenjis
Copy link
Member

kenjis commented Sep 30, 2023

Yes, it is expected, but probably the auth filter does nothing in after().

@neznaika0
Copy link
Contributor

Good. What will happen if the filter changes something or does more complicated work?

@kenjis
Copy link
Member

kenjis commented Oct 12, 2023

I sent another PR #8033

@kenjis
Copy link
Member

kenjis commented Oct 13, 2023

@kenjis
Copy link
Member

kenjis commented Oct 16, 2023

Closed by #8033

@kenjis kenjis closed this as completed Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

No branches or pull requests

5 participants