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.

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

[Routing] Implement i18n routing #26143

Merged
merged 2 commits into from Mar 19, 2018

Conversation

@frankdejonge
Contributor

frankdejonge commented Feb 11, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets N/A
License MIT

This PR introduces support for I18N routing into core. This is a port from a bundle I've made recently, now merged into the default implementation. While it's ok to have this as a bundle, it was suggested by @nicolas-grekas to create a PR for this so it can be included into the core.

New usages

YAML

contact:
    controller: ContactController::formAction
    path:
        en: /send-us-an-email
        nl: /stuur-ons-een-email

Will be effectively the same as declaring:

contact.en:
    controller: ContactController::formAction
    path: /send-us-an-email
    defaults:
        _locale: en

contact.nl:
    controller: ContactController::formAction
    path: /stuur-ons-een-email
    defaults:
        _locale: nl

Annotation usage:

<?php

use Symfony\Component\Routing\Annotation\Route;

class ContactController
{
    /**
     * @Route({"en": "/send-us-an-email", "nl": "/stuur-ons-een-email"}, name="contact") 
     */
    public function formAction()
    {
        
    }
}

/** 
 * @Route("/contact") 
 */
class PrefixedContactController
{
    /**
     * @Route({"en": "/send-us-an-email", "nl": "/stuur-ons-een-email"}, name="contact") 
     */
    public function formAction()
    {
        
    }
}

Route generation

<?php
/** @var UrlGeneratorInterface $urlGenerator */
$urlWithCurrentLocale = $urlGenerator->generate('contact');
$urlWithSpecifiedLocale = $urlGenerator->generate('contact', ['_locale' => 'nl']);

Route generation is based on your request locale. When not available it falls back on a configured default. This way of route generation means you have a "route locale switcher" out of the box, but generate the current route with another locale for most cases.

Advantages

Having i18n routes defined like this has some advantages:

  • Less error prone.
  • No need to keep requirements or defaults in sync with other definitions.
  • No need to {_locale} in the path (bad for route matching performance).
  • Better developer experience.

Next steps

I've ported all the things the bundle supported, before moving on I'd like to discuss this first in order not to waste our collective time. This initial PR should give a clear enough picture to see what/how/why this is done.

