Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
bug #34192 [Routing] Fix URL generator instantiation (X-Coder264, Hyp…
…eMC)

This PR was merged into the 4.3 branch.

Discussion
----------

[Routing] Fix URL generator instantiation

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| License       | MIT

After merging my fix (PR #31023) from the 4.2 branch to 4.3 (which was master at the time) one code path which instantiates the url generator was missed. This new code path was added via #28865 and was missed during the merge because there was no merge conflict there as that code path didn't exist on 4.2.

So the current state on the 4.3 branch is:

- https://github.com/symfony/symfony/blob/v4.3.5/src/Symfony/Component/Routing/Router.php#L356 (default locale is properly passed to the constructor)

- https://github.com/symfony/symfony/blob/v4.3.5/src/Symfony/Component/Routing/Router.php#L372 (default locale is **NOT** properly passed to the constructor)

- https://github.com/symfony/symfony/blob/v4.3.5/src/Symfony/Component/Routing/Router.php#L378 (default locale is properly passed to the constructor)

I noticed this in my app because tests started to fail on the branch where we are upgrading our app from SF 4.2 to 4.3 due to this. This PR fixes the inconsistency (now the default locale is passed on all three places) which fixes the bug.

Commits
-------

9aa66e2 Add tests to ensure defaultLocale is properly passed to the URL generator
16c9baf Fix URL generator instantiation
  • Loading branch information
nicolas-grekas committed Nov 4, 2019
2 parents e057a9c + 9aa66e2 commit a56ac78
Show file tree
Hide file tree
Showing 2 changed files with 81 additions and 1 deletion.
2 changes: 1 addition & 1 deletion src/Symfony/Component/Routing/Router.php
Expand Up @@ -369,7 +369,7 @@ function (ConfigCacheInterface $cache) {
);

if ($compiled) {
$this->generator = new $this->options['generator_class'](require $cache->getPath(), $this->context, $this->logger);
$this->generator = new $this->options['generator_class'](require $cache->getPath(), $this->context, $this->logger, $this->defaultLocale);
} else {
if (!class_exists($this->options['generator_cache_class'], false)) {
require_once $cache->getPath();
Expand Down
80 changes: 80 additions & 0 deletions src/Symfony/Component/Routing/Tests/RouterTest.php
Expand Up @@ -22,10 +22,26 @@ class RouterTest extends TestCase

private $loader = null;

private $cacheDir;

protected function setUp(): void
{
$this->loader = $this->getMockBuilder('Symfony\Component\Config\Loader\LoaderInterface')->getMock();
$this->router = new Router($this->loader, 'routing.yml');

$this->cacheDir = sys_get_temp_dir().\DIRECTORY_SEPARATOR.uniqid('router_', true);
}

protected function tearDown(): void
{
if (is_dir($this->cacheDir)) {
array_map('unlink', glob($this->cacheDir.\DIRECTORY_SEPARATOR.'*'));
rmdir($this->cacheDir);
}

$this->loader = null;
$this->router = null;
$this->cacheDir = null;
}

public function testSetOptionsWithSupportedOptions()
Expand Down Expand Up @@ -132,4 +148,68 @@ public function testMatchRequestWithRequestMatcherInterface()

$this->router->matchRequest(Request::create('/'));
}

public function testDefaultLocaleIsPassedToGeneratorClass()
{
$this->loader->expects($this->once())
->method('load')->with('routing.yml', null)
->willReturn(new RouteCollection());

$router = new Router($this->loader, 'routing.yml', [
'cache_dir' => null,
], null, null, 'hr');

$generator = $router->getGenerator();

$this->assertInstanceOf('Symfony\Component\Routing\Generator\UrlGeneratorInterface', $generator);

$p = new \ReflectionProperty($generator, 'defaultLocale');
$p->setAccessible(true);

$this->assertSame('hr', $p->getValue($generator));
}

public function testDefaultLocaleIsPassedToCompiledGeneratorCacheClass()
{
$this->loader->expects($this->once())
->method('load')->with('routing.yml', null)
->willReturn(new RouteCollection());

$router = new Router($this->loader, 'routing.yml', [
'cache_dir' => $this->cacheDir,
], null, null, 'hr');

$generator = $router->getGenerator();

$this->assertInstanceOf('Symfony\Component\Routing\Generator\UrlGeneratorInterface', $generator);

$p = new \ReflectionProperty($generator, 'defaultLocale');
$p->setAccessible(true);

$this->assertSame('hr', $p->getValue($generator));
}

/**
* @group legacy
*/
public function testDefaultLocaleIsPassedToNotCompiledGeneratorCacheClass()
{
$this->loader->expects($this->once())
->method('load')->with('routing.yml', null)
->willReturn(new RouteCollection());

$router = new Router($this->loader, 'routing.yml', [
'cache_dir' => $this->cacheDir,
'generator_class' => 'Symfony\Component\Routing\Generator\UrlGenerator',
], null, null, 'hr');

$generator = $router->getGenerator();

$this->assertInstanceOf('Symfony\Component\Routing\Generator\UrlGeneratorInterface', $generator);

$p = new \ReflectionProperty($generator, 'defaultLocale');
$p->setAccessible(true);

$this->assertSame('hr', $p->getValue($generator));
}
}

0 comments on commit a56ac78

Please sign in to comment.