-
Notifications
You must be signed in to change notification settings - Fork 11k
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
[5.4] Localize routes #16541
[5.4] Localize routes #16541
Conversation
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 a good feature, however I am not convinced that registering the routes for all languages is the proper way to implement this. Have you looked into alternatives?
public function localize() | ||
{ | ||
if ($this->container && $this->container->bound('Illuminate\Routing\RoutesLocalizer')) { | ||
$localizer = $this->container->make('Illuminate\Routing\RoutesLocalizer'); |
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 the current Router instance be passed here as well?
To be honest I'm not convinced of this:
It should work quite opposite. Visiting |
*/ | ||
private function getLocaleFromRequest($request) | ||
{ | ||
return $this->requestPathHasLocale($request) ? |
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.
Since this method is private and the requestPathHasLocale
check has already been done in handle
why bother repeating it here?
I'm 👎 for this, I think that the route:list will be really dirty not sure if we can hide / group up these routes but we should consider it. Think about it if you have 8 locales defined in your config and you have a resource inside the localize it will turn into 9*7 = 63 entries on route:list just for a single resource. I'm 👍 for the rest this seems better than the previous PR and by appending the locale to the names we avoid creating a new helper... |
There's also one more thing to consider. You are mentioning Also I think what @fernandobandeira mentioned is good point. We probably wouldn't put all localized routes in |
@mnabialek we redirect to a URI with the default language to make URIs consistent across the application, I'm not quite sure what's best here, the other way around is easy but nots ure if it's the most common. |
Registering all routes is definitely the proper way to do it. Other packages that do not take this approach are not reaping the benefits of route caching at all. |
Could this be used as a group? Like if you want some routes registered for each languages, then you but them in a localize route group. However routes you do not like routing can be left out of the group. EDIT: Seems that this might already be possible using the middleware in a group. |
Yes that is already possible. |
@taylorotwell Why do we need to cache the routes? I understand that caching in itself is advantageous, however adding so many additional routes does put significant weight on the matching of routes. An alternative would be to test if the route starts with any of the known locales, and then proceed route matching using the remaining part of the URL. |
@themsaid Yes, it is somehow consistent, but it might be not the best way in all cases. In fact I think it should be configurable in what direction this redirection will work. Also keep in mind such use case - you have website with 100000 urls with one language and you decide to add new language. What should happen than? Would you like search engines to reindex all the urls because now something on domain.com/foo should be redirected to domain.com/en/foo ? Of course this is possible but it can affect rankings in search engines, also language prefix for current language can affect SEO. Also it would be useful if the routes localization take also other method - I mean domain method, so each of language might have separate domain (or subdomain) what could be also useful. I've mentioned about localization things http://stackoverflow.com/questions/23463101/seo-friendly-url-how-to-best-way/23504057#23504057 and http://stackoverflow.com/questions/25082154/how-to-create-multilingual-translated-routes-in-laravel if you would like take a look (obviously those are not the best solutions but they show things that should be considered when making routes localization). Also about localization some other things should be considered as for example #16429 or #11924 for instance (I haven't checked if they are already included) |
If people want to do localization with another approach, they can build a package to do it. That is not under discussion here. |
I think if the Laravel router could provide a new feature that allow the developper to define route parameters that are not referenced in the controller method (Maybe they could be called global parameters). It will allow to do something like this, and maybe solve the localized routes in a much cleaner way. We use Route::get('(locale)/articles/{slug}', [
'uses' => 'ArticlesController@show',
'as' => 'articles.show'
]); The route('articles.show', ['locale' => 'fr', 'slug' => $article->slug]) To avoid passing the locale when generating urls, the Laravel router could provide $router->bindGlobalParameter('locale', app()->getLocale());
// Or
$router->bindGlobalParameter('locale', function(){
// Custom logic
return $locale;
}); To avoid defining the In the class ArticlesController
{
// As you can see there is no $locale
public function show(Request $request, $slug)
{
$locale = $request->route()->globalParameter('locale');
}
} To avoid fetching the $locale manually on each request, a middleware could handle setting the application locale with What i think that |
@taylorotwell Could you expand further why route caching wont work with a {locale} prefix for all routes? Or is there some special part of it that doesn't do as well when having a dynamic part that early in the string? @themsaid I'm paranoid and missing tests that involves actual locales. Things like en_GB and en_US, es_ES and es_MX, fr_FR and fr_CA. I am not making these up, the majority of speakers of Spanish can be found in Mexico, not Spain, and they do have different dialect that needs separate translations. https://en.wikipedia.org/wiki/Spanish_dialects_and_varieties |
@themsaid @sisve |
I also think it'd be nice to have the opposite behaviour ( Or why not allowing both behaviours? |
@fernandobandeira It's mostly about clarity. You can currently make up any string; but what stops a future code change to consider that an error and limit the string to two lowercase characters? Boom, locale support broken but no test detected it. If the tests had actual locale values it would be more apparent that Laravel supports locales and they should work. |
I've read through both this PR and the one that referenced this. I really believe that what @taylorotwell and @themsaid are trying to do is a fantastic addition to the framework. What really bugs me is people like @sisve who comes along and says not one good thing about the idea, and offers no pull request, as was asked by @taylorotwell. Yes your arguments may be valid, but if you believe they are valid, back up your arguments. Provide examples. Provide PR's. @themsaid was fine by making a "staging PR," and there was absolutely no harm done to anyone by doing this. He presented an idea, people discussed it (somewhat civilally) and then he opened a new one that is more refined. I see no wrong doing here. Needless to say, if you don't want to use Laravels native locale support, then don't use it. There's nothing making you use it. If you don't want to use it then use your own package or a package that does what you want. What you want isn't necessary what the rest of the Laravel community wants. |
|
@sisve Agreed, I just hate coming in here and always seeing people being ripped apart. I didn't mean to solo you out either, want my intention, but after reading my original post I see that it may have came off that way. Sorry. |
Any news on this issue? |
News here #16650 |
Think I will just leave this to packages for now. There is too much disagreement and controversy surrounding the implementations in this thread and I don't want to deal with the fallout of that. Hopefully at least the URL hooks in place now will make it much easier for localized URLs to be built now. |
Hi @taylorotwell, How about an external laravel package created by laravel ? |
Now here #16736 |
Following up on: #16453
This PR applies localization on routes that have a
localize
middleware registered, only these routes will be registered once for every locale inapp.locales
configuration.For example, registering this route
/foo
will lead to laravel registering/en/foo
and/ar/foo
as well, so the three routes will be available for the dispatcher./en/foo
will present the foo route with the app locale set toen
./ar/foo
will present the foo route with the app locale set toar
./foo
will redirect the browser to/en/foo
sinceen
is the default locale.This approach will make all localized routes appear in the
route:list
command.If the route is named, the locale will be appended to the route name for the generated routes.
foo
,en.foo
,ar.foo
.Using
url('/bar')
will result ahttp://app.dev/en/bar
link based on the current locale, that way you don't have to worry about URL generation or Routes registration while building multilingual websites.To start localizing routes you need to add the following to your RoutesServiceProvider: