-
-
Notifications
You must be signed in to change notification settings - Fork 511
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
Application is being localized on non-localized routes #921
Comments
Friendly ping @iwasherefirst2 |
I was genuinely surprised to realize that the app locale is set for all routes, but you're absolutely right—it makes a lot of sense. I suspect I never noticed this because, in my use case, I primarily deal with non-localized API endpoints where the app locale’s value isn’t critical. Your idea is fantastic and represents a significant improvement over the current approach. I’ve always found it odd that Here are my thoughts on the specific points you raised:
This feels more elegant and avoids polluting the core Laravel
Possible Conflicts: Named Routes and Ziggy Integration A key downside of the proposed structure is its impact on named routes. The current system simplifies localization because With the new structure, achieving this functionality would require additional refactoring:
This level of change could deter many users from upgrading, especially if they rely heavily on named routes and tools like Ziggy. One would need to replace usage of the Laravel or Ziggy SummaryDespite the trade-offs, the proposed changes offer significant benefits, including resolving long-standing issues like the current caching complexity, the side effects of
Given the potential challenges of forcing all users to migrate and maintaining compatibility with the current approach, I’m leaning toward creating a new repository for the revised structure. This would allow users to choose the localization strategy that best suits their needs without introducing excessive maintenance burdens. What are your thoughts on this @jordyvanderhaegen ? It would also be helpful to hear the repository owner’s perspective on this @mcamara |
Hi @iwasherefirst2, thanks for sharing your thoughts!
Not sure, middleware could be applied by the user, or could be applied when registering the routes. Don't really have an idea on what the benefits or drawbacks on both approaches would be. The middleware could be named something similar to the current method name, e.g. I liked the idea of having it on the Route class because it could be chained by methods like
Good point, I didn't really think about this. Ideally, you should still be able to use the regular
This already exists through methods like
Leaving developers with multiple options to chose from doesn't seem like a good idea to me. That being said, I wouldn't mind having a go at this and sharing my thoughts along the way. |
If we can find a good solution that avoids breaking route names, it makes sense to integrate it into this repository as part of a potential v3. The new approach outlined in Laravel 11.x documentation is indeed very interesting. If I understand your proposal correctly, the idea is to move away from the original concept of iterating with a for loop to generate a separate route for each language. Instead, we’d define a single route that handles all languages by using {locale} as a parameter. This is a more modern and Laravel-aligned way to achieve localization. A possible implementation could look something like this:
With this setup:
I’d be happy to experiment with this approach further and explore its implications. Looks like a clean solution. |
@jordyvanderhaegen I started to work on your idea: https://github.com/mcamara/laravel-localization/pull/923/files#diff-924c6cd435d4d9746f12971cd4baef64d9236ae6fe2a43d4297affdd1d2a8873 The approach from my last response using So now, I’ve created a route macro that generates each route twice (if What I’ve done so far:
In general, this was much easier than I expected. I think most things will work out of the box with this approach. However, some tasks remain: To-Do List:
In the past few weeks, I’ve closed or labeled many issues. I’m confident that this new approach will resolve nearly all the issues labeled as bugs. Many feature requests were hard to implement previously due to the custom cache logic, but with that removed, they’ll be much simpler to address. Additionally, there’s no longer a need to use Overall, I believe this will be a really nice improvement. @jordyvanderhaegen Let me know if you’d like to work on any of the remaining tasks. I’d especially appreciate help with the Docker setup for running tests locally. Otherwise, I’ll continue with the tasks next week. |
The routes will look like this if
And if
|
Describe the bug
Currently, this package causes the app locale to be set when visiting routes that are not within a localized route group even when included in the
urlsIgnored
config option. This is caused by'prefix' => LaravelLocalization::setLocale()
which is part of the recommended configuration.One would expect the app to use the default locale when visiting a route that is not localized. Since
urlsIgnored
is only checked within the middleware of this package and not in theLaravelLocalization::setLocale()
method, this is not happening.So the
setLocale
method will eventually negotiate the language from the user'sAccept-Language
header.A short term solution might be to respect the
urlsIgnored
option in theLaravelLocalization::setLocale()
method.However, I think the entire
LaravelLocalization::setLocale()
being evaluated each time the routes file gets called is weird behaviour. From my experience working with Laravel, I would expect the locale to be set through middleware.This however, would require a substantial amount of this package to be rewritten and would definitely be a breaking change.
I wouldn't mind having a go at this, so here are some thoughts that come to mind when setting the locale through middleware:
supportedLocales
config option.This is something that should be handled by the package, not by the end user.
This would cause the amount of registered routes to grow, but should be fine when using route caching.
Ideally a user is able to register a group of routes that should be localized
Separate routes per locale would allow this package to use Laravel's default
php artisan route:cache
command.This is currently a custom command within this package and has caused some issues in the past.
Separate routes per locale would also show all localized routes within the
php artisan route:list
command.This would cause the command to display the following:
Setting the locale through middleware would make it easier for users to use this package within tests.
Currently, it is really cumbersome to start every test with
$this->refreshApplicationWithLocale('en')
.Setting the locale through middleware would make it easier to test this package itself.
A test case for the issues above looked something like this, where you really need to know the internals to know what's going on.
So just sharing my thoughts here, but this could solve some related issues like #617
Steps to reproduce
supportedLocales
in the config option:app.locale
to be set toen
urlsIgnored
config option:es
language/not-localized
in your browserExpected behavior
You should see the current locale be
es
instead of the expecteden
.The text was updated successfully, but these errors were encountered: