Skip to content
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

Open
jordyvanderhaegen opened this issue Dec 2, 2024 · 6 comments
Open

Application is being localized on non-localized routes #921

jordyvanderhaegen opened this issue Dec 2, 2024 · 6 comments
Assignees

Comments

@jordyvanderhaegen
Copy link
Collaborator

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 the LaravelLocalization::setLocale() method, this is not happening.
So the setLocale method will eventually negotiate the language from the user's Accept-Language header.

A short term solution might be to respect the urlsIgnored option in the LaravelLocalization::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:

  1. Separate routes would need to be registered for each prefix (locale) within the 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
- Route::group([
-     'prefix' => LaravelLocalization::setLocale(),
-     'middleware' => ['localeSessionRedirect', 'localizationRedirect', 'localeViewPath']
- ], function () {
-     Route::get('about', function () { ... });
- });
+ Route::localizedGroup(function () {
+     Route::get('about', function () { ... });
+ });
  1. 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.

  2. 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:

- GET|HEAD       about-mcamara ..................................................................
+ GET|HEAD       es/about-mcamara ...............................................................
+ GET|HEAD       en/about-mcamara ...............................................................
  1. 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').

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

final class IgnoreUrlsTest extends TestCase
{
    use UsesLocalizedRoutes;

    protected function setUp(): void
    {
        putenv('APP_RUNNING_IN_CONSOLE=false');

        parent::setUp();
    }

    protected function defineRoutes($router): void
    {
        $router->group([
            'prefix' => LaravelLocalization::setLocale(),
            'middleware' => [
                LaravelLocalizationRoutes::class,
                LaravelLocalizationRedirectFilter::class,
                LocaleSessionRedirect::class,
            ],
        ], function () {});

        $router->get('without-localization', function () {
            return app()->getLocale();
        });
    }

    protected function defineEnvironment($app): void
    {
        $app['config']->set('app.locale', 'en');
        $app['config']->set('laravellocalization.urlsIgnored', ['/without-localization']);
        $app->instance('request', Request::create(
            uri: '/without-localization',
            server: ['HTTP_ACCEPT_LANGUAGE' => 'es'],
        ));
    }

    public function testCanIgnoreUrls(): void
    {
        $response = $this->get('/without-localization');

        $response->assertStatus(200);

        $this->assertEquals('en', $response->getContent());
    }
}

So just sharing my thoughts here, but this could solve some related issues like #617

Steps to reproduce

  1. Create a route file with the following content
Route::group([
    'prefix' => LaravelLocalization::setLocale(),
    'middleware' => ['localeSessionRedirect', 'localizationRedirect', 'localeViewPath']
], function () {
    Route::get('localized', fn () => app()->getLocale());
});

Route::get('not-localized', fn () => app()->getLocale());
  1. Provide at least 2 supportedLocales in the config option:
'supportedLocales' => [
    'en'          => ['name' => 'English',                'script' => 'Latn', 'native' => 'English', 'regional' => 'en_GB'],
    'es'          => ['name' => 'Spanish',                'script' => 'Latn', 'native' => 'español', 'regional' => 'es_ES'],
],
  1. Configure your app.locale to be set to en
  2. Include the route that is not localized within the urlsIgnored config option:
'urlsIgnored' => ['/not-localized'],
  1. Configure your browser to prefer the es language
  2. Browse to /not-localized in your browser

Expected behavior

You should see the current locale be es instead of the expected en.

@jordyvanderhaegen
Copy link
Collaborator Author

Friendly ping @iwasherefirst2

@iwasherefirst2
Copy link
Collaborator

iwasherefirst2 commented Dec 2, 2024

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 LaravelLocalization::setLocale() is used directly in the routes file. This becomes even more cumbersome in modular Laravel setups, where LaravelLocalization::setLocale() has to be repeated across multiple route files, which isn’t ideal.

Here are my thoughts on the specific points you raised:

  1. Route Middleware Configuration
    This looks excellent. I assume the middleware to be applied would be configurable? Having a dedicated function for defining localized routes is much cleaner. It might make sense to add a routes function to the LaravelLocalization class like this, instead of adding a new function to Laravel’s Route class:
LaravelLocalization::routes(function () {
    Route::get('about', function () {
        // ...
    });
});

This feels more elegant and avoids polluting the core Laravel Route class.

  1. Caching Improvements This would be an amazing improvement. Maintaining the custom caching logic has been a pain point, and it’s the main blocker for implementing requests like Issue #382.

  2. Handling All Route Variants I believe all three route variants should be generated, depending on whether hideDefaultLocaleInURL is enabled. It would also be a great improvement if artisan route:list displayed all routes, including localized ones. Laravel’s route filtering (e.g., artisan route:list --path=/en/) makes it easy to view routes for specific locales when needed.

  3. & 5. Refactoring These points are excellent. Splitting middleware into smaller, testable units and cleaning up the code will greatly improve maintainability.

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 route('my.route') automatically generates the correct localized URL, avoiding the need for LaravelLocalization::localizeUrl('/test'). This is particularly valuable when combined with tools like Ziggy, which enable JavaScript to access named routes directly. Losing this functionality would require significant refactoring for users and could discourage adoption of the new structure.

With the new structure, achieving this functionality would require additional refactoring:

  • Prefixing each route group with a locale-based name like de.*, en.*, hidden_locale.* etc.
  • Creating custom route helpers for both PHP and JavaScript to dynamically generate the correct localized URLs.

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 route helper in all files.

Summary

Despite the trade-offs, the proposed changes offer significant benefits, including resolving long-standing issues like the current caching complexity, the side effects of setLocale within routes, incomplete route:list results, and incompatibilities with tools like Nova. While the loss of seamless route helper functionality is a drawback, the overall simplification and maintainability improvements outweigh the downsides. I think we have two options now:

  1. Force All Users to Migrate
    This approach would require all users upgrading to version 3.x to update their routes.php files and replace all uses of the route helper in their PHP and JavaScript files. While this is the easiest option for maintainers, it imposes the heaviest burden on users, which could discourage adoption.

  2. Separate Repository
    Another option is to create a new repository for the revised structure. This would allow developers to explicitly choose the localization strategy that suits their app. While this reduces confusion and avoids forcing all users into a single paradigm, maintaining two repositories increases the maintenance burden. Alternatively, the current repository could be marked as deprecated, with only minimal updates (e.g., bumping Laravel versions) moving forward.

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

@jordyvanderhaegen
Copy link
Collaborator Author

Hi @iwasherefirst2, thanks for sharing your thoughts!

I assume the middleware to be applied would be configurable? Having a dedicated function for defining localized routes is much cleaner. It might make sense to add a routes function to the LaravelLocalization class like this, instead of adding a new function to Laravel’s Route class.

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

I liked the idea of having it on the Route class because it could be chained by methods like ->middleware(...).
I haven't dug in too deep, so I'm not entirely sure if that's even possible.

A key downside of the proposed structure is its impact on named routes. The current system simplifies localization because route('my.route') automatically generates the correct localized URL, avoiding the need for LaravelLocalization::localizeUrl('/test'). This is particularly valuable when combined with tools like Ziggy, which enable JavaScript to access named routes directly. Losing this functionality would require significant refactoring for users and could discourage adoption of the new structure.

Good point, I didn't really think about this. Ideally, you should still be able to use the regular route helper to generate URL's for the current locale. Seems like Laravel has a solution for this, which is also supported by Ziggy.
https://laravel.com/docs/11.x/urls#default-values
This is a solution in the form of route parameters, which is different from the prefixes used currently.

Creating custom route helpers for both PHP and JavaScript to dynamically generate the correct localized URLs.

This already exists through methods like getLocalizedURL or getURLFromRouteNameTranslated for PHP.
I would avoid shipping custom helpers for JS. It's currently not the case and seems quite the burden to maintain.

Force All Users to Migrate
This approach would require all users upgrading to version 3.x to update their routes.php files and replace all uses of the route helper in their PHP and JavaScript files. While this is the easiest option for maintainers, it imposes the heaviest burden on users, which could discourage adoption.

Separate Repository
Another option is to create a new repository for the revised structure. This would allow developers to explicitly choose the localization strategy that suits their app. While this reduces confusion and avoids forcing all users into a single paradigm, maintaining two repositories increases the maintenance burden. Alternatively, the current repository could be marked as deprecated, with only minimal updates (e.g., bumping Laravel versions) moving forward.

Leaving developers with multiple options to chose from doesn't seem like a good idea to me.
Look at Laravel's current state of different auth options like Laravel Breeze, Laravel Jetstream, and Laravel Fortify.
It takes some time to understand what different options they provide and which one to pick.
I feel like this package should ship a single solution, without users having to decide on which package they should pick.
In case we go through with this, the migration path should be documented very clearly.

That being said, I wouldn't mind having a go at this and sharing my thoughts along the way.
I've created a new 3.x branch and will create a PR as soon as I have something that works.
I'll let you know when that's the case!

@iwasherefirst2
Copy link
Collaborator

iwasherefirst2 commented Dec 3, 2024

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:

Route::group([
    'prefix' => '{locale?}', // Make the locale optional
    'middleware' => ['setLocale', 'localeSessionRedirect'], // Add your middleware
    'where' => ['locale' => 'en|de|fr'] // Restrict locale to specific values
], function () {
    // Localized routes go here
});

With this setup:

  • The {locale} parameter is optional, which aligns well with the hideDefaultLocaleInURL feature.
  • The setLocale middleware ensures the application uses the appropriate locale, either from the parameter or a fallback default.
  • This approach eliminates the need to manage route name prefixes, fixing many of the challenges mentioned in this issue.
  • The where clause validates the locale parameter to ensure only allowed languages are handled.
  • The helper getLocalizedURL or getURLFromRouteNameTranslated could be replaced by using route together with locale paramter

I’d be happy to experiment with this approach further and explore its implications. Looks like a clean solution.
Should I proceed with a POC implementation to test this out, or are you planning to work on it? Let me know how we can best collaborate on this!

@iwasherefirst2
Copy link
Collaborator

iwasherefirst2 commented Dec 14, 2024

@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 '{locale?} didn’t work because a parameter in a URL can only be optional if it’s the last part of the URL.

So now, I’ve created a route macro that generates each route twice (if hideDefaultLocaleInUrl is enabled). The middleware essentially performs the LaravelLocalized::setLocale(..) logic, and in it, I modify the currentLang value of LaravelLocalized. I moved the localesMapping into a separate middleware, I hope this works as its supposed to, I am not 100% sure about its use-cases.

What I’ve done so far:

  • Created middleware for setLocale
  • Removed setLocale from the LaravelLocalisation class
  • Removed custom cache logic
  • Created custom middleware for LocaleMapping and decoupled it from setLocale
  • Updated the README up to the 'Usage' section

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:

  • Update the entire README
  • Update tests
  • Create a Docker image to run the tests locally (would be very helpful)
  • Make the URL parameter name {locale} configurable via the config file
  • Perform smoke tests for each feature
  • Create a helper routeLocalized that adds the default_lang. prefix for the default language when hideDefaultLocaleInURL is enabled
  • Double-check the use cases for localesMapping and verify if my middleware is sufficient
  • Use default values for locale, should be set in middleware (https://laravel.com/docs/11.x/urls#default-values)
  • Integrate translated route feature

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 segment(1) since we can directly fetch the locale from the URL via the parameter.

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.

@iwasherefirst2 iwasherefirst2 pinned this issue Dec 14, 2024
@iwasherefirst2 iwasherefirst2 self-assigned this Dec 14, 2024
@iwasherefirst2
Copy link
Collaborator

The routes will look like this if hideDefaultLocaleInURL is enabled

GET|HEAD      about-mcamara ..................................................................
GET|HEAD   {locale}/about-mcamara ...................................................

And if hideDefaultLocaleInURL is set to false they look like this

GET|HEAD   {locale}/about-mcamara ...................................................

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

No branches or pull requests

2 participants