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.

Sign up for GitHub

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

Jump to bottom

[Routing] Prevent localized routes _locale default & requirement from being overridden #35928

Merged
merged 1 commit into from Mar 2, 2020
Merged

[Routing] Prevent localized routes _locale default & requirement from being overridden #35928

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 21 additions & 0 deletions src/Symfony/Component/Routing/Route.php
View file
Open in desktop
Expand Up @@ -376,6 +376,10 @@ public function setDefaults(array $defaults)
*/
public function addDefaults(array $defaults)
{
if (isset($defaults['_locale']) && $this->isLocalized()) {
unset($defaults['_locale']);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about throwing? (same in other places)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be a BC break, and more importantly it would forbid legit use cases where one imports several routes and some of them are localized.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand how this is breaking BC while we are already making it a noop here. Instead of legit use I see a silent failure, which could be reported at compile time for something wrongly configured.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no error on the userland side, reporting an exception would break legit use cases as I mentioned also. This is a bug, not a userland issue.

}

foreach ($defaults as $name => $default) {
$this->defaults[$name] = $default;
}
Expand Down Expand Up @@ -418,6 +422,10 @@ public function hasDefault($name)
*/
public function setDefault($name, $default)
{
if ('_locale' === $name && $this->isLocalized()) {
return $this;
}

$this->defaults[$name] = $default;
$this->compiled = null;

Expand Down Expand Up @@ -461,6 +469,10 @@ public function setRequirements(array $requirements)
*/
public function addRequirements(array $requirements)
{
if (isset($requirements['_locale']) && $this->isLocalized()) {
unset($requirements['_locale']);
}

foreach ($requirements as $key => $regex) {
$this->requirements[$key] = $this->sanitizeRequirement($key, $regex);
}
Expand Down Expand Up @@ -503,6 +515,10 @@ public function hasRequirement($key)
*/
public function setRequirement($key, $regex)
{
if ('_locale' === $key && $this->isLocalized()) {
return $this;
}

$this->requirements[$key] = $this->sanitizeRequirement($key, $regex);
$this->compiled = null;

Expand Down Expand Up @@ -577,4 +593,9 @@ private function sanitizeRequirement(string $key, $regex)

return $regex;
}

private function isLocalized(): bool
{
return isset($this->defaults['_locale']) && isset($this->defaults['_canonical_route']) && ($this->requirements['_locale'] ?? null) === preg_quote($this->defaults['_locale'], RouteCompiler::REGEX_DELIMITER);
}
}
61 changes: 61 additions & 0 deletions src/Symfony/Component/Routing/Tests/RouteTest.php
View file
Open in desktop
Expand Up @@ -271,4 +271,65 @@ public function testSerializedRepresentationKeepsWorking()
$this->assertEquals($route, $unserialized);
$this->assertNotSame($route, $unserialized);
}

/**
* @dataProvider provideNonLocalizedRoutes
*/
public function testLocaleDefaultWithNonLocalizedRoutes(Route $route)
{
$this->assertNotSame('fr', $route->getDefault('_locale'));
$route->setDefault('_locale', 'fr');
$this->assertSame('fr', $route->getDefault('_locale'));
}

/**
* @dataProvider provideLocalizedRoutes
*/
public function testLocaleDefaultWithLocalizedRoutes(Route $route)
{
$expected = $route->getDefault('_locale');
$this->assertIsString($expected);
$this->assertNotSame('fr', $expected);
$route->setDefault('_locale', 'fr');
$this->assertSame($expected, $route->getDefault('_locale'));
}

/**
* @dataProvider provideNonLocalizedRoutes
*/
public function testLocaleRequirementWithNonLocalizedRoutes(Route $route)
{
$this->assertNotSame('fr', $route->getRequirement('_locale'));
$route->setRequirement('_locale', 'fr');
$this->assertSame('fr', $route->getRequirement('_locale'));
}

/**
* @dataProvider provideLocalizedRoutes
*/
public function testLocaleRequirementWithLocalizedRoutes(Route $route)
{
$expected = $route->getRequirement('_locale');
$this->assertIsString($expected);
$this->assertNotSame('fr', $expected);
$route->setRequirement('_locale', 'fr');
$this->assertSame($expected, $route->getRequirement('_locale'));
}

public function provideNonLocalizedRoutes()
{
return [
[(new Route('/foo'))],
[(new Route('/foo'))->setDefault('_locale', 'en')],
[(new Route('/foo'))->setDefault('_locale', 'en')->setDefault('_canonical_route', 'foo')],
[(new Route('/foo'))->setDefault('_locale', 'en')->setDefault('_canonical_route', 'foo')->setRequirement('_locale', 'foobar')],
];
}

public function provideLocalizedRoutes()
{
return [
[(new Route('/foo'))->setDefault('_locale', 'en')->setDefault('_canonical_route', 'foo')->setRequirement('_locale', 'en')],
];
}
}