Skip to content

Commit

Permalink
[BUGFIX] Allow slashes in enhanced routes having aspects definitions
Browse files Browse the repository at this point in the history
The changes made in #86895 broke previous behaviour in page routing by
disallowing the use of the "requirements" option for a route when
aspects are present - "aspects now take precedence over requirements".

While the reasoning behind this change is valid, the requirements in
the Symfony Routing package would now always default to '[^/]++'
(every character except "/") for routes that use aspects.

Before, it was possible to manually set the requirements to '.+' to
allow slashes in an argument value.

Instead of purging requirements for variable names having an aspect
definition as well, thoese requirements are set to `.+` to relax the
default Symfony constraints on default delimiter `/` in this case.

Resolves: #90531
Resolves: #88291
Resolves: #87333
Related: #86895
Releases: master, 9.5
Change-Id: I27076aa0425d050e729db84d8e3461a329321342
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/63655
Reviewed-by: Oliver Hader <oliver.hader@typo3.org>
Reviewed-by: Benni Mack <benni@typo3.org>
Tested-by: TYPO3com <noreply@typo3.com>
Tested-by: Benni Mack <benni@typo3.org>
  • Loading branch information
IndyIndyIndy authored and bmack committed Mar 10, 2020
1 parent 306ddc7 commit e98ba4a
Show file tree
Hide file tree
Showing 3 changed files with 99 additions and 14 deletions.
34 changes: 22 additions & 12 deletions typo3/sysext/core/Classes/Routing/Enhancer/AbstractEnhancer.php
Expand Up @@ -60,11 +60,23 @@ protected function applyRequirements(Route $route, array $requirements, string $
if (empty($requirements)) {
return;
}

$requirements = $this->getVariableProcessor()
->deflateKeys($requirements, $namespace, $route->getArguments());

// only keep requirements that are actually part of the current route path
$requirements = $this->filterValuesByPathVariables($route, $requirements);
$requirements = $this->purgeValuesByAspects($route, $requirements);
// In general requirements are not considered when having aspects defined for a
// given variable name and thus aspects are more specific and take precedence:
// + since requirements in classic Symfony focus on internal arguments
// and aspects define a mapping between URL part (e.g. 'some-example-news')
// and the corresponding internal argument (e.g. 'tx_news_pi1[news]=123')
// + thus, the requirement definition cannot be used for resolving and generating
// a route at the same time (it would have to be e.g. `[\w_._]+` AND `\d+`)
// This call overrides default Symfony regular expression pattern `[^/]+` (see
// `RouteCompiler::compilePattern()`) with `.+` to allow URI parameters like
// `some-example-news/january` as well.
$requirements = $this->overrideValuesByAspect($route, $requirements, '.+');

if (!empty($requirements)) {
$route->setRequirements($requirements);
}
Expand All @@ -90,22 +102,20 @@ protected function filterValuesByPathVariables(Route $route, array $values): arr
}

/**
* Purges values that have an aspect definition.
*
* + aspects: ['page' => StaticRangeMapper()]
* + values: ['entity' => 'entity...', 'page' => 'page...', 'other' => 'other...']
* + result: ['entity' => 'entity...', 'other' => 'other...']
* Overrides items having an aspect definition with a given
* $overrideValue in target $values array.
*
* @param Route $route
* @param array $values
* @param string $overrideValue
* @return array
*/
protected function purgeValuesByAspects(Route $route, array $values): array
protected function overrideValuesByAspect(Route $route, array $values, string $overrideValue): array
{
return array_diff_key(
$values,
$route->getAspects()
);
foreach (array_keys($route->getAspects()) as $variableName) {
$values[$variableName] = $overrideValue;
}
return $values;
}

/**
Expand Down
Expand Up @@ -897,6 +897,71 @@ public function routeDefaultsForMultipleParametersAreConsidered(TestSet $testSet
$this->assertGeneratedUriEquals($testSet);
}

public function routeRequirementsAreSkippedHavingAspectsDataProvider($parentSet = null): array
{
$builder = Builder::create();
// variables (applied when invoking expectations)
$variables = Variables::create()->define([
'routePrefix' => 'enhance',
'aspectName' => 'value',
'inArguments' => 'staticArguments' // either 'dynamicArguments' or 'staticArguments'
]);
return Permutation::create($variables)
->withTargets(
TestSet::create($parentSet)
->withMergedApplicables(LanguageContext::create(0))
->withTargetPageId(1100)
->withUrl(
VariableValue::create(
'https://acme.us/welcome/enhance/[[value]][[pathSuffix]]',
Variables::create(['pathSuffix' => ''])
)
)
)
->withApplicableSet(
VariablesContext::create(Variables::create([
'value' => 'hundred',
'resolveValue' => 100,
])),
VariablesContext::create(Variables::create([
'value' => 'hundred/binary',
'resolveValue' => 1100100,
]))
)
->withApplicableItems($builder->declareEnhancers())
->withApplicableSet(
AspectDeclaration::create('StaticValueMapper')->withConfiguration([
VariableItem::create('aspectName', [
'type' => 'StaticValueMapper',
'map' => [
'hundred' => 100,
'hundred/binary' => 1100100,
],
])
])
)
->withApplicableSet(
EnhancerDeclaration::create('requirements.value=not-match-when-having-aspect')->withConfiguration([
'requirements' => [
'value' => 'not-match-when-having-aspect',
]
])
)
->permute()
->getTargetsForDataProvider();
}

/**
* @param TestSet $testSet
*
* @test
* @dataProvider routeRequirementsAreSkippedHavingAspectsDataProvider
*/
public function routeRequirementsAreSkippedHavingAspects(TestSet $testSet): void
{
$this->assertGeneratedUriEquals($testSet);
}

private function assertGeneratedUriEquals(TestSet $testSet): void
{
$builder = Builder::create();
Expand Down
Expand Up @@ -691,7 +691,6 @@ public function routeRequirementsAreSkippedHavingAspectsDataProvider($parentSet
$builder = Builder::create();
// variables (applied when invoking expectations)
$variables = Variables::create()->define([
'resolveValue' => 100,
'routePrefix' => 'enhance',
'aspectName' => 'value',
'inArguments' => 'staticArguments' // either 'dynamicArguments' or 'staticArguments'
Expand All @@ -703,18 +702,29 @@ public function routeRequirementsAreSkippedHavingAspectsDataProvider($parentSet
->withTargetPageId(1100)
->withUrl(
VariableValue::create(
'https://acme.us/welcome/enhance/hundred[[pathSuffix]]',
'https://acme.us/welcome/enhance/[[value]][[pathSuffix]]',
Variables::create(['pathSuffix' => ''])
)
)
)
->withApplicableSet(
VariablesContext::create(Variables::create([
'value' => 'hundred',
'resolveValue' => 100,
])),
VariablesContext::create(Variables::create([
'value' => 'hundred/binary',
'resolveValue' => 1100100,
]))
)
->withApplicableItems($builder->declareEnhancers())
->withApplicableSet(
AspectDeclaration::create('StaticValueMapper')->withConfiguration([
VariableItem::create('aspectName', [
'type' => 'StaticValueMapper',
'map' => [
'hundred' => 100,
'hundred/binary' => 1100100,
],
])
])
Expand Down

0 comments on commit e98ba4a

Please sign in to comment.