Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
stof left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Among the +999 -366 diff of this PR, half of it is totally unnecessary (and makes review and maintenance harder).
| useInvalidArgumentException; | ||
| useLogicException; | ||
| useReflectionClass; | ||
| useReflectionMethod; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Symfony never add use statements for classes of the global namespace. So please revert these changes
| protectedfunctionaddRoute(RouteCollection$collection,$annotation,$globals,ReflectionClass$class,ReflectionMethod$method) | ||
| { | ||
| $name =$annot->getName(); | ||
| $name =$annotation->getName(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
sad we cannot go for better...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Please avoid reorganizing this code. It makes merging branches harder (and again, for no actual benefit)
| } | ||
| abstractprotectedfunctionconfigureRoute(Route$route,\ReflectionClass$class,\ReflectionMethod$method,$annot); | ||
| protectedfunctionconfigureRoute(Route$route,ReflectionClass$class,ReflectionMethod$method,$annot) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
why making it a concrete method ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@nicolas-grekas do you mean keep it abstract?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yes if possible, so that implementers are forced to implement this from beginning
| if (isset($config['resource']) &&isset($config['path'])) { | ||
| thrownew\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']))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would use 2 differentif, to be able to give the right error message for each case
| * @before | ||
| */ | ||
| publicfunctiontestLoadAbstractClass() | ||
| publicfunctionregister_annotation_loader() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
please keep this in thesetUp method
| } | ||
| /** | ||
| * @test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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).
| { | ||
| $loader =newYamlFileLoader(newFileLocator(array(__DIR__.'/../Fixtures/controller'))); | ||
| $routeCollection =$loader->load('routing.yml'); | ||
| $this->loader =newYamlFileLoader(newFileLocator([__DIR__ .'/../Fixtures/controller'])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I agree, this should be reverted
nicolas-grekas left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thank you for working on this!
For the reader, the corner stone of this PR is the additional logic inUrlGenerator::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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
instead of the goto, the next "if" could just be an "elseif", isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
An "else" would be less surprising for the future reader so let's change yes please :)
| if ($routeinstanceof Route) { | ||
| unset($parameters['_locale']); | ||
| goto generate; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
"elseif" also?
| protectedfunctionaddRoute(RouteCollection$collection,$annot,$globals,\ReflectionClass$class,\ReflectionMethod$method) | ||
| /** | ||
| * @param RouteCollection $collection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we removed what we considered "useless" docblocks previously
Id' suggest to do a partial docblock here, listing only $annot and $globals (that's supported : )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 |
nicolas-grekasFeb 12, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
| return$this->path; | ||
| } | ||
| publicfunctionsetLocales(array$locales) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
in theory we could reuseset/getPath() supporting both string and array API. Not sure if that simplifies the code in any way..
There was a problem hiding this comment.
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.
| if (null ===$route =$this->routes->get($name)) { | ||
| $locale =$parameters['_locale'] | ||
| ??$this->context->getParameter('_locale') | ||
| ?:$this->defaultLocale; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
frankdejonge commentedFeb 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 commentedFeb 12, 2018
Go to continue on my side, having this in core would be great. |
weaverryan commentedFeb 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 wasnot 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 Later, I can also help write (or just write) the docs for this :). +1 and 🎆 |
javiereguiluz commentedFeb 12, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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 Before (no translation)contact:controller:ContactController::formActionpath:/send-us-an-email After (original proposal)contact:controller:ContactController::formActionlocales:en:/send-us-an-emailnl:/stuur-ons-een-email After (alternative proposal)contact:controller:ContactController::formActionpath:en:/send-us-an-emailnl:/stuur-ons-een-email Comment: locale codes can be super tricky and weird. Question: how can I define different requirements per locale? site:controller:DefaultController::siteActionpath:en:/site/{category}es:/sitio/{category}requirements:# 'category' must be 'home|blog|contact' in English and 'inicio|blog|contacto' in Spanishcategory:???? Question: how can you define different route imports per locale? @Route("/site") <-- in English@Route("/sitio") <-- in Spanishclass DefaultControllerextends Controller{// ...} site:resource:'../src/Controller/'prefix:'/site'<-- in Englishprefix:'/sitio'<-- in Spanishtype: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: This should be defined as: cars:controller:DefaultController::carActionpath:en:/carses:/{_locale}/cochesfr:/{_locale}/voitures |
stof commentedFeb 12, 2018
@weaverryan the order of bundles being important is only because the bundle is not using proper service decoration features though AFAIK. |
stof commentedFeb 12, 2018
@javiereguiluz this would be better defined using the locale statically rather than using |
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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; | ||
| } | ||
| publicfunctiongetLocales() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
array return type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I followed the rest of the methods here, none of them have return types or docblocks
| private static\$declaredRoutes; | ||
| public function __construct(RequestContext\$context, LoggerInterface\$logger = null) | ||
| public function __construct(RequestContext\$context, LoggerInterface\$logger = null,\$defaultLocale = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
string type hint
| protected$logger; | ||
| protected$defaultLocale; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should be private I suppose (like we do for all new properties, no matter the previous ones)
| ); | ||
| publicfunction__construct(RouteCollection$routes,RequestContext$context,LoggerInterface$logger =null) | ||
| publicfunction__construct(RouteCollection$routes,RequestContext$context,LoggerInterface$logger =null,$defaultLocale =null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
string type hint
| $defaults[$param->getName()] =$param->getDefaultValue(); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
$name.'.'.$locale
| controller:ImportedController::someAction | ||
| locales: | ||
| nl:/voorbeeld | ||
| en:/example No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
missing EOL (same below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is fixed.
| useSymfony\Component\Config\FileLocator; | ||
| useSymfony\Component\Routing\Loader\YamlFileLoader; | ||
| useSymfony\Component\Config\Resource\FileResource; | ||
| useSymfony\Component\Routing\Loader\YamlFileLoader; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
to be reverted: even ordering should be preserved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's so hard not to use my IDE...
| { | ||
| $loader =newYamlFileLoader(newFileLocator(array(__DIR__.'/../Fixtures/controller'))); | ||
| $routeCollection =$loader->load('routing.yml'); | ||
| $this->loader =newYamlFileLoader(newFileLocator([__DIR__ .'/../Fixtures/controller'])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I agree, this should be reverted
| { | ||
| $loader =newYamlFileLoader(newFileLocator(array(__DIR__.'/../Fixtures/controller'))); | ||
| $routeCollection =$loader->load('routing.yml'); | ||
| $this->loader =newYamlFileLoader(newFileLocator([__DIR__ .'/../Fixtures/controller'])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we still use array() vs [] for historical consistency (2.8 is still maintained for 1 year, after that we'll reconsider I hope)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah, this is something PHPStorm did automatically on paste.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@nicolas-grekas might be nice to have fabbot.io pick that up /cc@fabpot
frankdejonge commentedFeb 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 What are next steps? Can anybody help me with the XML one? |
frankdejonge commentedFeb 12, 2018
@stof have all of your concerns been addressed now? |
nicolas-grekas commentedFeb 12, 2018
I like using an array for path also. |
| $code =$this->generatorDumper->dump([ | ||
| 'class' =>'LocalizedProjectUrlGenerator', | ||
| ]); | ||
| file_put_contents($this->testTmpFilepath,$code); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You shouldunlink() the fixtures at end of this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
already handled in tearDown
| * @test | ||
| */ | ||
| publicfunctiontestLoadMissingClass() | ||
| publicfunctionsimple_path_routes() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You should not mix test method naming & follow Symfony ones already, so:testSimplePathRoutes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Corrected.
weaverryan commentedFeb 13, 2018
+1 for using |
frankdejonge commentedFeb 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 <?phpclass DefaultControllerextends Controller{/** * @Route({"en": "/contact", "es": "/contacto"}, name="contact_form") */publicfunctioncontactAction() {}/** * @Route("/send", name="contact_send") */publicfunctioncontactAction() {}} This results in:
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 commentedFeb 13, 2018
To further clarify, you can also prefix localised on class level: <?php/** * @Route({"en": "/site", "es": "/sitio"}) */class DefaultControllerextends Controller{/** * @Route({"en": "/contact", "es": "/contacto"}, name="contact_form") */publicfunctioncontactAction() {}/** * @Route("/send", name="contact_send") */publicfunctioncontactAction() {}} Which results in those same definitions as my previous example. |
javiereguiluz commentedFeb 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:
|
nicolas-grekas commentedFeb 13, 2018
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 commentedFeb 13, 2018
@javiereguiluz in order to reassure you I've added this test case: publicfunctiontestImportingRoutesWithOfficialLocales(){$loader =newYamlFileLoader(newFileLocator(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']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
allfalse === empty() should be!empty()
| $hasPathOrLocales =false ===empty($path) ||$isLocalized; | ||
| $this->configureRoute($route,$class,$method,$annot); | ||
| if (false ===$hasPrefix &&false ===$hasPathOrLocales) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
if (!$hasPrefix && !$hasPathOrLocales) {
| $this->configureRoute($route,$class,$method,$annot); | ||
| if (false ===$hasPrefix &&false ===$hasPathOrLocales) { | ||
| thrownew \LogicException("annotation for{$class->name}::{$method->name} has no path."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Annotation (upper case A) + double quotes around FQCN+method, could use sprintf
| { | ||
| privatestatic$availableKeys =array( | ||
| 'resource','type','prefix','path','host','schemes','methods','defaults','requirements','options','condition','controller','name_prefix', | ||
| 'resource','type','prefix','path','host','schemes','methods', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 | ||
| )); | ||
| thrownew \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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should be removed I suppose
| class AnnotationClassLoaderTestextends AbstractAnnotationLoaderTest | ||
| useDoctrine\Common\Annotations\AnnotationReader; | ||
| useDoctrine\Common\Annotations\AnnotationRegistry; | ||
| useLogicException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
oups, we missed this file, it's behind on CS, please revert them and keep only the relevant changes for this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
| { | ||
| if (isset($data['value'])) { | ||
| $data['path'] =$data['value']; | ||
| $data[is_array($data['value']) ?'locales' :'path'] =$data['value']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
does this mean it's possible to addlocales=... 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?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thevalue is the first value so we map it to the correct property. Because in yaml we now usepath: 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 usepath=LOCALISED_VALUE.
| $locale =$parameters['_locale'] | ||
| ??$this->context->getParameter('_locale') | ||
| ?:$this->defaultLocale; | ||
| $route =$this->routes->get($name.'.'.$locale); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 namedfoo andfoo.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 offoo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That's correct, but that's not a BC break, because nobody has such localized routes today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I may have a "normal" route namedfoo.en, couldn't I?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 bothhome andhome.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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
But that's not a BC break, that's just a regular collision. i.e. this won't break anyunchanged existing code.
Nyholm commentedFeb 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: {%forlocaleinlocales %} {%setarr=routeParams|merge({'_locale':locale}) %} <linkrel="alternate"hreflang="{{locale }}"href="{{ url(route,arr) }}" />{%endfor %} |
| $path =$annot->getPath(); | ||
| $locales =$annot->getLocales(); | ||
| $hasLocalizedPrefix = !empty($globals['locales']); | ||
| $hasPrefix =$hasLocalizedPrefix || !empty($globals['path']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would really avoid usingempty whenever possible.$globals['path'] = '0'; would be considered empty here, which is not what we want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Converted this tonull === checks to prevent this.
nicolas-grekas commentedFeb 28, 2018
ping@Tobion if you want to have a look, it's ready on my side :) |
Tobion commentedFeb 28, 2018
Will take a look soon |
nicolas-grekas commentedMar 12, 2018
Hello, I'm back :) |
nicolas-grekas commentedMar 14, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
For transparency, here are some questions that are left open:
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 commentedMar 19, 2018
Thank you@frankdejonge. |
…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 | MITThis 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```yamlcontact: controller: ContactController::formAction path: en: /send-us-an-email nl: /stuur-ons-een-email```Will be effectively the same as declaring:```yamlcontact.en: controller: ContactController::formAction path: /send-us-an-email defaults: _locale: encontact.nl: controller: ContactController::formAction path: /stuur-ons-een-email defaults: _locale: nl```### Annotation usage:```php<?phpuse 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.## AdvantagesHaving 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 stepsI'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?- [ ] DocumentationCommits-------4ae66dc [Routing] Handle "_canonical_route"e32c414 [Routing] Implement i18n routing
frankdejonge commentedMar 19, 2018
@nicolas-grekas w000000t! |
| * @param string|array $path the path, or the localized paths of the route | ||
| */ | ||
| finalpublicfunctionadd(string$name,string$path):RouteConfigurator | ||
| finalpublicfunctionadd(string$name,$path):RouteConfigurator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IMO it's better to offer a separate method likeaddLocalized where we can keep type declarations and makes the intention clear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
do you not agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
sounds good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I prefer the current way, because each new method adds to the autocompletion list and amplifies the choice complexity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Then the best method isdoIt(...$args) no more method needed
nicolas-grekasMar 21, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@andersonamuller did you actually try it on your IDE? AFAIK, the@param in the phpdoc provides exactly what you are describing.
andersonamullerMar 21, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is basically what it looks like in PhpStorm by default:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@frankdejonge I don't think I deserve being lectured by you.
- My comment was not directed at you at all. So I cannot have hurt your feelings which is what the guidelines tries to prevent.
- 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?
- I hope Nicolas does not feel disrespected by me considering how much praise I gave him for all his work so far.
nicolas-grekasMar 21, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| $this->locales =$locales; | ||
| } | ||
| publicfunctiongetLocales():array |
There was a problem hiding this comment.
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 begetLocalizedPaths because that is what it seems to return
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
updated in7101893
| $this->configureRoute($route,$class,$method,$annot); | ||
| if (0 !==$locale) { | ||
| $route->setDefault('_locale',$locale); | ||
| $route->setDefault('_canonical_route',$name); |
There was a problem hiding this comment.
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.
Tobion commentedMar 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 commentedMar 19, 2018
@Tobion you'll get an exception when the routes are loaded, so compile-time not run-time. |
| </xsd:choice> | ||
| </xsd:complexType> | ||
| <xsd:complexTypename="localised-path"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
localized-path as we use AE anywhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed in2004092
ampaze commentedOct 2, 2018
Would I be the first to ask for per-locale hosts when importing routes? |
remmel commentedMar 16, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I see 2 possibilities, can we agree with one and how it will work ?
site:resource:'../src/Controller/'prefix:en:'/site'es:'://www.website.es/sitio'type:annotation Questions:
site:resource:'../src/Controller/'prefix:en:'/site'es:'/sitio'host:es:'www.website.es'type:annotation Questions:
|
nicolas-grekas commentedMar 16, 2019
@remmel could you please open a separate issue with some introduction + your question? I don't remember so I can't tell now, and if I do this will be lost in some already merged PR. Thanks |
remmel commentedMar 20, 2019
Yes@nicolas-grekas I've just done it in#30617 |
Uh oh!
There was an error while loading.Please reload this page.
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
Will be effectively the same as declaring:
Annotation usage:
Route generation
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:
requirementsordefaultsin sync with other definitions.{_locale}in the path (bad for route matching performance).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
Configuratorimplemented for this as well. He opted to help with this (for which I'm very thankful).