From 7174ef55628c7bfa8318ae6d30e858d2ced5afd5 Mon Sep 17 00:00:00 2001 From: kenjis Date: Tue, 26 Sep 2023 15:28:51 +0900 Subject: [PATCH 1/7] 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/7] 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/7] 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/7] 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 ======================== From 1421218a9dc7ef29902443ee7417300efe825b63 Mon Sep 17 00:00:00 2001 From: kenjis Date: Thu, 12 Oct 2023 09:34:25 +0900 Subject: [PATCH 5/7] fix: merge outer filters with inner filters --- system/Router/RouteCollection.php | 11 ++++++++++- tests/system/Router/RouteCollectionTest.php | 6 +++--- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/system/Router/RouteCollection.php b/system/Router/RouteCollection.php index 325111e519a4..66d6f276c334 100644 --- a/system/Router/RouteCollection.php +++ b/system/Router/RouteCollection.php @@ -771,9 +771,18 @@ public function group(string $name, ...$params) $callback = array_pop($params); if ($params && is_array($params[0])) { + $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 ?? [], - array_shift($params) + $options ); } diff --git a/tests/system/Router/RouteCollectionTest.php b/tests/system/Router/RouteCollectionTest.php index 4a62ce0b3e8d..d6b7e0dfe6ac 100644 --- a/tests/system/Router/RouteCollectionTest.php +++ b/tests/system/Router/RouteCollectionTest.php @@ -395,7 +395,7 @@ static function ($routes) { 'filter' => ['csrf'], ], 'admin/profile' => [ - 'filter' => ['honeypot'], + 'filter' => ['csrf', 'honeypot'], ], ]; $this->assertSame($expected, $routes->getRoutesOptions()); @@ -425,10 +425,10 @@ static function ($routes) { $expected = [ 'admin/dashboard' => [ - 'filter' => 'csrf', + 'filter' => ['csrf'], ], 'admin/profile' => [ - 'filter' => 'csrf', + 'filter' => ['csrf'], 'namespace' => 'Admin', ], ]; From 2be0c3000c40d35f1e364d4f4de47b4862d17f80 Mon Sep 17 00:00:00 2001 From: kenjis Date: Thu, 12 Oct 2023 09:46:52 +0900 Subject: [PATCH 6/7] docs: update docs --- user_guide_src/source/incoming/routing.rst | 11 ++++++----- user_guide_src/source/incoming/routing/026.php | 1 + 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/user_guide_src/source/incoming/routing.rst b/user_guide_src/source/incoming/routing.rst index 8b6aa927fa90..1f64a6b1c329 100644 --- a/user_guide_src/source/incoming/routing.rst +++ b/user_guide_src/source/incoming/routing.rst @@ -546,12 +546,13 @@ It is possible to nest groups within groups for finer organization if you need i This would handle the URL at **admin/users/list**. -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. +**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``. -The above code runs ``myfilter:config`` for ``admin``, and only ``myfilter:region`` -for ``admin/users/list``. +But other options that you specify in the inner ``group()`` options, the value +is overwritten. .. note:: Prior to v4.5.0, due to a bug, options passed to the outer ``group()`` are not merged with the inner ``group()`` options. diff --git a/user_guide_src/source/incoming/routing/026.php b/user_guide_src/source/incoming/routing/026.php index 5098325026cd..b83e33fce0cb 100644 --- a/user_guide_src/source/incoming/routing/026.php +++ b/user_guide_src/source/incoming/routing/026.php @@ -2,6 +2,7 @@ $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 3477f15c75b8c71b7eca2732fde85dafc5360814 Mon Sep 17 00:00:00 2001 From: kenjis Date: Sat, 14 Oct 2023 08:04:28 +0900 Subject: [PATCH 7/7] docs: fix by proofreading Co-authored-by: MGatner --- user_guide_src/source/incoming/routing.rst | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/user_guide_src/source/incoming/routing.rst b/user_guide_src/source/incoming/routing.rst index 1f64a6b1c329..39b5d3db8cdd 100644 --- a/user_guide_src/source/incoming/routing.rst +++ b/user_guide_src/source/incoming/routing.rst @@ -551,8 +551,7 @@ This would handle the URL at **admin/users/list**. 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. +Any other overlapping options passed to the inner `group()` will overwrite their values. .. note:: Prior to v4.5.0, due to a bug, options passed to the outer ``group()`` are not merged with the inner ``group()`` options.