If and when accepted I/we can move forward to implement the XML loader and @nicolas-grekas mentioned there should be a Configurator implemented for this as well. He opted to help with this (for which I'm very thankful).

  • Yaml Loader
  • Annotation Loader
  • XML Loader
  • PHP Loader?
  • Documentation
@stof

Among the +999 -366 diff of this PR, half of it is totally unnecessary (and makes review and maintenance harder).

use InvalidArgumentException;
use LogicException;
use ReflectionClass;
use ReflectionMethod;

This comment has been minimized.

@stof

stof Feb 12, 2018

Member

Symfony never add use statements for classes of the global namespace. So please revert these changes

{
$name = $annot->getName();
$name = $annotation->getName();

This comment has been minimized.

@stof

stof Feb 12, 2018

Member

please don't rename the variable. It will make merging older branches into master harder for no benefit (and we will have to do it for years to come)

This comment has been minimized.

@SulivanDotEu

SulivanDotEu Feb 12, 2018

sad we cannot go for better...

This comment has been minimized.

@stof

stof Feb 12, 2018

Member

well, renaming it only in master makes our maintenance work harder for the next 5 years, each time this code would be modified. That's clearly not good (especially given that this would only make this code better for contributors, as users will never see this variable name)

$globals['path'] = $annot->getPath();
}
if (null !== $annot->getRequirements()) {

This comment has been minimized.

@stof

stof Feb 12, 2018

Member

Please avoid reorganizing this code. It makes merging branches harder (and again, for no actual benefit)

@@ -265,5 +351,7 @@ protected function createRoute($path, $defaults, $requirements, $options, $host,
return new Route($path, $defaults, $requirements, $options, $host, $schemes, $methods, $condition);
}
abstract protected function configureRoute(Route $route, \ReflectionClass $class, \ReflectionMethod $method, $annot);
protected function configureRoute(Route $route, ReflectionClass $class, ReflectionMethod $method, $annot)

This comment has been minimized.

@stof

stof Feb 12, 2018

Member

why making it a concrete method ?

This comment has been minimized.

@frankdejonge

frankdejonge Feb 12, 2018

Contributor

The tests for this class were testing through a partial mock, which is not a good way of testing it. In order to make the entire class concrete this method, which is merely a "hook", needs to be concrete too.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 12, 2018

Member

Could we keep it protected, and replace the mock by a child class? We're doing so in other places, with the class in the same file as the tests, below them.

This comment has been minimized.

@frankdejonge

frankdejonge Feb 12, 2018

Contributor

@nicolas-grekas do you mean keep it abstract?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 12, 2018

Member

yes if possible, so that implementers are forced to implement this from beginning

if (isset($config['resource']) && isset($config['path'])) {
throw new \InvalidArgumentException(sprintf(
'The routing file "%s" must not specify both the "resource" key and the "path" key for "%s". Choose between an import and a route definition.',
if (isset($config['resource']) && (isset($config['path']) || isset($config['locales']))) {

This comment has been minimized.

@stof

stof Feb 12, 2018

Member

I would use 2 different if, to be able to give the right error message for each case

*/
public function testLoadAbstractClass()
public function register_annotation_loader()

This comment has been minimized.

@stof

stof Feb 12, 2018

Member

please keep this in the setUp method

}
/**
* @test

This comment has been minimized.

@stof

stof Feb 12, 2018

Member

Please don't change all existing tests, especially when your new code does not follow the Symfony conventions. Changing existing tests makes it much harder for reviewers (I would now have to review all tests to be sure that the new ones are equivalent to the old ones, except I won't do it and I will wait for a revert instead).

@@ -111,8 +120,8 @@ public function testLoadWithResource()
public function testLoadRouteWithControllerAttribute()
{
$loader = new YamlFileLoader(new FileLocator(array(__DIR__.'/../Fixtures/controller')));
$routeCollection = $loader->load('routing.yml');
$this->loader = new YamlFileLoader(new FileLocator([__DIR__ . '/../Fixtures/controller']));

This comment has been minimized.

@stof

stof Feb 12, 2018

Member

why changing this to a class property everywhere ? The local variable is totally OK (and even avoids the need to look for other places using it).

Thus, it makes merging branches a pain.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 12, 2018

Member

I agree, this should be reverted

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Feb 12, 2018

@nicolas-grekas

Thank you for working on this!
For the reader, the corner stone of this PR is the additional logic in UrlGenerator::generate(). This is what provides more than just DX, but also a new feature.

I agree with stof: all non-related CS changes should be reverted, to keep the patch "pure" on what it achieves (and ease mergers' job in the future.)

if (isset(self::$declaredRoutes[$name.'.'.$locale])) {
unset($parameters['_locale']);
$name = $name.'.'.$locale;
goto generate;

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 12, 2018

Member

instead of the goto, the next "if" could just be an "elseif", isn't it?

This comment has been minimized.

@frankdejonge

frankdejonge Feb 12, 2018

Contributor

If I'd use an elseif then I would still need to use another else to throw the exception. It's all the same to me in the end, I thought this would create an easier to understand flow, but if it doesn't do that I'll happily revert it.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 12, 2018

Member

An "else" would be less surprising for the future reader so let's change yes please :)

if ($route instanceof Route) {
unset($parameters['_locale']);
goto generate;

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 12, 2018

Member

"elseif" also?

$this->addRoute($collection, $annot, $globals, $class, $class->getMethod('__invoke'));
}
return $collection;
}
protected function addRoute(RouteCollection $collection, $annot, $globals, \ReflectionClass $class, \ReflectionMethod $method)
/**
* @param RouteCollection $collection

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 12, 2018

Member

we removed what we considered "useless" docblocks previously
Id' suggest to do a partial docblock here, listing only $annot and $globals (that's supported : )

This comment has been minimized.

@frankdejonge

frankdejonge Feb 12, 2018

Contributor

That makes sense :)

* Discussion point:
* This is current behaviour. When a resource is imported and the defaults are set
* the parent defaults have precedence over the imported routes. This seems weird
* and probably unwanted to me. This can be fixed by correcting the behaviour in the

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 12, 2018

Member

I think this is on purpose: the logic is "who is responsible for their routes?". It must be the consumer - ie the outside of the imported routes. Said differently, imported routes should be overrideable, and this is what the current logic provides.
Yes, applying the same defaults to all routes might no be best. But then you can still redefine them one by one.
That's my understanding at least.

This comment has been minimized.

@frankdejonge

frankdejonge Feb 12, 2018

Contributor

Ah ok, it makes sense when you explain it like that, although I can't imagine anybody really using it like that. Then again, it falls under the BC promise so probably not wise to alter this behaviour. I'll remove the comments and keep the current implementation.

@@ -62,6 +63,16 @@ public function getPath()
return $this->path;
}
public function setLocales(array $locales)

This comment has been minimized.

@docteurklein

docteurklein Feb 12, 2018

Contributor

Is there a reason to have this setter?

This comment has been minimized.

@frankdejonge

frankdejonge Feb 12, 2018

Contributor

This is how annotations hydrate itself.

This comment has been minimized.

@iltar

iltar Feb 13, 2018

Contributor

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

This comment has been minimized.

@ro0NL

ro0NL Feb 13, 2018

Contributor

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

This comment has been minimized.

@frankdejonge

frankdejonge Feb 13, 2018

Contributor

@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.

if (null === $route = $this->routes->get($name)) {
$locale = $parameters['_locale']
?? $this->context->getParameter('_locale')
?: $this->defaultLocale;

This comment has been minimized.

@docteurklein

docteurklein Feb 12, 2018

Contributor

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

This comment has been minimized.

@frankdejonge

frankdejonge Feb 12, 2018

Contributor

Correct, which results in a route not found.

This comment has been minimized.

@ro0NL

ro0NL Feb 13, 2018

Contributor

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?

@frankdejonge

This comment has been minimized.

Contributor

frankdejonge commented Feb 12, 2018

@stof as mentioned in the description this is a port of the bundle in order to discuss the feature before putting in more time to get it in a mergeable state, which includes following conventions more closely and reverting CS changes for project maintainability. I see your points about the coding style changes and am happy to revert them if the feature is accepted (if not it's just wasted energy ;-) )

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Feb 12, 2018

Go to continue on my side, having this in core would be great.

@weaverryan

This comment has been minimized.

Member

weaverryan commented Feb 12, 2018

Nice work! This looks like a clear win to me to have in core. I recently worked with someone on a Symfony app who was really surprised this was not part of core, and then struggled through trying to use a community bundle (it was the worst part of his Symfony experience). Also, the bundle you created @frankdejonge looks great - but because it's not in core, there were a few "workarounds" you needed to do that aren't here (the I18nRoute and order of the bundle being registered).

Later, I can also help write (or just write) the docs for this :).

+1 and 🎆

@javiereguiluz

This comment has been minimized.

Member

javiereguiluz commented Feb 12, 2018

@frankdejonge thanks for contributing this feature. I like it! I haven't looked into the internals, but I have some questions and comments:


Question: would it be easier to understand if we allowed to use a map in the path option in addition to strings?

Before (no translation)

contact:
    controller: ContactController::formAction
    path: /send-us-an-email

After (original proposal)

contact:
    controller: ContactController::formAction
    locales:
        en: /send-us-an-email
        nl: /stuur-ons-een-email

After (alternative proposal)

contact:
    controller: ContactController::formAction
    path:
        en: /send-us-an-email
        nl: /stuur-ons-een-email

Comment: locale codes can be super tricky and weird. en, es, etc. are the best case. But these are also valid: pt-PT, pt_BR, fr.UTF-8, sr@latin, etc. Are we sure we support all those edge cases?


Question: how can I define different requirements per locale?

site:
    controller: DefaultController::siteAction
    path:
        en: /site/{category}
        es: /sitio/{category}
    requirements:
        # 'category' must be 'home|blog|contact' in English and 'inicio|blog|contacto' in Spanish
        category: ????

Question: how can you define different route imports per locale?

@Route("/site")   <-- in English
@Route("/sitio")  <-- in Spanish
class DefaultController extends Controller
{
    // ...
}
site:
    resource: '../src/Controller/'
    prefix: '/site'   <-- in English
    prefix: '/sitio'  <-- in Spanish
    type: annotation

Comment: we should also need to update the debug:router and router:match commands and also the profiler panel. But maybe we can leave this for a separate PR.


Question: can the placeholders of each locale be different? I ask because it's common to add the locale as a prefix for all URLs except for the ones of the default locale. Example:

/fr/voitures
/es/coches
/cars

This should be defined as:

cars:
    controller: DefaultController::carAction
    path:
        en: /cars
        es: /{_locale}/coches
        fr: /{_locale}/voitures
@stof

This comment has been minimized.

Member

stof commented Feb 12, 2018

@weaverryan the order of bundles being important is only because the bundle is not using proper service decoration features though AFAIK.

@stof

This comment has been minimized.

Member

stof commented Feb 12, 2018

Question: can the placeholders of each locale be different? I ask because it's common to add the locale as a prefix for all URLs except for the ones of the default locale. Example:

/fr/voitures
/es/coches
/cars

This should be defined as:

cars:
    controller: DefaultController::carAction
    path:
        en: /cars
        es: /{_locale}/coches
        fr: /{_locale}/voitures

@javiereguiluz this would be better defined using the locale statically rather than using {_locale} though (as you need to have one pattern per locale anyway). It would avoid making the pattern dynamic (and so making matching faster thanks to existing optimizations)

@nicolas-grekas

I didn't yet review the logic in the route annotation, but here are some CS-related comments. Annoying but has to be done...

$this->locales = $locales;
}
public function getLocales()

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 12, 2018

Member

array return type?

This comment has been minimized.

@frankdejonge

frankdejonge Feb 12, 2018

Contributor

I followed the rest of the methods here, none of them have return types or docblocks

@@ -53,10 +53,11 @@ class {$options['class']} extends {$options['base_class']}
{
private static \$declaredRoutes;
public function __construct(RequestContext \$context, LoggerInterface \$logger = null)
public function __construct(RequestContext \$context, LoggerInterface \$logger = null, \$defaultLocale = null)

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 12, 2018

Member

string type hint

@@ -37,6 +38,8 @@ class UrlGenerator implements UrlGeneratorInterface, ConfigurableRequirementsInt
protected $logger;
protected $defaultLocale;

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 12, 2018

Member

should be private I suppose (like we do for all new properties, no matter the previous ones)

@@ -65,11 +68,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, $defaultLocale = null)

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 12, 2018

Member

string type hint

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();
}
}

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 12, 2018

Member

I'd suggest to remove these 3 changed blank lines also sorry about that :)

$localeRoute = clone $route;
$localeRoute->setPath($path);
$localeRoute->setDefault('_locale', $locale);
yield "{$name}.{$locale}" => $localeRoute;

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 12, 2018

Member

$name.'.'.$locale

controller: ImportedController::someAction
locales:
nl: /voorbeeld
en: /example

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 12, 2018

Member

missing EOL (same below)

This comment has been minimized.

@frankdejonge

frankdejonge Feb 12, 2018

Contributor

This is fixed.

use Symfony\Component\Config\Resource\FileResource;
use Symfony\Component\Routing\Loader\YamlFileLoader;

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 12, 2018

Member

to be reverted: even ordering should be preserved

This comment has been minimized.

@frankdejonge

frankdejonge Feb 12, 2018

Contributor

It's so hard not to use my IDE...

@@ -111,8 +120,8 @@ public function testLoadWithResource()
public function testLoadRouteWithControllerAttribute()
{
$loader = new YamlFileLoader(new FileLocator(array(__DIR__.'/../Fixtures/controller')));
$routeCollection = $loader->load('routing.yml');
$this->loader = new YamlFileLoader(new FileLocator([__DIR__ . '/../Fixtures/controller']));

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 12, 2018

Member

I agree, this should be reverted

@@ -121,8 +130,8 @@ public function testLoadRouteWithControllerAttribute()
public function testLoadRouteWithoutControllerAttribute()
{
$loader = new YamlFileLoader(new FileLocator(array(__DIR__.'/../Fixtures/controller')));
$routeCollection = $loader->load('routing.yml');
$this->loader = new YamlFileLoader(new FileLocator([__DIR__ . '/../Fixtures/controller']));

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 12, 2018

Member

we still use array() vs [] for historical consistency (2.8 is still maintained for 1 year, after that we'll reconsider I hope)

This comment has been minimized.

@frankdejonge

frankdejonge Feb 12, 2018

Contributor

Ah, this is something PHPStorm did automatically on paste.

This comment has been minimized.

@frankdejonge

frankdejonge Feb 12, 2018

Contributor

@nicolas-grekas might be nice to have fabbot.io pick that up /cc @fabpot

@frankdejonge

This comment has been minimized.

Contributor

frankdejonge commented Feb 12, 2018

It took be the better part of an evening but I think I've reverted all the unneeded or seemingly unrelated changes from the PR.

@javiereguiluz I like your proposal of just using the path: key in the yaml definitions. What do the others think about that? @nicolas-grekas? @weaverryan? We could do the same for the annotation one, for that one we'd use the constructor to hydrate the correct field so the getLocales method would still have the return type check for better guarantees.

What are next steps? Can anybody help me with the XML one?

@frankdejonge

This comment has been minimized.

Contributor

frankdejonge commented Feb 12, 2018

@stof have all of your concerns been addressed now?

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Feb 12, 2018

I like using an array for path also.

$code = $this->generatorDumper->dump([
'class' => 'LocalizedProjectUrlGenerator',
]);
file_put_contents($this->testTmpFilepath, $code);

This comment has been minimized.

@stloyd

stloyd Feb 13, 2018

Contributor

You should unlink() the fixtures at end of this test.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 19, 2018

Member

already handled in tearDown

*/
public function testLoadMissingClass()
public function simple_path_routes()

This comment has been minimized.

@stloyd

stloyd Feb 13, 2018

Contributor

You should not mix test method naming & follow Symfony ones already, so: testSimplePathRoutes.

This comment has been minimized.

@frankdejonge

frankdejonge Feb 13, 2018

Contributor

Corrected.

@weaverryan

This comment has been minimized.

Member

weaverryan commented Feb 13, 2018

+1 for using path as an array instead of locales :)

@frankdejonge

This comment has been minimized.

Contributor

frankdejonge commented Feb 13, 2018

@javiereguiluz Hi! If you want to import you can do that localised too:

site:
    resource: '../src/Controller/'
    prefix:
        en: '/site'
        es: '/sitio'
    type: annotation
<?php

class DefaultController extends Controller
{
    /**
     * @Route({"en": "/contact", "es": "/contacto"}, name="contact_form")
     */
    public function contactAction() {}

    /**
     * @Route("/send", name="contact_send")
     */
    public function contactAction() {}
}

This results in:

name path
contact_form.en /site/contact
contact_form.es /sitio/contacto
contact_send.en /site/send
contact_send.es /sitio/send

So you can use localised prefixes with non-localised paths. You can use localised prefixes with localised paths. You can also use non-localised prefixes with localised paths. The one restriction there is, when mixing prefixes and paths with locales all locales must match.

@frankdejonge

This comment has been minimized.

Contributor

frankdejonge commented Feb 13, 2018

To further clarify, you can also prefix localised on class level:

<?php

/**
  * @Route({"en": "/site", "es": "/sitio"})
  */
class DefaultController extends Controller
{
    /**
     * @Route({"en": "/contact", "es": "/contacto"}, name="contact_form")
     */
    public function contactAction() {}

    /**
     * @Route("/send", name="contact_send")
     */
    public function contactAction() {}
}

Which results in those same definitions as my previous example.

@javiereguiluz

This comment has been minimized.

Member

javiereguiluz commented Feb 13, 2018

@frankdejonge fantastic! What a great job you have done in this pull request! Thank you (and thanks for your patience during the review process 🙏). The only two minor comments left from my previous comment are:

  • Double check that locales like this don't cause any problem -> pt-PT, pt_BR, fr.UTF-8, sr@latin, etc.
  • Update of the debug:router and router:match commands and also the routing profiler panel. But maybe we should leave this for a separate PR?
@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Feb 13, 2018

Update of the debug:router and router:match commands and also the routing profiler panel

Actually since this generates routes with the locale added as a suffix, there is nothing to do to have basic support. Should be enough, at least for a first round IMHO.

@frankdejonge

This comment has been minimized.

Contributor

frankdejonge commented Feb 13, 2018

@javiereguiluz in order to reassure you I've added this test case:

public function testImportingRoutesWithOfficialLocales()
{
    $loader = new YamlFileLoader(new FileLocator(array(__DIR__.'/../Fixtures/localized')));
    $routes = $loader->load('officially_formatted_locales.yml');

    $this->assertCount(3, $routes);
    $this->assertEquals('/omelette-du-fromage', $routes->get('official.fr.UTF-8')->getPath());
    $this->assertEquals('/eu-não-sou-espanhol', $routes->get('official.pt-PT')->getPath());
    $this->assertEquals('/churrasco', $routes->get('official.pt_BR')->getPath());
}
$route = $this->createRoute($globals['path'].$annot->getPath(), $defaults, $requirements, $options, $host, $schemes, $methods, $condition);
$path = $annot->getPath();
$locales = $annot->getLocales();
$hasLocalizedPrefix = false === empty($globals['locales']);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 13, 2018

Member

all false === empty() should be !empty()

$this->configureRoute($route, $class, $method, $annot);
if (false === $hasPrefix && false === $hasPathOrLocales) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 13, 2018

Member

if (!$hasPrefix && !$hasPathOrLocales) {

$this->configureRoute($route, $class, $method, $annot);
if (false === $hasPrefix && false === $hasPathOrLocales) {
throw new \LogicException("annotation for {$class->name}::{$method->name} has no path.");

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 13, 2018

Member

Annotation (upper case A) + double quotes around FQCN+method, could use sprintf

@@ -28,7 +28,8 @@
class YamlFileLoader extends FileLoader
{
private static $availableKeys = array(
'resource', 'type', 'prefix', 'path', 'host', 'schemes', 'methods', 'defaults', 'requirements', 'options', 'condition', 'controller', 'name_prefix',
'resource', 'type', 'prefix', 'path', 'host', 'schemes', 'methods',

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 13, 2018

Member

CS change, should be reverted

'The routing file "%s" must not specify both the "resource" key and the "path" key for "%s". Choose between an import and a route definition.',
$path, $name
));
throw new \InvalidArgumentException(sprintf('The routing file "%s" must not specify both the "resource" key and the "path" key for "%s". Choose between an import and a route definition.', $path, $name));

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 13, 2018

Member

I feel like may have mislead you sorry: if there are no changes here, the CS should be kept (same below)

/**
* @var YamlFileLoader
*/
private $loader;

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 13, 2018

Member

should be removed I suppose

class AnnotationClassLoaderTest extends AbstractAnnotationLoaderTest
use Doctrine\Common\Annotations\AnnotationReader;
use Doctrine\Common\Annotations\AnnotationRegistry;
use LogicException;

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 13, 2018

Member

oups, we missed this file, it's behind on CS, please revert them and keep only the relevant changes for this PR

This comment has been minimized.

@frankdejonge

frankdejonge Feb 13, 2018

Contributor

Seriously, we should really really really have a CS fixer which does this. It's SOOOOOO BORING and doesn't add any value, just frustration. All this code hasn't changes in 4 years, there's no TLS that has had any change in them, the likeliness of this changing again in older supported version is just so small.

@@ -39,10 +40,15 @@ class Route
public function __construct(array $data)
{
if (isset($data['value'])) {
$data['path'] = $data['value'];
$data[is_array($data['value']) ? 'locales' : 'path'] = $data['value'];

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 13, 2018

Member

does this mean it's possible to add locales=... in an annotation and have it work?
is this desired? if yes, it should be covered by a test case (but I'd say it shouldn't so that there is only one way to do it, isn't it?)

This comment has been minimized.

@frankdejonge

frankdejonge Feb 13, 2018

Contributor

The value is the first value so we map it to the correct property. Because in yaml we now use path: as the key I asked @weaverryan what to do about the annotation case, we figured it would be best to keep it inline. So for the annotation you can use path=LOCALISED_VALUE.

$locale = $parameters['_locale']
?? $this->context->getParameter('_locale')
?: $this->defaultLocale;
$route = $this->routes->get($name.'.'.$locale);

This comment has been minimized.

@Nyholm

Nyholm Feb 13, 2018

Member

Excuse my ignorance with the routing internals. But isnt this a BC break in some specific edge cases?

(I assume the UrlGenerator would get the default locale in the constructor automatically (ie Symfonys service definition is updated).)

If I have two routes with named foo and foo.en. They go to two different controllers. With current state of the PR and "en" as default locale, my second route will be overwritten by the "localized" version of foo.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 13, 2018

Member

That's correct, but that's not a BC break, because nobody has such localized routes today.

This comment has been minimized.

@Nyholm

Nyholm Feb 13, 2018

Member

I may have a "normal" route named foo.en, couldn't I?

This comment has been minimized.

@frankdejonge

frankdejonge Feb 13, 2018

Contributor

If you'd already have localised routes in the same way as now registered then and also have the same in a non-localised manner, then it would technically generate the wrong one. But that'd be when you have both home and home.en. I don't know how big this risk is, we could mitigate it by using a different suffix pattern (like double underscore or double point even).

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 13, 2018

Member

But that's not a BC break, that's just a regular collision. i.e. this won't break any unchanged existing code.

@Nyholm

This comment has been minimized.

Member

Nyholm commented Feb 13, 2018

Great feature. Thank you.

I would wish that there was a way to fetch all the localized version of a URL. It would be a real great thing if a future PR could support hreflang:

{% for locale in locales %}
    {% set arr = routeParams|merge({'_locale':locale}) %}
    <link rel="alternate" hreflang="{{ locale }}" href="{{ url(route, arr) }}" />
{% endfor %}
$path = $annot->getPath();
$locales = $annot->getLocales();
$hasLocalizedPrefix = !empty($globals['locales']);
$hasPrefix = $hasLocalizedPrefix || !empty($globals['path']);

This comment has been minimized.

@fabpot

fabpot Feb 14, 2018

Member

I would really avoid using empty whenever possible. $globals['path'] = '0'; would be considered empty here, which is not what we want.

This comment has been minimized.

@frankdejonge

frankdejonge Feb 14, 2018

Contributor

Converted this to null === checks to prevent this.

@frankdejonge

This comment has been minimized.

Contributor

frankdejonge commented Feb 26, 2018

@nicolas-grekas I've updated it based on our talk. There are some implementation caveats: in order to preserve routing order a new routing collection is built up during the loop. In order to preserve the "loaded resources" registration the new collection is built off the clone of the original (preserving all the metadata in a transparent way) then looping over its routes and removing them at the start. At the end of the loop all the original routes are removed and those without a localized path but imported with a locale are now properly expanded into their localized counterparts.

@nicolas-grekas

all good on my side

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Feb 28, 2018

ping @Tobion if you want to have a look, it's ready on my side :)

@Tobion

This comment has been minimized.

Member

Tobion commented Feb 28, 2018

Will take a look soon

@20uf

20uf approved these changes Feb 28, 2018

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Mar 12, 2018

Hello, I'm back :)
Without further feedback, I'd like to merge this PR this week.
It's on the path of (ie will conflict with) other improvements we're doing on the Routing component.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Mar 14, 2018

For transparency, here are some questions that are left open:

  1. using the translator component somehow -> use JMSI18nRoutingBundle (which itself could leverage the new _canonical_route parameter if wanted)
  2. generalize param-dependent routes to more than just _locale -> would add complexity, and use case is unclear, left for the future
  3. allow maps of per-locale hosts when importing (similar to prefix maps added in this PR)
  4. allow maps of per-locale hosts when defining a route

3 & 4 look quite achievable, with 3. being the most useful to my current understanding. I'd suggest to wait for someone to actually ask for it before implementing.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Mar 19, 2018

Thank you @frankdejonge.

@nicolas-grekas nicolas-grekas merged commit 4ae66dc into symfony:master Mar 19, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Mar 19, 2018

feature #26143 [Routing] Implement i18n routing (frankdejonge, nicola…
…s-grekas)

This PR was merged into the 4.1-dev branch.

Discussion
----------

[Routing]  Implement i18n routing

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      |no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | N/A
| License       | MIT

This PR introduces support for I18N routing into core. This is a port from a bundle I've made recently, now merged into the default implementation. While it's ok to have this as a bundle, it was suggested by @nicolas-grekas to create a PR for this so it can be included into the core.

## New usages

### YAML

```yaml
contact:
    controller: ContactController::formAction
    path:
        en: /send-us-an-email
        nl: /stuur-ons-een-email
```

Will be effectively the same as declaring:

```yaml
contact.en:
    controller: ContactController::formAction
    path: /send-us-an-email
    defaults:
        _locale: en

contact.nl:
    controller: ContactController::formAction
    path: /stuur-ons-een-email
    defaults:
        _locale: nl
```

### Annotation usage:

```php
<?php

use Symfony\Component\Routing\Annotation\Route;

class ContactController
{
    /**
     * @route({"en": "/send-us-an-email", "nl": "/stuur-ons-een-email"}, name="contact")
     */
    public function formAction()
    {

    }
}

/**
 * @route("/contact")
 */
class PrefixedContactController
{
    /**
     * @route({"en": "/send-us-an-email", "nl": "/stuur-ons-een-email"}, name="contact")
     */
    public function formAction()
    {

    }
}
```

### Route generation

```php
<?php
/** @var UrlGeneratorInterface $urlGenerator */
$urlWithCurrentLocale = $urlGenerator->generate('contact');
$urlWithSpecifiedLocale = $urlGenerator->generate('contact', ['_locale' => 'nl']);
```

Route generation is based on your request locale. When not available it falls back on a configured default. This way of route generation means you have a "route locale switcher" out of the box, but generate the current route with another locale for most cases.

## Advantages

Having i18n routes defined like this has some advantages:

* Less error prone.
* No need to keep `requirements` or `defaults` in sync with other definitions.
* No need to `{_locale}` in the path (bad for route matching performance).
* Better developer experience.

### Next steps

I've ported all the things the bundle supported, before moving on I'd like to discuss this first in order not to waste our collective time. This initial PR should give a clear enough picture to see what/how/why this is done.

If and when accepted I/we can move forward to implement the XML loader and @nicolas-grekas mentioned there should be a `Configurator` implemented for this as well. He opted to help with this (for which I'm very thankful).

- [x] Yaml Loader
- [x] Annotation Loader
- [x] XML Loader
- [x] PHP Loader?
- [ ] Documentation

Commits
-------

4ae66dc [Routing] Handle "_canonical_route"
e32c414 [Routing] Implement i18n routing
@frankdejonge

This comment has been minimized.

Contributor

frankdejonge commented Mar 19, 2018

@nicolas-grekas w000000t!

@frankdejonge frankdejonge deleted the frankdejonge:feature/i18n-routing branch Mar 19, 2018

*/
final public function add(string $name, string $path): RouteConfigurator
final public function add(string $name, $path): RouteConfigurator

This comment has been minimized.

@Tobion

Tobion Mar 19, 2018

Member

IMO it's better to offer a separate method like addLocalized where we can keep type declarations and makes the intention clear

This comment has been minimized.

@Tobion

Tobion Mar 21, 2018

Member

do you not agree?

This comment has been minimized.

@xabbuh

xabbuh Mar 21, 2018

Member

sounds good to me

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 21, 2018

Member

I prefer the current way, because each new method adds to the autocompletion list and amplifies the choice complexity.

This comment has been minimized.

@Tobion

Tobion Mar 21, 2018

Member

Then the best method is doIt(...$args) no more method needed

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 21, 2018

Member

@andersonamuller did you actually try it on your IDE? AFAIK, the @param in the phpdoc provides exactly what you are describing.

This comment has been minimized.

@andersonamuller

andersonamuller Mar 21, 2018

@nicolas-grekas I did, I use PhpStorm. I either have to have a new tab view open with the method documentation or select the method and the look for the documentation, but in the autocomplete hover list it does not show anything from the PHPDoc part.

This comment has been minimized.

@xabbuh

xabbuh Mar 21, 2018

Member

This is basically what it looks like in PhpStorm by default:

bildschirmfoto 2018-03-21 um 17 13 07

This comment has been minimized.

@Tobion

Tobion Mar 21, 2018

Member

@frankdejonge I don't think I deserve being lectured by you.

  1. My comment was not directed at you at all. So I cannot have hurt your feelings which is what the guidelines tries to prevent.
  2. Of the thousands review comments I did, how many of them did not respect the guidelines in you're opinion that I deserve being disciplined?
  3. I hope Nicolas does not feel disrespected by me considering how much praise I gave him for all his work so far.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 21, 2018

Member

@Tobion your comment was sarcastic for sure :) But I didn't take it personally so all is fine on my side. Still nice to take care of each other (and I feel we do by having these discussions.)

@xabbuh thanks for the screenshot. On my side, all is good as is, same as yaml.

$this->locales = $locales;
}
public function getLocales(): array

This comment has been minimized.

@Tobion

Tobion Mar 19, 2018

Member

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

This comment has been minimized.

@Tobion

Tobion Mar 19, 2018

Member

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

This comment has been minimized.

@nicolas-grekas
$this->configureRoute($route, $class, $method, $annot);
if (0 !== $locale) {
$route->setDefault('_locale', $locale);
$route->setDefault('_canonical_route', $name);

This comment has been minimized.

@Tobion

Tobion Mar 19, 2018

Member

_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.

@Tobion

This comment has been minimized.

Member

Tobion commented Mar 19, 2018

If you want to add a support for a new locale, but forget to define the translated path for a route, you'll get an exception trying to generate that route? Does not seem good DX wise and could easily break your app if you are not careful.

@frankdejonge

This comment has been minimized.

Contributor

frankdejonge commented Mar 19, 2018

@Tobion you'll get an exception when the routes are loaded, so compile-time not run-time.

@@ -24,6 +24,14 @@
</xsd:choice>
</xsd:complexType>
<xsd:complexType name="localised-path">

This comment has been minimized.

@xabbuh

xabbuh Mar 21, 2018

Member

localized-path as we use AE anywhere else?

This comment has been minimized.

@nicolas-grekas
@ampaze

This comment has been minimized.

ampaze commented Oct 2, 2018

For transparency, here are some questions that are left open:

  1. allow maps of per-locale hosts when importing (similar to prefix maps added in this PR)
  2. allow maps of per-locale hosts when defining a route

3 & 4 look quite achievable, with 3. being the most useful to my current understanding. I'd suggest to wait for someone to actually ask for it before implementing.

Would I be the first to ask for per-locale hosts when importing routes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment