From 7174ef55628c7bfa8318ae6d30e858d2ced5afd5 Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 26 Sep 2023 15:28:51 +0900 Subject: [PATCH 1/4] test: add tests for nested group() and options --- tests/system/Router/RouteCollectionTest.php | 98 +++++++++++++++++++++ 1 file changed, 98 insertions(+) diff --git a/tests/system/Router/RouteCollectionTest.php b/tests/system/Router/RouteCollectionTest.php index 37844fed6cdb..4a62ce0b3e8d 100644 --- a/tests/system/Router/RouteCollectionTest.php +++ b/tests/system/Router/RouteCollectionTest.php @@ -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'], + ], + ]; + $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(); From 029b79622a3461e9a4067478eb07e26eb0a75dad Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 26 Sep 2023 15:29:26 +0900 Subject: [PATCH 2/4] fix: outer options are not merged with inner options --- system/Router/RouteCollection.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/system/Router/RouteCollection.php b/system/Router/RouteCollection.php index ca080152ff28..325111e519a4 100644 --- a/system/Router/RouteCollection.php +++ b/system/Router/RouteCollection.php @@ -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)) { From 0fe4bfa1958c6e2f850fc38637ae2069a076e10a Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 26 Sep 2023 15:51:57 +0900 Subject: [PATCH 3/4] docs: update docs --- user_guide_src/source/incoming/routing.rst | 10 +++++++++- user_guide_src/source/incoming/routing/026.php | 5 +++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/user_guide_src/source/incoming/routing.rst b/user_guide_src/source/incoming/routing.rst index f956c68fb4df..74c8983e83d0 100644 --- a/user_guide_src/source/incoming/routing.rst +++ b/user_guide_src/source/incoming/routing.rst @@ -544,7 +544,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: diff --git a/user_guide_src/source/incoming/routing/026.php b/user_guide_src/source/incoming/routing/026.php index 1ed2a8a96ee6..5098325026cd 100644 --- a/user_guide_src/source/incoming/routing/026.php +++ b/user_guide_src/source/incoming/routing/026.php @@ -1,7 +1,8 @@ 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'); }); }); From 8d62a57433de19504c670163a7a87a23c50b25ac Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 26 Sep 2023 16:23:37 +0900 Subject: [PATCH 4/4] docs: add changelog and upgrade --- user_guide_src/source/changelogs/v4.5.0.rst | 7 +++++ user_guide_src/source/incoming/routing.rst | 2 ++ .../source/installation/upgrade_450.rst | 31 +++++++++++++++++++ 3 files changed, 40 insertions(+) diff --git a/user_guide_src/source/changelogs/v4.5.0.rst b/user_guide_src/source/changelogs/v4.5.0.rst index f109180c8009..7c578c8531ec 100644 --- a/user_guide_src/source/changelogs/v4.5.0.rst +++ b/user_guide_src/source/changelogs/v4.5.0.rst @@ -27,6 +27,13 @@ Filter Execution Order The order in which Controller Filters are executed has changed. See :ref:`Upgrading Guide ` 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 ` for details. + Others ------ diff --git a/user_guide_src/source/incoming/routing.rst b/user_guide_src/source/incoming/routing.rst index 74c8983e83d0..8b6aa927fa90 100644 --- a/user_guide_src/source/incoming/routing.rst +++ b/user_guide_src/source/incoming/routing.rst @@ -535,6 +535,8 @@ given route config options: .. literalinclude:: routing/027.php +.. _routing-nesting-groups: + Nesting Groups ============== diff --git a/user_guide_src/source/installation/upgrade_450.rst b/user_guide_src/source/installation/upgrade_450.rst index 240203f4b888..ca0fc2aa817c 100644 --- a/user_guide_src/source/installation/upgrade_450.rst +++ b/user_guide_src/source/installation/upgrade_450.rst @@ -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 ========================