fix: route options are not merged (inner filter overrides outer filter) #7981

kenjis · 2023-09-26T06:54:39Z

Description
Fixes #7963

merge outer group() options with inner group() options
outer group filter option is not merged with inner group filter option. See https://github.com/codeigniter4/CodeIgniter4/pull/7981/files#diff-9e755caf334ca07345b43d4fdc3986ac1f9bb9594c6f78ebdec59bc780699b73R375-R400

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

michalsn

I'm trying to imagine scenarios in which this could have been intentionally used, and frankly, I can't. But I'm sure there will be devs who take advantage of current behavior.

michalsn · 2023-09-26T07:52:00Z

tests/system/Router/RouteCollectionTest.php

+    public function testGroupNestedWithOuterAndInnerOption(): void
+    {
+        $routes = $this->getCollector();
+
+        $routes->group(
+            'admin',
+            ['filter' => ['csrf']],
+            static function ($routes) {
+                $routes->get('dashboard', static function () {
+                });
+
+                $routes->group(
+                    'profile',
+                    ['filter' => ['honeypot']],
+                    static function ($routes) {
+                        $routes->get('/', static function () {
+                        });
+                    }
+                );
+            }
+        );
+
+        $expected = [
+            'admin/dashboard' => [
+                'filter' => ['csrf'],
+            ],
+            'admin/profile' => [
+                'filter' => ['honeypot'],
+            ],
+        ];


This is still confusing to me but I guess it's because we can have only one filter when Config\Feature::multipleFilters is disabled.

Will this gonna end up as:

$expected = [ 'admin/dashboard' => [ 'filter' => ['csrf'], ], 'admin/profile' => [ 'filter' => ['csrf', 'honeypot'], ], ];

when multipleFilters option will be enabled?

No. it is just because using array_merge().
https://3v4l.org/tskmm

It is the current behavior that filters are not merged, and also the reason why the Note in the user guide was added.

One possible behavior would be to merge filters as well, but then it would not be possible to delete filters in inner group. Multiple filters are already the default in 4.5.

Good point about removing filters.

I don't know... now I'm leaning towards staying with the current behavior and not inheriting anything. Because the new behavior seems even more confusing - it's inherited only if there are no other filters. I can imagine people adding a new filter over time and "losing" functionality.

Thanks to the current note, at least we have clear rules to follow.

The current behavior is not that straightforward either.
The current behavior is that if you don't pass an options array to the inner group, the outer options are inherited.

That's why when you set outer filter and add namespace option to inner group, the filter that set in outer suddenly will be gone for the inner route.

It seems that this issue is not so easy to conclude.

Yes, this is a bit more complex than I thought at first. Especially if we add namespaces to it.

Personally, I would probably merge all the declared filters from the external to the internal group using array_merge_recursive and introduce something like filterExclude to remove the filters if needed. However, this seems a bit too complex... not sure if we want to make such big changes.

Although the rules would be clear and need no further explanation.

I am very sorry for my translation.

Yes, to exclude the outside filter, I gave an example of except in the Config. Or we need to write such a filter for all routes except unnecessary ones (this is your option)

I disagree with the new feature to exclude route filters in the Config\Filter.
The Config\Filter and route filters are the same filters, but I think mixing these settings is a source of confusion.

Also, I don't like to add a new key for excluding filters in the route option.
It also is complicated.

Well, this does not cancel your method - we leave the outside filters empty and configure each route individually

I'm sorry, but what you don't like about the exception in the Config is a function of the framework. There is no other way to fine tune

Config\Filter defines filters for URI paths, not for routes.
except excepts the filter to the URI path in Config\Filter.
https://codeigniter4.github.io/CodeIgniter4/incoming/filters.html#except-for-a-few-uris

Filters for a route are different things. If you specify a filter to a route, it is always applied.
The definitions in Config\Filter does nothing to route filters.

What I don't like is to mix the two kind of filters. That is, except in Config\Filter excepts route filters, too.
It is really complicated and confusing.

@michalsn @neznaika0 @MGatner I changed my mind. I sent another PR #8033 that behaves like michalsn expects in #7981 (comment)

github-actions · 2023-09-28T20:38:53Z

👋 Hi, @kenjis!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

github-actions · 2023-10-01T23:57:34Z

👋 Hi, @kenjis!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

kenjis · 2023-10-03T05:43:26Z

Rebased to resolve a conflict.

github-actions · 2023-10-05T00:42:26Z

👋 Hi, @kenjis!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

kenjis · 2023-10-05T00:46:21Z

Rebased to resolve a conflict.

kenjis · 2023-10-14T21:59:35Z

Closed by #8033

kenjis added bug Verified issues on the current code behavior or pull requests that will fix them breaking change Pull requests that may break existing functionalities 4.5 labels Sep 26, 2023

kenjis mentioned this pull request Sep 26, 2023

Bug: route group filter bug #7963

Closed

michalsn reviewed Sep 26, 2023

View reviewed changes

github-actions bot added the stale Pull requests with conflicts label Sep 28, 2023

kenjis mentioned this pull request Sep 28, 2023

chore: Separate workflow to add/remove 'stale' label #7959

Merged

1 task

kenjis force-pushed the fix-route-option-merge branch from b2d49e9 to f4013c4 Compare September 29, 2023 05:22

kenjis mentioned this pull request Sep 29, 2023

Dev: Check Conflicts Remove label does not work #7992

Open

kenjis removed the stale Pull requests with conflicts label Sep 29, 2023

github-actions bot added the stale Pull requests with conflicts label Oct 1, 2023

kenjis force-pushed the fix-route-option-merge branch from f4013c4 to 9cad762 Compare October 3, 2023 05:43

kenjis force-pushed the fix-route-option-merge branch from 9cad762 to 0fd10e7 Compare October 3, 2023 06:15

kenjis removed the stale Pull requests with conflicts label Oct 3, 2023

github-actions bot added the stale Pull requests with conflicts label Oct 5, 2023

kenjis force-pushed the fix-route-option-merge branch from 0fd10e7 to c8cb5cc Compare October 5, 2023 00:46

kenjis removed the stale Pull requests with conflicts label Oct 5, 2023

MGatner approved these changes Oct 8, 2023

View reviewed changes

kenjis marked this pull request as draft October 12, 2023 00:14

kenjis added 4 commits October 12, 2023 09:16

test: add tests for nested group() and options

7174ef5

fix: outer options are not merged with inner options

029b796

docs: update docs

0fe4bfa

docs: add changelog and upgrade

8d62a57

kenjis force-pushed the fix-route-option-merge branch from c8cb5cc to 8d62a57 Compare October 12, 2023 00:16

kenjis mentioned this pull request Oct 12, 2023

fix: route options are not merged (outer filter is merged with inner filter) #8033

Merged

5 tasks

kenjis changed the title ~~fix: route options are not merged~~ fix: route options are not merged (inner filter overrides the outer filter) Oct 12, 2023

kenjis changed the title ~~fix: route options are not merged (inner filter overrides the outer filter)~~ fix: route options are not merged (inner filter overrides outer filter) Oct 12, 2023

kenjis merged commit 8d62a57 into codeigniter4:4.5 Oct 14, 2023
54 checks passed

kenjis deleted the fix-route-option-merge branch October 14, 2023 21:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: route options are not merged (inner filter overrides outer filter) #7981

fix: route options are not merged (inner filter overrides outer filter) #7981

kenjis commented Sep 26, 2023 •

edited

Loading

michalsn left a comment

michalsn Sep 26, 2023

kenjis Sep 26, 2023

michalsn Sep 26, 2023

kenjis Sep 26, 2023

michalsn Sep 26, 2023

neznaika0 Oct 9, 2023

kenjis Oct 9, 2023

neznaika0 Oct 9, 2023 •

edited

Loading

kenjis Oct 9, 2023

kenjis Oct 12, 2023

github-actions bot commented Sep 28, 2023

github-actions bot commented Oct 1, 2023

kenjis commented Oct 3, 2023

github-actions bot commented Oct 5, 2023

kenjis commented Oct 5, 2023

kenjis commented Oct 14, 2023 •

edited

Loading

fix: route options are not merged (inner filter overrides outer filter) #7981

fix: route options are not merged (inner filter overrides outer filter) #7981

Conversation

kenjis commented Sep 26, 2023 • edited Loading

michalsn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neznaika0 Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 28, 2023

github-actions bot commented Oct 1, 2023

kenjis commented Oct 3, 2023

github-actions bot commented Oct 5, 2023

kenjis commented Oct 5, 2023

kenjis commented Oct 14, 2023 • edited Loading

kenjis commented Sep 26, 2023 •

edited

Loading

neznaika0 Oct 9, 2023 •

edited

Loading

kenjis commented Oct 14, 2023 •

edited

Loading