codeigniter4 / CodeIgniter4

Open Source PHP Framework (originally from EllisLab)
https://codeigniter.com/
MIT License
5.4k stars 1.9k forks source link

Bug: route group filter bug #7963

Closed sajibAdhi closed 1 year ago

sajibAdhi commented 1 year ago

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

kenjis commented 1 year ago

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

sajibAdhi commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@codeigniter4/core-team Why the options array passed to the outer group() is not merged with the inner group() options array? https://github.com/codeigniter4/CodeIgniter4/blob/c4bd34e48ba20be97c92357f3e0d27552aa8cb96/system/Router/RouteCollection.php#L774

michalsn commented 1 year ago

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

kenjis commented 1 year ago

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 commented 1 year ago

Followed up on the PR.

neznaika0 commented 1 year ago

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 commented 1 year ago

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

neznaika0 commented 1 year ago

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

kenjis commented 1 year ago

I sent another PR #8033

kenjis commented 1 year ago

Hi all, I created two PRs. Please approve whichever you think is better.

kenjis commented 1 year ago

Closed by #8033