From c89ad6dd56e31ca1f901643ad199cdcbbd103d72 Mon Sep 17 00:00:00 2001 From: Ingolf Steinhardt Date: Tue, 4 Aug 2026 19:54:01 +0200 Subject: [PATCH 1/3] Fix URL parameter type for filter rules without frontend filter widget The URL parameter type ("URL type for the parameter") introduced with #1558 and fixed up in #1563 was only honoured for filter rules that render a frontend filter widget. ListControllerTrait::getFilterParameters() obtained the type from getParameterFilterWidgets(), which returns nothing for rules without widget - the usual detail page rules. Those parameters fell back to "slugNget" and were accepted as slug as well as GET, no matter what was configured. The type is now obtained from the filter settings themselves: * Simple::getParameterTypes() reports the configured param_type for all parameters of a setting, WithChildren and ExpressionRule merge the types of their children and Collection::getParameterTypes() aggregates all settings of the collection. * ParameterTypes::fromSetting() provides the backwards compatibility layer for filter settings not implementing getParameterTypes(). They are treated as "slugNget" and trigger a deprecation. The method becomes part of ISimple in MetaModels 3.0 - adding it now would break implementations not extending Simple. Render\Setting\Collection::buildJumpToUrlFor() builds the jumpTo URL of the detail page as slug or as GET according to the configured type - it always used slug before, so a rule configured as GET produced links that did not match its own configuration. A parameter passed via another type than the configured one now results in a 404 instead of silently rendering the unfiltered list under an URL that looks like it is filtered. This is limited to rules without frontend filter widget; for widgets the frontend filter handles the URL (see #1563) and a value of the wrong type stays unused as before. As a side effect the expensive getParameterFilterWidgets() call is gone from the regular rendering path. It is only performed when a mismatch was detected, that is on the path ending in a 404 anyway. --- .../Controller/ListControllerTrait.php | 53 +++++-- src/Filter/Setting/Collection.php | 14 ++ src/Filter/Setting/CustomSql.php | 15 ++ src/Filter/Setting/ExpressionRule.php | 15 ++ src/Filter/Setting/ICollection.php | 11 ++ src/Filter/Setting/ISimple.php | 10 ++ src/Filter/Setting/ParameterTypes.php | 74 ++++++++++ src/Filter/Setting/Simple.php | 14 ++ src/Filter/Setting/WithChildren.php | 13 ++ src/Render/Setting/Collection.php | 15 +- .../Fixtures/ListControllerTraitDouble.php | 32 +++++ .../Controller/ListControllerTraitTest.php | 115 +++++++++++++++ tests/Filter/Setting/CollectionTest.php | 79 +++++++++++ .../Setting/SimpleParameterTypesTest.php | 133 ++++++++++++++++++ 14 files changed, 581 insertions(+), 12 deletions(-) create mode 100644 src/Filter/Setting/ParameterTypes.php create mode 100644 tests/CoreBundle/Controller/Fixtures/ListControllerTraitDouble.php create mode 100644 tests/CoreBundle/Controller/ListControllerTraitTest.php create mode 100644 tests/Filter/Setting/SimpleParameterTypesTest.php diff --git a/src/CoreBundle/Controller/ListControllerTrait.php b/src/CoreBundle/Controller/ListControllerTrait.php index db9652f77..2243f752b 100644 --- a/src/CoreBundle/Controller/ListControllerTrait.php +++ b/src/CoreBundle/Controller/ListControllerTrait.php @@ -24,6 +24,7 @@ use Contao\BackendTemplate; use Contao\CoreBundle\Csrf\ContaoCsrfTokenManager; +use Contao\CoreBundle\Exception\PageNotFoundException; use Contao\CoreBundle\Routing\ScopeMatcher; use Contao\Input; use Contao\Model; @@ -339,22 +340,36 @@ private function getResponseInternal(Template $template, Model $model, Request $ * @param ItemList $itemRenderer The list renderer instance to be used. * * @return string[] + * + * @throws PageNotFoundException When a parameter is passed via another URL type than the configured one. */ private function getFilterParameters(FilterUrl $filterUrl, ItemList $itemRenderer): array { $filterSetting = $itemRenderer->getFilterSettings(); + // Obtain the types from the filter settings themselves - filter rules without frontend filter widget + // (i.e. the usual detail page rules) do not render a widget but still define a parameter type. /** @var array $wantedByType */ - $wantedByType = []; - // FIXME: improve this call - it does too much. - foreach ( - $filterSetting->getParameterFilterWidgets([], [], new FrontendFilterOptions()) as $widgetName => $widget - ) { - $wantedByType[$widgetName] = (string) ($widget['param_type'] ?? 'slugNget'); - } + $wantedByType = $filterSetting->getParameterTypes(); $result = []; + /** @var list|null $widgetParameters */ + $widgetParameters = null; foreach ($filterSetting->getParameters() as $name) { - if (null !== $value = $this->tryReadFromSlugOrGet($filterUrl, $name, $wantedByType[$name] ?? 'slugNget')) { + $paramType = $wantedByType[$name] ?? 'slugNget'; + if ($this->isParameterTypeMismatch($filterUrl, $name, $paramType)) { + // Only filter rules without frontend filter widget are guarded - for widgets the frontend filter + // handles the URL and a value of the wrong type simply stays unused. + // FIXME: improve this call - it does too much (only performed when a mismatch was detected). + $widgetParameters ??= \array_keys( + $filterSetting->getParameterFilterWidgets([], [], new FrontendFilterOptions()) + ); + if (!\in_array($name, $widgetParameters, true)) { + throw new PageNotFoundException( + \sprintf('Filter parameter "%s" must get passed as "%s".', $name, $paramType) + ); + } + } + if (null !== $value = $this->tryReadFromSlugOrGet($filterUrl, $name, $paramType)) { $result[$name] = $value; } } @@ -362,6 +377,28 @@ private function getFilterParameters(FilterUrl $filterUrl, ItemList $itemRendere return $result; } + /** + * Determine if a filter parameter is passed via another URL type than the configured one. + * + * Values passed via the "wrong" type are not read at all - without the resulting 404 the page would silently + * render the unfiltered list under an URL that looks like it is filtered (see #1563). + * + * @param FilterUrl $filterUrl The filter URL to check. + * @param string $name The parameter name to check. + * @param string $paramType The configured URL type of the parameter. + * + * @return bool + */ + private function isParameterTypeMismatch(FilterUrl $filterUrl, string $name, string $paramType): bool + { + // The deprecated "slugNget" accepts both variants. + return match ($paramType) { + 'get' => null !== $filterUrl->getSlug($name), + 'slug' => null !== $filterUrl->getGet($name), + default => false, + }; + } + /** * Get parameter from get or slug. * diff --git a/src/Filter/Setting/Collection.php b/src/Filter/Setting/Collection.php index 52728e4ae..e922f6a88 100644 --- a/src/Filter/Setting/Collection.php +++ b/src/Filter/Setting/Collection.php @@ -167,6 +167,20 @@ public function getParameters() return [] === $parameters ? [] : \array_merge(...$parameters); } + /** + * {@inheritdoc} + */ + #[\Override] + public function getParameterTypes() + { + $types = []; + foreach ($this->arrSettings as $objSetting) { + $types[] = ParameterTypes::fromSetting($objSetting); + } + + return [] === $types ? [] : \array_merge(...$types); + } + /** * {@inheritdoc} */ diff --git a/src/Filter/Setting/CustomSql.php b/src/Filter/Setting/CustomSql.php index 49af48caf..86ef540d4 100644 --- a/src/Filter/Setting/CustomSql.php +++ b/src/Filter/Setting/CustomSql.php @@ -50,6 +50,7 @@ use function array_intersect_key; use function array_key_exists; use function array_keys; +use function array_fill_keys; use function array_map; use function array_merge; use function array_reduce; @@ -234,6 +235,20 @@ public function getParameters() return $arrParams; } + /** + * Retrieve the URL parameter type for all registered parameters from the setting. + * + * @return array The parameter types as array. parametername => type + */ + public function getParameterTypes() + { + // Legacy settings without a value keep the lenient behaviour of accepting both variants. + return array_fill_keys( + $this->getParameters(), + (string) ($this->get('param_type') ?: ParameterTypes::LEGACY_TYPE) + ); + } + /** * {@inheritdoc} */ diff --git a/src/Filter/Setting/ExpressionRule.php b/src/Filter/Setting/ExpressionRule.php index 5e35b2300..fad952151 100644 --- a/src/Filter/Setting/ExpressionRule.php +++ b/src/Filter/Setting/ExpressionRule.php @@ -125,6 +125,21 @@ public function getParameters(): array return array_merge(...$parameters); } + /** + * Retrieve the URL parameter type for all registered parameters from the setting. + * + * @return array The parameter types as array. parametername => type + */ + public function getParameterTypes(): array + { + $types = []; + foreach ($this->children as $child) { + $types[] = ParameterTypes::fromSetting($child); + } + + return array_merge([], ...$types); + } + #[Override] public function getParameterDCA(): array { diff --git a/src/Filter/Setting/ICollection.php b/src/Filter/Setting/ICollection.php index c5adcc915..531b0bb03 100644 --- a/src/Filter/Setting/ICollection.php +++ b/src/Filter/Setting/ICollection.php @@ -82,6 +82,17 @@ public function generateFilterUrlFrom(IItem $objItem, IRenderSettings $objRender */ public function getParameters(); + /** + * Retrieve the URL parameter type for all registered parameters from the settings. + * + * The type determines from where the value of a parameter may get read and how the URL for it has to be built. + * Valid types are "slug" (key/value in the URL path), "get" (key=value in the query string) and the deprecated + * "slugNget" (both of them). + * + * @return array The parameter types as array. parametername => type + */ + public function getParameterTypes(); + /** * Retrieve the names of all parameters for listing in frontend filter configuration. * diff --git a/src/Filter/Setting/ISimple.php b/src/Filter/Setting/ISimple.php index 3d5e69665..86517c08b 100644 --- a/src/Filter/Setting/ISimple.php +++ b/src/Filter/Setting/ISimple.php @@ -30,6 +30,16 @@ /** * This interface handles the abstraction for a single filter setting. + * + * Implementations should also provide the following method: + * public function getParameterTypes(): array + * It returns the URL parameter type for all registered parameters (parametername => type). The type determines from + * where the value of a parameter may get read and how the URL for it has to be built. Valid types are "slug" + * (key/value in the URL path), "get" (key=value in the query string) and the deprecated "slugNget" (both of them). + * See Simple::getParameterTypes() for the default implementation. + * + * The method will become part of this interface in MetaModels 3.0 - until then, implementations not providing it + * are treated as "slugNget" (the lenient legacy behaviour). */ interface ISimple { diff --git a/src/Filter/Setting/ParameterTypes.php b/src/Filter/Setting/ParameterTypes.php new file mode 100644 index 000000000..c7f8f6377 --- /dev/null +++ b/src/Filter/Setting/ParameterTypes.php @@ -0,0 +1,74 @@ + + * @copyright 2012-2026 The MetaModels team. + * @license https://github.com/MetaModels/core/blob/master/LICENSE LGPL-3.0-or-later + * @filesource + */ + +declare(strict_types=1); + +namespace MetaModels\Filter\Setting; + +use function array_fill_keys; +use function method_exists; + +/** + * Helper to obtain the URL parameter types from a filter setting. + * + * This provides the backwards compatibility layer for filter settings not (yet) implementing + * "getParameterTypes()" - the method will become part of ISimple in MetaModels 3.0. + * + * @internal + */ +final class ParameterTypes +{ + /** + * The lenient legacy type, accepting both slug and GET. + */ + public const LEGACY_TYPE = 'slugNget'; + + /** + * Obtain the URL parameter types of the passed filter setting. + * + * @param ISimple $setting The filter setting to obtain the types from. + * + * @return array The parameter types as array. parametername => type + */ + public static function fromSetting(ISimple $setting): array + { + if (!method_exists($setting, 'getParameterTypes')) { + // Settings without any parameter can not be affected - stay silent for them. + if ([] === ($parameters = $setting->getParameters())) { + return []; + } + + // @codingStandardsIgnoreStart + @trigger_error( + 'Filter setting "' . $setting::class . '" does not implement "getParameterTypes()". ' . + 'The parameters are treated as "' . self::LEGACY_TYPE . '". ' . + 'The method will be required in MetaModels 3.0.', + E_USER_DEPRECATED + ); + // @codingStandardsIgnoreEnd + + return array_fill_keys($parameters, self::LEGACY_TYPE); + } + + /** @var array $types */ + $types = $setting->getParameterTypes(); + + return $types; + } +} diff --git a/src/Filter/Setting/Simple.php b/src/Filter/Setting/Simple.php index 1770cc4ca..2ba5a4141 100644 --- a/src/Filter/Setting/Simple.php +++ b/src/Filter/Setting/Simple.php @@ -571,6 +571,20 @@ public function getParameters() return []; } + /** + * Retrieve the URL parameter type for all registered parameters from the setting. + * + * @return array The parameter types as array. parametername => type + */ + public function getParameterTypes() + { + // Legacy settings without a value keep the lenient behaviour of accepting both variants. + return \array_fill_keys( + $this->getParameters(), + (string) ($this->get('param_type') ?: ParameterTypes::LEGACY_TYPE) + ); + } + /** * {@inheritdoc} */ diff --git a/src/Filter/Setting/WithChildren.php b/src/Filter/Setting/WithChildren.php index f419d438e..4691dcc08 100644 --- a/src/Filter/Setting/WithChildren.php +++ b/src/Filter/Setting/WithChildren.php @@ -76,6 +76,19 @@ public function getParameters() return $arrParams; } + /** + * {@inheritdoc} + */ + #[\Override] + public function getParameterTypes() + { + $arrTypes = []; + foreach ($this->arrChildren as $objSetting) { + $arrTypes = array_merge($arrTypes, ParameterTypes::fromSetting($objSetting)); + } + return $arrTypes; + } + /** * {@inheritdoc} */ diff --git a/src/Render/Setting/Collection.php b/src/Render/Setting/Collection.php index 9f8370533..a9f239a4a 100644 --- a/src/Render/Setting/Collection.php +++ b/src/Render/Setting/Collection.php @@ -339,18 +339,25 @@ public function buildJumpToUrlFor(IItem $item /**, ?int $referenceType */) if (!empty($information['filterSetting'])) { /** @var \MetaModels\Filter\Setting\ICollection $filterSetting */ - $filterSetting = $information['filterSetting']; - $parameterList = $filterSetting->generateFilterUrlFrom($item, $this); + $filterSetting = $information['filterSetting']; + $parameterList = $filterSetting->generateFilterUrlFrom($item, $this); + $parameterTypes = $filterSetting->getParameterTypes(); foreach ($parameterList as $strKey => $strValue) { // Sadly the filter values are currently encoded due to legacy reasons. // For MetaModels 3, they should be passed around decoded everywhere. - $filterUrl->setSlug($strKey, \rawurldecode($strValue))->setGet($strKey, ''); + $strValue = \rawurldecode($strValue); + // Build the URL as configured in the filter setting - "slugNget" and anything else use the slug. + if ('get' === ($parameterTypes[$strKey] ?? 'slug')) { + $filterUrl->setGet($strKey, $strValue)->setSlug($strKey, ''); + continue; + } + $filterUrl->setSlug($strKey, $strValue)->setGet($strKey, ''); } } $result['params'] = $parameterList; - $result['deep'] = !empty($filterUrl->getSlugParameters()); + $result['deep'] = !empty($filterUrl->getSlugParameters()) || !empty($filterUrl->getGetParameters()); $result['url'] = $this->filterUrlBuilder->generate( $filterUrl, diff --git a/tests/CoreBundle/Controller/Fixtures/ListControllerTraitDouble.php b/tests/CoreBundle/Controller/Fixtures/ListControllerTraitDouble.php new file mode 100644 index 000000000..55c5878b2 --- /dev/null +++ b/tests/CoreBundle/Controller/Fixtures/ListControllerTraitDouble.php @@ -0,0 +1,32 @@ + + * @copyright 2012-2026 The MetaModels team. + * @license https://github.com/MetaModels/core/blob/master/LICENSE LGPL-3.0-or-later + * @filesource + */ + +declare(strict_types=1); + +namespace MetaModels\Test\CoreBundle\Controller\Fixtures; + +use MetaModels\CoreBundle\Controller\ListControllerTrait; + +/** + * Test double to access the methods of the list controller trait. + */ +final class ListControllerTraitDouble +{ + use ListControllerTrait; +} diff --git a/tests/CoreBundle/Controller/ListControllerTraitTest.php b/tests/CoreBundle/Controller/ListControllerTraitTest.php new file mode 100644 index 000000000..f6f666a60 --- /dev/null +++ b/tests/CoreBundle/Controller/ListControllerTraitTest.php @@ -0,0 +1,115 @@ + + * @copyright 2012-2026 The MetaModels team. + * @license https://github.com/MetaModels/core/blob/master/LICENSE LGPL-3.0-or-later + * @filesource + */ + +declare(strict_types=1); + +namespace MetaModels\Test\CoreBundle\Controller; + +use MetaModels\CoreBundle\Controller\ListControllerTrait; +use MetaModels\Filter\FilterUrl; +use MetaModels\Test\CoreBundle\Controller\Fixtures\ListControllerTraitDouble; +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\Attributes\DataProvider; +use PHPUnit\Framework\TestCase; + +/** + * Test the URL type handling of the list controllers. + * + * @covers \MetaModels\CoreBundle\Controller\ListControllerTrait + */ +#[CoversClass(ListControllerTrait::class)] +class ListControllerTraitTest extends TestCase +{ + /** + * Data provider for parameters passed via the configured URL type. + * + * @return array, 2: array}> + */ + public static function providerMatchingType(): array + { + return [ + 'slug as slug' => ['slug', [], ['alias' => 'the-value']], + 'get as get' => ['get', ['alias' => 'the-value'], []], + 'slugNget as slug' => ['slugNget', [], ['alias' => 'the-value']], + 'slugNget as get' => ['slugNget', ['alias' => 'the-value'], []], + 'slugNget as both' => ['slugNget', ['alias' => 'the-value'], ['alias' => 'the-value']], + 'slug but not passed' => ['slug', [], []], + 'get but not passed' => ['get', [], []], + 'other parameter' => ['slug', ['other' => 'the-value'], []], + ]; + } + + /** + * Parameters passed via the configured URL type (or not at all) are no mismatch. + * + * @param string $paramType The configured URL type. + * @param array $getParameters The GET parameters of the URL. + * @param array $slugParameters The slug parameters of the URL. + */ + #[DataProvider('providerMatchingType')] + public function testMatchingTypeIsNoMismatch( + string $paramType, + array $getParameters, + array $slugParameters + ): void { + self::assertFalse( + $this->isParameterTypeMismatch(new FilterUrl([], $getParameters, $slugParameters), 'alias', $paramType) + ); + } + + /** + * A parameter configured as "slug" but passed as GET is a mismatch. + */ + public function testGetOnSlugParameterIsMismatch(): void + { + $filterUrl = new FilterUrl([], ['alias' => 'the-value'], []); + + self::assertTrue($this->isParameterTypeMismatch($filterUrl, 'alias', 'slug')); + } + + /** + * A parameter configured as "get" but passed as slug is a mismatch. + */ + public function testSlugOnGetParameterIsMismatch(): void + { + $filterUrl = new FilterUrl([], [], ['alias' => 'the-value']); + + self::assertTrue($this->isParameterTypeMismatch($filterUrl, 'alias', 'get')); + } + + /** + * Call the private isParameterTypeMismatch method on a class using the trait. + * + * @param FilterUrl $filterUrl The filter URL to check. + * @param string $name The parameter name to check. + * @param string $paramType The configured URL type of the parameter. + * + * @return bool + */ + private function isParameterTypeMismatch(FilterUrl $filterUrl, string $name, string $paramType): bool + { + $reflection = new \ReflectionClass(ListControllerTraitDouble::class); + // The trait constructor requires the whole service stack - not needed for this check. + $instance = $reflection->newInstanceWithoutConstructor(); + $method = $reflection->getMethod('isParameterTypeMismatch'); + $method->setAccessible(true); + + return $method->invoke($instance, $filterUrl, $name, $paramType); + } +} diff --git a/tests/Filter/Setting/CollectionTest.php b/tests/Filter/Setting/CollectionTest.php index a76afe9a4..b3f3e3cd2 100644 --- a/tests/Filter/Setting/CollectionTest.php +++ b/tests/Filter/Setting/CollectionTest.php @@ -20,6 +20,8 @@ namespace MetaModels\Test\Filter\Setting; use MetaModels\Filter\Setting\Collection; +use MetaModels\Filter\Setting\ISimple; +use MetaModels\Filter\Setting\Simple; use MetaModels\FrontendIntegration\FrontendFilterOptions; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\TestCase; @@ -52,4 +54,81 @@ public function testGetParametersReturnsEmptyArrayWhenNoSettings(): void self::assertSame([], $collection->getParameters()); } + + /** + * getParameterTypes() returns an empty array when the collection has no settings. + */ + public function testGetParameterTypesReturnsEmptyArrayWhenNoSettings(): void + { + $collection = new Collection([]); + + self::assertSame([], $collection->getParameterTypes()); + } + + /** + * getParameterTypes() collects the types of all contained settings - also for settings not rendering a + * frontend filter widget (i.e. the usual detail page filter rules). + */ + public function testGetParameterTypesCollectsTypesFromAllSettings(): void + { + $collection = new Collection([]); + $collection->addSetting($this->mockSetting(['alias' => 'get'])); + $collection->addSetting($this->mockSetting(['category' => 'slug', 'legacy' => 'slugNget'])); + + self::assertSame( + ['alias' => 'get', 'category' => 'slug', 'legacy' => 'slugNget'], + $collection->getParameterTypes() + ); + } + + /** + * Settings not implementing getParameterTypes() (BC layer) are treated as "slugNget". + */ + public function testGetParameterTypesFallsBackToSlugNgetForLegacySettings(): void + { + $legacySetting = $this->getMockForAbstractClass(ISimple::class); + $legacySetting->method('getParameters')->willReturn(['legacy_param']); + + $collection = new Collection([]); + $collection->addSetting($legacySetting); + + $previous = set_error_handler( + static function (int $severity, string $message) use (&$deprecation): bool { + unset($severity); + $deprecation = $message; + + return true; + }, + E_USER_DEPRECATED + ); + + try { + $types = $collection->getParameterTypes(); + } finally { + set_error_handler($previous); + } + + self::assertSame(['legacy_param' => 'slugNget'], $types); + self::assertStringContainsString('getParameterTypes()', (string) $deprecation); + } + + /** + * Mock a filter setting providing the passed parameter types. + * + * @param array $types The parameter types (parametername => type). + * + * @return ISimple + */ + private function mockSetting(array $types): ISimple + { + $setting = $this + ->getMockBuilder(Simple::class) + ->disableOriginalConstructor() + ->onlyMethods(['getParameters', 'getParameterTypes']) + ->getMockForAbstractClass(); + $setting->method('getParameters')->willReturn(array_keys($types)); + $setting->method('getParameterTypes')->willReturn($types); + + return $setting; + } } diff --git a/tests/Filter/Setting/SimpleParameterTypesTest.php b/tests/Filter/Setting/SimpleParameterTypesTest.php new file mode 100644 index 000000000..5b5e88587 --- /dev/null +++ b/tests/Filter/Setting/SimpleParameterTypesTest.php @@ -0,0 +1,133 @@ + + * @copyright 2012-2026 The MetaModels team. + * @license https://github.com/MetaModels/core/blob/master/LICENSE LGPL-3.0-or-later + * @filesource + */ + +declare(strict_types=1); + +namespace MetaModels\Test\Filter\Setting; + +use MetaModels\Filter\FilterUrlBuilder; +use MetaModels\Filter\Setting\ICollection; +use MetaModels\Filter\Setting\Simple; +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\Attributes\DataProvider; +use PHPUnit\Framework\MockObject\MockObject; +use PHPUnit\Framework\TestCase; +use Symfony\Component\EventDispatcher\EventDispatcherInterface; +use Symfony\Contracts\Translation\TranslatorInterface; + +/** + * Test the URL parameter types of simple filter settings. + * + * @covers \MetaModels\Filter\Setting\Simple + */ +#[CoversClass(Simple::class)] +class SimpleParameterTypesTest extends TestCase +{ + /** + * Data provider for the configurable URL types. + * + * @return array + */ + public static function providerParamType(): array + { + return [ + 'slug' => ['slug'], + 'get' => ['get'], + 'slugNget' => ['slugNget'], + ]; + } + + /** + * The configured param_type is reported for every parameter of the setting. + * + * This has to work for settings without frontend filter widget as well (the usual detail page filter rules), + * as the type is otherwise unknown and both slug and GET would be accepted. + * + * @param string $paramType The configured URL type. + */ + #[DataProvider('providerParamType')] + public function testReportsConfiguredType(string $paramType): void + { + $setting = $this->mockSimpleFilterSetting(['my_param'], ['param_type' => $paramType]); + + self::assertSame(['my_param' => $paramType], $setting->getParameterTypes()); + } + + /** + * Legacy settings without stored param_type keep the lenient behaviour of accepting slug and GET. + */ + public function testFallsBackToSlugNget(): void + { + $setting = $this->mockSimpleFilterSetting(['my_param'], []); + + self::assertSame(['my_param' => 'slugNget'], $setting->getParameterTypes()); + } + + /** + * All parameters of a setting share the configured type. + */ + public function testCoversAllParameters(): void + { + $setting = $this->mockSimpleFilterSetting(['from', 'to'], ['param_type' => 'get']); + + self::assertSame(['from' => 'get', 'to' => 'get'], $setting->getParameterTypes()); + } + + /** + * A setting without any parameter reports no types. + */ + public function testIsEmptyWithoutParameters(): void + { + $setting = $this->mockSimpleFilterSetting([], ['param_type' => 'get']); + + self::assertSame([], $setting->getParameterTypes()); + } + + /** + * Mock a Simple filter setting returning the passed parameter names. + * + * @param list $parameters The parameter names the setting shall report. + * @param array $properties The initialization data. + * + * @return Simple|MockObject + */ + private function mockSimpleFilterSetting(array $parameters, array $properties) + { + $filterUrlBuilder = $this->getMockBuilder(FilterUrlBuilder::class) + ->disableOriginalConstructor() + ->getMock(); + + $setting = $this + ->getMockBuilder(Simple::class) + ->setConstructorArgs( + [ + $this->getMockForAbstractClass(ICollection::class), + $properties, + $this->getMockForAbstractClass(EventDispatcherInterface::class), + $filterUrlBuilder, + $this->getMockForAbstractClass(TranslatorInterface::class) + ] + ) + ->onlyMethods(['getParameters']) + ->getMockForAbstractClass(); + $setting->method('getParameters')->willReturn($parameters); + + return $setting; + } +} From 9fb1af9d4eceff99e15f2ce1329cd80fc5d83511 Mon Sep 17 00:00:00 2001 From: Ingolf Steinhardt Date: Thu, 6 Aug 2026 10:37:09 +0200 Subject: [PATCH 2/3] Announce getParameterTypes() via @method and drop the 404 Adding a method to ICollection - like to any other interface - is a BC break and has to wait for the next major release. ICollection therefore keeps its previous method list and both, ICollection and ISimple, announce getParameterTypes() via a "@method" annotation plus a note that not implementing it is deprecated. The backwards compatibility layer ParameterTypes::fromSetting() now takes collections as well, so implementations not providing the method keep working and are treated as "slugNget". A filter parameter passed via another URL type than the configured one no longer results in a 404. The value is not read and the parameter stays unused, just like any other unknown parameter - the 404 was too harsh for a mistyped or outdated URL. As the outcome no longer differs between rules with and without frontend filter widget, the special casing of widgets and its additional getParameterFilterWidgets() call are gone again, together with the tests of the removed guard. --- .../Controller/ListControllerTrait.php | 56 ++------- src/Filter/Setting/Collection.php | 5 +- src/Filter/Setting/ICollection.php | 20 ++- src/Filter/Setting/ISimple.php | 17 +-- src/Filter/Setting/ParameterTypes.php | 13 +- src/Render/Setting/Collection.php | 3 +- .../Fixtures/ListControllerTraitDouble.php | 32 ----- .../Controller/ListControllerTraitTest.php | 115 ------------------ 8 files changed, 40 insertions(+), 221 deletions(-) delete mode 100644 tests/CoreBundle/Controller/Fixtures/ListControllerTraitDouble.php delete mode 100644 tests/CoreBundle/Controller/ListControllerTraitTest.php diff --git a/src/CoreBundle/Controller/ListControllerTrait.php b/src/CoreBundle/Controller/ListControllerTrait.php index 2243f752b..005975287 100644 --- a/src/CoreBundle/Controller/ListControllerTrait.php +++ b/src/CoreBundle/Controller/ListControllerTrait.php @@ -24,7 +24,6 @@ use Contao\BackendTemplate; use Contao\CoreBundle\Csrf\ContaoCsrfTokenManager; -use Contao\CoreBundle\Exception\PageNotFoundException; use Contao\CoreBundle\Routing\ScopeMatcher; use Contao\Input; use Contao\Model; @@ -34,7 +33,7 @@ use MetaModels\Filter\FilterUrl; use MetaModels\Filter\FilterUrlBuilder; use MetaModels\Filter\Setting\IFilterSettingFactory; -use MetaModels\FrontendIntegration\FrontendFilterOptions; +use MetaModels\Filter\Setting\ParameterTypes; use MetaModels\Helper\SortingLinkGenerator; use MetaModels\IFactory; use MetaModels\IItem; @@ -340,65 +339,30 @@ private function getResponseInternal(Template $template, Model $model, Request $ * @param ItemList $itemRenderer The list renderer instance to be used. * * @return string[] - * - * @throws PageNotFoundException When a parameter is passed via another URL type than the configured one. */ private function getFilterParameters(FilterUrl $filterUrl, ItemList $itemRenderer): array { $filterSetting = $itemRenderer->getFilterSettings(); // Obtain the types from the filter settings themselves - filter rules without frontend filter widget // (i.e. the usual detail page rules) do not render a widget but still define a parameter type. - /** @var array $wantedByType */ - $wantedByType = $filterSetting->getParameterTypes(); + $wantedByType = ParameterTypes::fromSetting($filterSetting); $result = []; - /** @var list|null $widgetParameters */ - $widgetParameters = null; foreach ($filterSetting->getParameters() as $name) { - $paramType = $wantedByType[$name] ?? 'slugNget'; - if ($this->isParameterTypeMismatch($filterUrl, $name, $paramType)) { - // Only filter rules without frontend filter widget are guarded - for widgets the frontend filter - // handles the URL and a value of the wrong type simply stays unused. - // FIXME: improve this call - it does too much (only performed when a mismatch was detected). - $widgetParameters ??= \array_keys( - $filterSetting->getParameterFilterWidgets([], [], new FrontendFilterOptions()) - ); - if (!\in_array($name, $widgetParameters, true)) { - throw new PageNotFoundException( - \sprintf('Filter parameter "%s" must get passed as "%s".', $name, $paramType) - ); - } - } - if (null !== $value = $this->tryReadFromSlugOrGet($filterUrl, $name, $paramType)) { - $result[$name] = $value; + $paramType = $wantedByType[$name] ?? ParameterTypes::LEGACY_TYPE; + $value = $this->tryReadFromSlugOrGet($filterUrl, $name, $paramType); + if (null === $value) { + // Either not passed at all or passed via another URL type than the configured one - in both cases + // the parameter simply stays unused. It has been marked as used in tryReadFromSlugOrGet() so a + // slug of the wrong type does not end up in a 404 for unused route arguments. + continue; } + $result[$name] = $value; } return $result; } - /** - * Determine if a filter parameter is passed via another URL type than the configured one. - * - * Values passed via the "wrong" type are not read at all - without the resulting 404 the page would silently - * render the unfiltered list under an URL that looks like it is filtered (see #1563). - * - * @param FilterUrl $filterUrl The filter URL to check. - * @param string $name The parameter name to check. - * @param string $paramType The configured URL type of the parameter. - * - * @return bool - */ - private function isParameterTypeMismatch(FilterUrl $filterUrl, string $name, string $paramType): bool - { - // The deprecated "slugNget" accepts both variants. - return match ($paramType) { - 'get' => null !== $filterUrl->getSlug($name), - 'slug' => null !== $filterUrl->getGet($name), - default => false, - }; - } - /** * Get parameter from get or slug. * diff --git a/src/Filter/Setting/Collection.php b/src/Filter/Setting/Collection.php index e922f6a88..9c1e9a032 100644 --- a/src/Filter/Setting/Collection.php +++ b/src/Filter/Setting/Collection.php @@ -168,9 +168,10 @@ public function getParameters() } /** - * {@inheritdoc} + * Retrieve the URL parameter type for all registered parameters of all contained settings. + * + * @return array The parameter types as array. parametername => type */ - #[\Override] public function getParameterTypes() { $types = []; diff --git a/src/Filter/Setting/ICollection.php b/src/Filter/Setting/ICollection.php index 531b0bb03..818b0295e 100644 --- a/src/Filter/Setting/ICollection.php +++ b/src/Filter/Setting/ICollection.php @@ -31,6 +31,15 @@ /** * This interface handles all filter setting abstraction. + * + * "getParameterTypes()" returns the URL parameter type for all registered parameters (parametername => type) of all + * contained filter settings, see ISimple for the possible types. + * + * Not implementing "getParameterTypes()" is deprecated, the method will get added to this interface in + * MetaModels 3.0. Until then, collections not providing it are treated as "slugNget" (the lenient legacy + * behaviour), see ParameterTypes::fromSetting(). + * + * @method array getParameterTypes() Retrieve the URL parameter type for all parameters. */ interface ICollection { @@ -82,17 +91,6 @@ public function generateFilterUrlFrom(IItem $objItem, IRenderSettings $objRender */ public function getParameters(); - /** - * Retrieve the URL parameter type for all registered parameters from the settings. - * - * The type determines from where the value of a parameter may get read and how the URL for it has to be built. - * Valid types are "slug" (key/value in the URL path), "get" (key=value in the query string) and the deprecated - * "slugNget" (both of them). - * - * @return array The parameter types as array. parametername => type - */ - public function getParameterTypes(); - /** * Retrieve the names of all parameters for listing in frontend filter configuration. * diff --git a/src/Filter/Setting/ISimple.php b/src/Filter/Setting/ISimple.php index 86517c08b..d915dbc20 100644 --- a/src/Filter/Setting/ISimple.php +++ b/src/Filter/Setting/ISimple.php @@ -31,15 +31,16 @@ /** * This interface handles the abstraction for a single filter setting. * - * Implementations should also provide the following method: - * public function getParameterTypes(): array - * It returns the URL parameter type for all registered parameters (parametername => type). The type determines from - * where the value of a parameter may get read and how the URL for it has to be built. Valid types are "slug" - * (key/value in the URL path), "get" (key=value in the query string) and the deprecated "slugNget" (both of them). - * See Simple::getParameterTypes() for the default implementation. + * "getParameterTypes()" returns the URL parameter type for all registered parameters (parametername => type). The + * type determines from where the value of a parameter may get read and how the URL for it has to be built. Valid + * types are "slug" (key/value in the URL path), "get" (key=value in the query string) and the deprecated "slugNget" + * (both of them). See Simple::getParameterTypes() for the default implementation. * - * The method will become part of this interface in MetaModels 3.0 - until then, implementations not providing it - * are treated as "slugNget" (the lenient legacy behaviour). + * Not implementing "getParameterTypes()" is deprecated, the method will get added to this interface in + * MetaModels 3.0. Until then, settings not providing it are treated as "slugNget" (the lenient legacy behaviour), + * see ParameterTypes::fromSetting(). + * + * @method array getParameterTypes() Retrieve the URL parameter type for all parameters. */ interface ISimple { diff --git a/src/Filter/Setting/ParameterTypes.php b/src/Filter/Setting/ParameterTypes.php index c7f8f6377..bdef0f6a0 100644 --- a/src/Filter/Setting/ParameterTypes.php +++ b/src/Filter/Setting/ParameterTypes.php @@ -25,10 +25,11 @@ use function method_exists; /** - * Helper to obtain the URL parameter types from a filter setting. + * Helper to obtain the URL parameter types from a filter setting or a filter setting collection. * - * This provides the backwards compatibility layer for filter settings not (yet) implementing - * "getParameterTypes()" - the method will become part of ISimple in MetaModels 3.0. + * This provides the backwards compatibility layer for implementations not (yet) providing + * "getParameterTypes()" - the method will get added to ISimple and ICollection in MetaModels 3.0. Adding it to the + * interfaces before would break every implementation out there, therefore it is only announced via "@method" there. * * @internal */ @@ -40,13 +41,13 @@ final class ParameterTypes public const LEGACY_TYPE = 'slugNget'; /** - * Obtain the URL parameter types of the passed filter setting. + * Obtain the URL parameter types of the passed filter setting or filter setting collection. * - * @param ISimple $setting The filter setting to obtain the types from. + * @param ICollection|ISimple $setting The filter setting to obtain the types from. * * @return array The parameter types as array. parametername => type */ - public static function fromSetting(ISimple $setting): array + public static function fromSetting(ICollection|ISimple $setting): array { if (!method_exists($setting, 'getParameterTypes')) { // Settings without any parameter can not be affected - stay silent for them. diff --git a/src/Render/Setting/Collection.php b/src/Render/Setting/Collection.php index a9f239a4a..a11fa3d17 100644 --- a/src/Render/Setting/Collection.php +++ b/src/Render/Setting/Collection.php @@ -31,6 +31,7 @@ use MetaModels\Filter\FilterUrl; use MetaModels\Filter\FilterUrlBuilder; use MetaModels\Filter\Setting\IFilterSettingFactory; +use MetaModels\Filter\Setting\ParameterTypes; use MetaModels\IItem; use MetaModels\IMetaModel; use MetaModels\ITranslatedMetaModel; @@ -341,7 +342,7 @@ public function buildJumpToUrlFor(IItem $item /**, ?int $referenceType */) /** @var \MetaModels\Filter\Setting\ICollection $filterSetting */ $filterSetting = $information['filterSetting']; $parameterList = $filterSetting->generateFilterUrlFrom($item, $this); - $parameterTypes = $filterSetting->getParameterTypes(); + $parameterTypes = ParameterTypes::fromSetting($filterSetting); foreach ($parameterList as $strKey => $strValue) { // Sadly the filter values are currently encoded due to legacy reasons. diff --git a/tests/CoreBundle/Controller/Fixtures/ListControllerTraitDouble.php b/tests/CoreBundle/Controller/Fixtures/ListControllerTraitDouble.php deleted file mode 100644 index 55c5878b2..000000000 --- a/tests/CoreBundle/Controller/Fixtures/ListControllerTraitDouble.php +++ /dev/null @@ -1,32 +0,0 @@ - - * @copyright 2012-2026 The MetaModels team. - * @license https://github.com/MetaModels/core/blob/master/LICENSE LGPL-3.0-or-later - * @filesource - */ - -declare(strict_types=1); - -namespace MetaModels\Test\CoreBundle\Controller\Fixtures; - -use MetaModels\CoreBundle\Controller\ListControllerTrait; - -/** - * Test double to access the methods of the list controller trait. - */ -final class ListControllerTraitDouble -{ - use ListControllerTrait; -} diff --git a/tests/CoreBundle/Controller/ListControllerTraitTest.php b/tests/CoreBundle/Controller/ListControllerTraitTest.php deleted file mode 100644 index f6f666a60..000000000 --- a/tests/CoreBundle/Controller/ListControllerTraitTest.php +++ /dev/null @@ -1,115 +0,0 @@ - - * @copyright 2012-2026 The MetaModels team. - * @license https://github.com/MetaModels/core/blob/master/LICENSE LGPL-3.0-or-later - * @filesource - */ - -declare(strict_types=1); - -namespace MetaModels\Test\CoreBundle\Controller; - -use MetaModels\CoreBundle\Controller\ListControllerTrait; -use MetaModels\Filter\FilterUrl; -use MetaModels\Test\CoreBundle\Controller\Fixtures\ListControllerTraitDouble; -use PHPUnit\Framework\Attributes\CoversClass; -use PHPUnit\Framework\Attributes\DataProvider; -use PHPUnit\Framework\TestCase; - -/** - * Test the URL type handling of the list controllers. - * - * @covers \MetaModels\CoreBundle\Controller\ListControllerTrait - */ -#[CoversClass(ListControllerTrait::class)] -class ListControllerTraitTest extends TestCase -{ - /** - * Data provider for parameters passed via the configured URL type. - * - * @return array, 2: array}> - */ - public static function providerMatchingType(): array - { - return [ - 'slug as slug' => ['slug', [], ['alias' => 'the-value']], - 'get as get' => ['get', ['alias' => 'the-value'], []], - 'slugNget as slug' => ['slugNget', [], ['alias' => 'the-value']], - 'slugNget as get' => ['slugNget', ['alias' => 'the-value'], []], - 'slugNget as both' => ['slugNget', ['alias' => 'the-value'], ['alias' => 'the-value']], - 'slug but not passed' => ['slug', [], []], - 'get but not passed' => ['get', [], []], - 'other parameter' => ['slug', ['other' => 'the-value'], []], - ]; - } - - /** - * Parameters passed via the configured URL type (or not at all) are no mismatch. - * - * @param string $paramType The configured URL type. - * @param array $getParameters The GET parameters of the URL. - * @param array $slugParameters The slug parameters of the URL. - */ - #[DataProvider('providerMatchingType')] - public function testMatchingTypeIsNoMismatch( - string $paramType, - array $getParameters, - array $slugParameters - ): void { - self::assertFalse( - $this->isParameterTypeMismatch(new FilterUrl([], $getParameters, $slugParameters), 'alias', $paramType) - ); - } - - /** - * A parameter configured as "slug" but passed as GET is a mismatch. - */ - public function testGetOnSlugParameterIsMismatch(): void - { - $filterUrl = new FilterUrl([], ['alias' => 'the-value'], []); - - self::assertTrue($this->isParameterTypeMismatch($filterUrl, 'alias', 'slug')); - } - - /** - * A parameter configured as "get" but passed as slug is a mismatch. - */ - public function testSlugOnGetParameterIsMismatch(): void - { - $filterUrl = new FilterUrl([], [], ['alias' => 'the-value']); - - self::assertTrue($this->isParameterTypeMismatch($filterUrl, 'alias', 'get')); - } - - /** - * Call the private isParameterTypeMismatch method on a class using the trait. - * - * @param FilterUrl $filterUrl The filter URL to check. - * @param string $name The parameter name to check. - * @param string $paramType The configured URL type of the parameter. - * - * @return bool - */ - private function isParameterTypeMismatch(FilterUrl $filterUrl, string $name, string $paramType): bool - { - $reflection = new \ReflectionClass(ListControllerTraitDouble::class); - // The trait constructor requires the whole service stack - not needed for this check. - $instance = $reflection->newInstanceWithoutConstructor(); - $method = $reflection->getMethod('isParameterTypeMismatch'); - $method->setAccessible(true); - - return $method->invoke($instance, $filterUrl, $name, $paramType); - } -} From d8cc677ee8bc698593c9e7bb1089d3934efffc64 Mon Sep 17 00:00:00 2001 From: Ingolf Steinhardt Date: Thu, 6 Aug 2026 10:43:37 +0200 Subject: [PATCH 3/3] Do not throw a 404 in the frontend filter either A get-only filter parameter accessed via slug resulted in a PageNotFoundException (#1563). This is too harsh for a mistyped or outdated URL and it is no longer in line with the list controllers, which ignore a value passed via the wrong URL type. The value was never used for filtering anyway: buildParameters() routes a slug that is not wanted as slug into the "other" parameters, so it never becomes part of the widget values. Only the exception is gone therefore. To not have the 404 come back in through the back door, all wanted parameter names are now marked as used in the Input class. Previously this was only done for the ones present as slug in the "all" parameters, so a get-only parameter passed via slug stayed unconsumed and Contao raised an UnusedArgumentsException - a 404 as well - whenever no list on the same page marked it. --- src/FrontendIntegration/FrontendFilter.php | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/src/FrontendIntegration/FrontendFilter.php b/src/FrontendIntegration/FrontendFilter.php index a0f296a0b..6fee8b21c 100644 --- a/src/FrontendIntegration/FrontendFilter.php +++ b/src/FrontendIntegration/FrontendFilter.php @@ -32,7 +32,6 @@ use ContaoCommunityAlliance\Contao\Bindings\ContaoEvents; use ContaoCommunityAlliance\Contao\Bindings\Events\Controller\RedirectEvent; use Contao\CoreBundle\Csrf\ContaoCsrfTokenManager; -use Contao\CoreBundle\Exception\PageNotFoundException; use Contao\CoreBundle\Exception\RedirectResponseException; use Contao\FrontendTemplate; use Contao\Input; @@ -435,10 +434,10 @@ protected function getFilters() ); // DAMN Contao - we have to "mark" the keys in the Input class as used as we get an 404 otherwise. + // This is also done for parameters passed via another URL type than the configured one. Their value is + // not used for filtering (see buildParameters()), but they must not end up in a 404 either. foreach ($wantedNames as $name) { - if ($all->hasSlug($name)) { - Input::get($name); - } + Input::get($name); } $values = \array_merge($all->getSlugParameters(), $all->getGetParameters()); @@ -450,13 +449,6 @@ protected function getFilters() $filterOptions ); - // 404 if a get-only filter parameter is accessed via slug. - foreach ($arrWidgets as $widgetName => $widget) { - if ('get' === ($widget['param_type'] ?? 'slug') && $all->hasSlug($widgetName)) { - throw new PageNotFoundException(); - } - } - // If we have POST data, we need to redirect now. if (Input::post('FORM_SUBMIT') === $this->formId) { foreach ($wantedNames as $widgetName) {