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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
Oct 14, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7174ef5
test: add tests for nested group() and options
kenjis Sep 26, 2023
029b796
fix: outer options are not merged with inner options
kenjis Sep 26, 2023
0fe4bfa
docs: update docs
kenjis Sep 26, 2023
8d62a57
docs: add changelog and upgrade
kenjis Sep 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion system/Router/RouteCollection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -771,7 +771,10 @@ public function group(string $name, ...$params)
$callback = array_pop($params);

if ($params && is_array($params[0])) {
$this->currentOptions = array_shift($params);
$this->currentOptions = array_merge(
$this->currentOptions ?? [],
array_shift($params)
);
}

if (is_callable($callback)) {
Expand Down
98 changes: 98 additions & 0 deletions tests/system/Router/RouteCollectionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,104 @@ static function ($routes): void {
$this->assertSame($expected, $routes->getRoutes());
}

public function testGroupNestedWithOuterOptionsWithoutInnerOptions(): void
{
$routes = $this->getCollector();

$routes->group(
'admin',
['namespace' => 'Admin', 'filter' => ['csrf']],
static function ($routes) {
$routes->get('dashboard', static function () {
});

$routes->group('profile', static function ($routes) {
$routes->get('/', static function () {
});
});
}
);

$expected = [
'admin/dashboard' => [
'namespace' => 'Admin',
'filter' => ['csrf'],
],
'admin/profile' => [
'namespace' => 'Admin',
'filter' => ['csrf'],
],
];
$this->assertSame($expected, $routes->getRoutesOptions());
}

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'],
],
];
Comment on lines +371 to +400
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@neznaika0 neznaika0 Oct 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

$this->assertSame($expected, $routes->getRoutesOptions());
}

public function testGroupNestedWithoutOuterOptionWithInnerOption(): void
{
$routes = $this->getCollector();

$routes->group(
'admin',
['filter' => 'csrf'],
static function ($routes) {
$routes->get('dashboard', static function () {
});

$routes->group(
'profile',
['namespace' => 'Admin'],
static function ($routes) {
$routes->get('/', static function () {
});
}
);
}
);

$expected = [
'admin/dashboard' => [
'filter' => 'csrf',
],
'admin/profile' => [
'filter' => 'csrf',
'namespace' => 'Admin',
],
];
$this->assertSame($expected, $routes->getRoutesOptions());
}

public function testGroupingWorksWithEmptyStringPrefix(): void
{
$routes = $this->getCollector();
Expand Down
7 changes: 7 additions & 0 deletions user_guide_src/source/changelogs/v4.5.0.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,13 @@ Filter Execution Order
The order in which Controller Filters are executed has changed. See
:ref:`Upgrading Guide <upgrade-450-filter-execution-order>` for details.

Nested Route Groups and Options
-------------------------------

Due to a bug fix, the behavior has changed so that options passed to the outer
``group()`` are merged with the options of the inner ``group()``.
See :ref:`Upgrading Guide <upgrade-450-nested-route-groups-and-options>` for details.

Others
------

Expand Down
12 changes: 11 additions & 1 deletion user_guide_src/source/incoming/routing.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,6 +535,8 @@ given route config options:

.. literalinclude:: routing/027.php

.. _routing-nesting-groups:

Nesting Groups
==============

Expand All @@ -544,7 +546,15 @@ It is possible to nest groups within groups for finer organization if you need i

This would handle the URL at **admin/users/list**.

.. note:: Options passed to the outer ``group()`` (for example ``namespace`` and ``filter``) are not merged with the inner ``group()`` options.
Options array passed to the outer ``group()`` are merged with the inner
``group()`` options array. But note that if you specify the same key in the
inner ``group()`` options, the value is overwritten.

The above code runs ``myfilter:config`` for ``admin``, and only ``myfilter:region``
for ``admin/users/list``.

.. note:: Prior to v4.5.0, due to a bug, options passed to the outer ``group()``
are not merged with the inner ``group()`` options.

.. _routing-priority:

Expand Down
5 changes: 3 additions & 2 deletions user_guide_src/source/incoming/routing/026.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
<?php

$routes->group('admin', static function ($routes) {
$routes->group('users', static function ($routes) {
$routes->group('admin', ['filter' => 'myfilter:config'], static function ($routes) {
$routes->get('/', 'Admin\Admin::index');
$routes->group('users', ['filter' => 'myfilter:region'], static function ($routes) {
$routes->get('list', 'Admin\Users::list');
});
});
31 changes: 31 additions & 0 deletions user_guide_src/source/installation/upgrade_450.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,37 @@ Mandatory File Changes
Breaking Changes
****************

.. _upgrade-450-nested-route-groups-and-options:

Nested Route Groups and Options
===============================

A bug that prevented options passed to outer ``group()`` from being merged with
options in inner ``group()`` has been fixed.

Check and correct your route configuration as it could change the values of the
options applied.

For example,

.. code-block:: php

$routes->group('admin', ['filter' => 'csrf'], static function ($routes) {
$routes->get('/', static function () {
// ...
});

$routes->group('users', ['namespace' => 'Users'], static function ($routes) {
$routes->get('/', static function () {
// ...
});
});
});

Now the ``csrf`` filter is executed for both the route ``admin`` and ``admin/users``.
In previous versions, it is executed only for the route ``admin``.
See also :ref:`routing-nesting-groups`.

Method Signature Changes
========================

Expand Down
Loading