Skip to content

[Routing] Implement i18n routing #26143

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

Merged
merged 2 commits into from
Mar 19, 2018
Merged

[Routing] Implement i18n routing #26143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e32c414
[Routing] Implement i18n routing
frankdejonge Feb 11, 2018
4ae66dc
[Routing] Handle "_canonical_route"
nicolas-grekas Feb 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion src/Symfony/Component/Routing/Annotation/Route.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
class Route
{
private $path;
private $locales = array();
private $name;
private $requirements = array();
private $options = array();
Expand All @@ -38,11 +39,20 @@ class Route
*/
public function __construct(array $data)
{
if (isset($data['locales'])) {
throw new \BadMethodCallException(sprintf('Unknown property "locales" on annotation "%s".', get_class($this)));
}

if (isset($data['value'])) {
$data['path'] = $data['value'];
$data[is_array($data['value']) ? 'locales' : 'path'] = $data['value'];
unset($data['value']);
}

if (isset($data['path']) && is_array($data['path'])) {
$data['locales'] = $data['path'];
unset($data['path']);
}

foreach ($data as $key => $value) {
$method = 'set'.str_replace('_', '', $key);
if (!method_exists($this, $method)) {
Expand All @@ -62,6 +72,16 @@ public function getPath()
return $this->path;
}

public function setLocales(array $locales)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to have this setter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how annotations hydrate itself.

Copy link
Contributor

@linaori linaori Feb 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Small nitpicking, it's how this particular annotation behaves. Sadly there's no real standard for it. Doctrine for example, uses public properties from what I can tell. Other annotations might use a k=>v storage.

For this annotation it's required to have a setter as @frankdejonge mentioned

Copy link
Contributor

Choose a reason for hiding this comment

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

in theory we could reuse set/getPath() supporting both string and array API. Not sure if that simplifies the code in any way..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ro0NL could be, but we've handled it in the constructor now, which enables us to add type check for returns and make the "special" case more explicit. I think especially making the case explicit adds a lot of value because it's more clear.

{
$this->locales = $locales;
}

public function getLocales(): array
Copy link
Contributor

Choose a reason for hiding this comment

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

missing phpdoc. it's not clear what this method returns at all. a better name would also be getLocalizedPaths because that is what it seems to return

Copy link
Contributor

@Tobion Tobion Mar 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

esp. if we want to add to possiblity to add localized hosts as well in the future

Copy link
Member

Choose a reason for hiding this comment

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

updated in 7101893

{
return $this->locales;
}

public function setHost($pattern)
{
$this->host = $pattern;
Expand Down
13 changes: 11 additions & 2 deletions src/Symfony/Component/Routing/Generator/Dumper/PhpGeneratorDumper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,13 @@ public function dump(array $options = array())
class {$options['class']} extends {$options['base_class']}
{
private static \$declaredRoutes;
private \$defaultLocale;

public function __construct(RequestContext \$context, LoggerInterface \$logger = null)
public function __construct(RequestContext \$context, LoggerInterface \$logger = null, string \$defaultLocale = null)
{
\$this->context = \$context;
\$this->logger = \$logger;
\$this->defaultLocale = \$defaultLocale;
if (null === self::\$declaredRoutes) {
self::\$declaredRoutes = {$this->generateDeclaredRoutes()};
}
Expand Down Expand Up @@ -107,7 +109,14 @@ private function generateGenerateMethod()
return <<<'EOF'
public function generate($name, $parameters = array(), $referenceType = self::ABSOLUTE_PATH)
{
if (!isset(self::$declaredRoutes[$name])) {
$locale = $parameters['_locale']
?? $this->context->getParameter('_locale')
?: $this->defaultLocale;

if (null !== $locale && (self::$declaredRoutes[$name.'.'.$locale][1]['_canonical_route'] ?? null) === $name) {
unset($parameters['_locale']);
$name .= '.'.$locale;
} elseif (!isset(self::$declaredRoutes[$name])) {
throw new RouteNotFoundException(sprintf('Unable to generate a URL for the named route "%s" as such route does not exist.', $name));
}

Expand Down
13 changes: 11 additions & 2 deletions src/Symfony/Component/Routing/Generator/UrlGenerator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ class UrlGenerator implements UrlGeneratorInterface, ConfigurableRequirementsInt

protected $logger;

private $defaultLocale;

/**
* This array defines the characters (besides alphanumeric ones) that will not be percent-encoded in the path segment of the generated URL.
*
Expand Down Expand Up @@ -65,11 +67,12 @@ class UrlGenerator implements UrlGeneratorInterface, ConfigurableRequirementsInt
'%7C' => '|',
);

public function __construct(RouteCollection $routes, RequestContext $context, LoggerInterface $logger = null)
public function __construct(RouteCollection $routes, RequestContext $context, LoggerInterface $logger = null, string $defaultLocale = null)
{
$this->routes = $routes;
$this->context = $context;
$this->logger = $logger;
$this->defaultLocale = $defaultLocale;
}

/**
Expand Down Expand Up @@ -109,7 +112,13 @@ public function isStrictRequirements()
*/
public function generate($name, $parameters = array(), $referenceType = self::ABSOLUTE_PATH)
{
if (null === $route = $this->routes->get($name)) {
$locale = $parameters['_locale']
?? $this->context->getParameter('_locale')
?: $this->defaultLocale;
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on what I see in the constructor, this->defaultLocale could be null, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, which results in a route not found.

Copy link
Contributor

@ro0NL ro0NL Feb 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

it still produces an implicit route lookup, i.e. get("route.").

Instead why not normalize $name and keep the route lookup below as is...

if (null !== $locale) {
   $name .= '.'.$locale;
}

that would do no?


if (null !== $locale && null !== ($route = $this->routes->get($name.'.'.$locale)) && $route->getDefault('_canonical_route') === $name) {
unset($parameters['_locale']);
} elseif (null === $route = $this->routes->get($name)) {
throw new RouteNotFoundException(sprintf('Unable to generate a URL for the named route "%s" as such route does not exist.', $name));
}

Expand Down
67 changes: 57 additions & 10 deletions src/Symfony/Component/Routing/Loader/AnnotationClassLoader.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

use Doctrine\Common\Annotations\Reader;
use Symfony\Component\Config\Resource\FileResource;
use Symfony\Component\Routing\Annotation\Route as RouteAnnotation;
use Symfony\Component\Routing\Route;
use Symfony\Component\Routing\RouteCollection;
use Symfony\Component\Config\Loader\LoaderInterface;
Expand Down Expand Up @@ -119,9 +120,11 @@ public function load($class, $type = null)
}
}

/** @var $annot RouteAnnotation */
if (0 === $collection->count() && $class->hasMethod('__invoke') && $annot = $this->reader->getClassAnnotation($class, $this->routeAnnotationClass)) {
$globals['path'] = '';
$globals['path'] = null;
$globals['name'] = '';
$globals['locales'] = array();
$this->addRoute($collection, $annot, $globals, $class, $class->getMethod('__invoke'));
}

Expand All @@ -137,11 +140,6 @@ protected function addRoute(RouteCollection $collection, $annot, $globals, \Refl
$name = $globals['name'].$name;

$defaults = array_replace($globals['defaults'], $annot->getDefaults());
foreach ($method->getParameters() as $param) {
if (false !== strpos($globals['path'].$annot->getPath(), sprintf('{%s}', $param->getName())) && !isset($defaults[$param->getName()]) && $param->isDefaultValueAvailable()) {
$defaults[$param->getName()] = $param->getDefaultValue();
}
}
$requirements = array_replace($globals['requirements'], $annot->getRequirements());
$options = array_replace($globals['options'], $annot->getOptions());
$schemes = array_merge($globals['schemes'], $annot->getSchemes());
Expand All @@ -157,11 +155,57 @@ protected function addRoute(RouteCollection $collection, $annot, $globals, \Refl
$condition = $globals['condition'];
}

$route = $this->createRoute($globals['path'].$annot->getPath(), $defaults, $requirements, $options, $host, $schemes, $methods, $condition);
$path = $annot->getLocales() ?: $annot->getPath();
$prefix = $globals['locales'] ?: $globals['path'];
$paths = array();

$this->configureRoute($route, $class, $method, $annot);
if (\is_array($path)) {
if (!\is_array($prefix)) {
foreach ($path as $locale => $localePath) {
$paths[$locale] = $prefix.$localePath;
}
} elseif ($missing = array_diff_key($prefix, $path)) {
throw new \LogicException(sprintf('Route to "%s" is missing paths for locale(s) "%s".', $class->name.'::'.$method->name, implode('", "', array_keys($missing))));
} else {
foreach ($path as $locale => $localePath) {
if (!isset($prefix[$locale])) {
throw new \LogicException(sprintf('Route to "%s" with locale "%s" is missing a corresponding prefix in class "%s".', $method->name, $locale, $class->name));
}

$paths[$locale] = $prefix[$locale].$localePath;
}
}
} elseif (\is_array($prefix)) {
foreach ($prefix as $locale => $localePrefix) {
$paths[$locale] = $localePrefix.$path;
}
} else {
$paths[] = $prefix.$path;
}

$collection->add($name, $route);
foreach ($method->getParameters() as $param) {
if (isset($defaults[$param->name]) || !$param->isDefaultValueAvailable()) {
continue;
}
foreach ($paths as $locale => $path) {
if (false !== strpos($path, sprintf('{%s}', $param->name))) {
$defaults[$param->name] = $param->getDefaultValue();
break;
}
}
}

foreach ($paths as $locale => $path) {
$route = $this->createRoute($path, $defaults, $requirements, $options, $host, $schemes, $methods, $condition);
$this->configureRoute($route, $class, $method, $annot);
if (0 !== $locale) {
$route->setDefault('_locale', $locale);
$route->setDefault('_canonical_route', $name);
Copy link
Contributor

@Tobion Tobion Mar 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

_canonical_route should be defined as a constant somewhere (maybe UrlGeneratorInterface). it it used so often now and has special meaning. This also applies to _locale and _fragment I'd say.

$collection->add($name.'.'.$locale, $route);
} else {
$collection->add($name, $route);
}
}
}

/**
Expand Down Expand Up @@ -208,7 +252,8 @@ protected function getDefaultRouteName(\ReflectionClass $class, \ReflectionMetho
protected function getGlobals(\ReflectionClass $class)
{
$globals = array(
'path' => '',
'path' => null,
'locales' => array(),
'requirements' => array(),
'options' => array(),
'defaults' => array(),
Expand All @@ -228,6 +273,8 @@ protected function getGlobals(\ReflectionClass $class)
$globals['path'] = $annot->getPath();
}

$globals['locales'] = $annot->getLocales();

if (null !== $annot->getRequirements()) {
$globals['requirements'] = $annot->getRequirements();
}
Expand Down
51 changes: 36 additions & 15 deletions src/Symfony/Component/Routing/Loader/Configurator/CollectionConfigurator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,30 +24,25 @@ class CollectionConfigurator

private $parent;
private $parentConfigurator;
private $parentPrefixes;

public function __construct(RouteCollection $parent, string $name, self $parentConfigurator = null)
public function __construct(RouteCollection $parent, string $name, self $parentConfigurator = null, array $parentPrefixes = null)
{
$this->parent = $parent;
$this->name = $name;
$this->collection = new RouteCollection();
$this->route = new Route('');
$this->parentConfigurator = $parentConfigurator; // for GC control
$this->parentPrefixes = $parentPrefixes;
}

public function __destruct()
{
$this->collection->addPrefix(rtrim($this->route->getPath(), '/'));
$this->parent->addCollection($this->collection);
}

/**
* Adds a route.
*/
final public function add(string $name, string $path): RouteConfigurator
{
$this->collection->add($this->name.$name, $route = clone $this->route);
if (null === $this->prefixes) {
$this->collection->addPrefix($this->route->getPath());
}

return new RouteConfigurator($this->collection, $route->setPath($path), $this->name, $this);
$this->parent->addCollection($this->collection);
}

/**
Expand All @@ -57,18 +52,44 @@ final public function add(string $name, string $path): RouteConfigurator
*/
final public function collection($name = '')
{
return new self($this->collection, $this->name.$name, $this);
return new self($this->collection, $this->name.$name, $this, $this->prefixes);
}

/**
* Sets the prefix to add to the path of all child routes.
*
* @param string|array $prefix the prefix, or the localized prefixes
*
* @return $this
*/
final public function prefix(string $prefix)
final public function prefix($prefix)
{
$this->route->setPath($prefix);
if (\is_array($prefix)) {
if (null === $this->parentPrefixes) {
// no-op
} elseif ($missing = array_diff_key($this->parentPrefixes, $prefix)) {
throw new \LogicException(sprintf('Collection "%s" is missing prefixes for locale(s) "%s".', $this->name, implode('", "', array_keys($missing))));
} else {
foreach ($prefix as $locale => $localePrefix) {
if (!isset($this->parentPrefixes[$locale])) {
throw new \LogicException(sprintf('Collection "%s" with locale "%s" is missing a corresponding prefix in its parent collection.', $this->name, $locale));
}

$prefix[$locale] = $this->parentPrefixes[$locale].$localePrefix;
}
}
$this->prefixes = $prefix;
$this->route->setPath('/');
} else {
$this->prefixes = null;
$this->route->setPath($prefix);
}

return $this;
}

private function createRoute($path): Route
{
return (clone $this->route)->setPath($path);
}
}
29 changes: 27 additions & 2 deletions src/Symfony/Component/Routing/Loader/Configurator/ImportConfigurator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,36 @@ public function __destruct()
/**
* Sets the prefix to add to the path of all child routes.
*
* @param string|array $prefix the prefix, or the localized prefixes
*
* @return $this
*/
final public function prefix(string $prefix)
final public function prefix($prefix)
{
$this->route->addPrefix($prefix);
if (!\is_array($prefix)) {
$this->route->addPrefix($prefix);
} else {
foreach ($prefix as $locale => $localePrefix) {
$prefix[$locale] = trim(trim($localePrefix), '/');
}
foreach ($this->route->all() as $name => $route) {
if (null === $locale = $route->getDefault('_locale')) {
$this->route->remove($name);
foreach ($prefix as $locale => $localePrefix) {
$localizedRoute = clone $route;
$localizedRoute->setDefault('_locale', $locale);
$localizedRoute->setDefault('_canonical_route', $name);
$localizedRoute->setPath($localePrefix.$route->getPath());
$this->route->add($name.'.'.$locale, $localizedRoute);
}
} elseif (!isset($prefix[$locale])) {
throw new \InvalidArgumentException(sprintf('Route "%s" with locale "%s" is missing a corresponding prefix in its parent collection.', $name, $locale));
} else {
$route->setPath($prefix[$locale].$route->getPath());
$this->route->add($name, $route);
}
}
}

return $this;
}
Expand Down
4 changes: 2 additions & 2 deletions src/Symfony/Component/Routing/Loader/Configurator/RouteConfigurator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@

namespace Symfony\Component\Routing\Loader\Configurator;

use Symfony\Component\Routing\Route;
use Symfony\Component\Routing\RouteCollection;

/**
Expand All @@ -24,11 +23,12 @@ class RouteConfigurator

private $parentConfigurator;

public function __construct(RouteCollection $collection, Route $route, string $name = '', CollectionConfigurator $parentConfigurator = null)
public function __construct(RouteCollection $collection, $route, string $name = '', CollectionConfigurator $parentConfigurator = null, array $prefixes = null)
{
$this->collection = $collection;
$this->route = $route;
$this->name = $name;
$this->parentConfigurator = $parentConfigurator; // for GC control
$this->prefixes = $prefixes;
}
}
Loading