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

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

Merged
merged 7 commits into from
Oct 14, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
14 changes: 13 additions & 1 deletion system/Router/RouteCollection.php
Original file line number Diff line number Diff line change
Expand Up @@ -771,7 +771,19 @@ public function group(string $name, ...$params)
$callback = array_pop($params);

if ($params && is_array($params[0])) {
$this->currentOptions = array_shift($params);
$options = array_shift($params);

if (isset($options['filter'])) {
// Merge filters.
$currentFilter = (array) ($this->currentOptions['filter'] ?? []);
$options['filter'] = array_merge($currentFilter, (array) $options['filter']);
}

// Merge options other than filters.
$this->currentOptions = array_merge(
$this->currentOptions ?? [],
$options
);
}

if (is_callable($callback)) {
Expand Down
98 changes: 98 additions & 0 deletions tests/system/Router/RouteCollectionTest.php
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' => ['csrf', 'honeypot'],
],
];
$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
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
13 changes: 12 additions & 1 deletion user_guide_src/source/incoming/routing.rst
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,16 @@ 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.
**Filter** option passed to the outer ``group()`` are merged with the inner
``group()`` filter option.
The above code runs ``myfilter:config`` for the route ``admin``, and ``myfilter:config``
and ``myfilter:region`` for the route ``admin/users/list``.

But other options that you specify in the inner ``group()`` options, the value
is overwritten.
kenjis marked this conversation as resolved.
Show resolved Hide resolved

.. 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
6 changes: 4 additions & 2 deletions user_guide_src/source/incoming/routing/026.php
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
<?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
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