chinleung / laravel-multilingual-routes

A package to handle multilingual routes in your Laravel application.
https://github.com/chinleung/laravel-multilingual-routes-demo
MIT License
394 stars 26 forks source link

current_route compatibility with spatie/laravel-translatable and spatie/laravel-sluggable? #70

Open greatislander opened 2 years ago

greatislander commented 2 years ago

Describe the bug

I'm using a model with a translatable name (using spatie/laravel-translatable) which is used as the source for a translatable slug (using the HasTranslatableSlug trait of spatie/laravel-sluggable).

However, a multilingual route for the model does not use the translated slug, but always uses the slug locale which matches the config value for app.locale.

To Reproduce

Steps to reproduce the behavior:

A model instance looks like this in Tinker:

>>> Organization::first()
=> App\Models\Organization {
     id: 1,
     created_at: "2022-05-05 17:35:25",
     updated_at: "2022-05-05 17:37:54",
     name: "{"en":"Canada Revenue Agency","fr":"Agence du revenu du Canada"}",
     slug: "{"en":"canada-revenue-agency","fr":"agence-du-revenu-du-canada"}",
}

In the model class, I specify the route key for the model (as explained in the laravel-sluggable docs):

    /**
     * Get the route key for the model.
     *
     * @return string
     */
    public function getRouteKeyName(): string
    {
        return 'slug';
    }

If I create a GET route and set the app locale to fr, the model view is displayed using the fr slug:

Route::get('/organizations/{organization}', [OrganizationController::class, 'show'])
    ->name('organizations.show');

config('app.locale') == 'fr':

http://laravel.test/organizations/agence-du-revenu-du-canada/

config('app.locale') == 'en':

http://laravel.test/organizations/canada-revenue-agency/

However, if I create a multilingual GET route, the slug uses the locale defined in config('app.locale') is used for both English and French routes:

Route::multilingual('/organizations/{organization}', [OrganizationController::class, 'show'])
    ->name('organizations.show');

config('app.locale') == 'fr':

http://laravel.test/en/organizations/agence-du-revenu-du-canada/ http://laravel.test/fr/organizations/agence-du-revenu-du-canada/

config('app.locale') == 'en':

http://laravel.test/en/organizations/canada-revenue-agency/ http://laravel.test/fr/organizations/canada-revenue-agency/

Expected behavior

A multilingual route should use the appropriately translated slug, if available.

Result of route:list:

  GET|HEAD  en/organizations/{organization} ....................................................................................................... en.organizations.show › OrganizationController@show
  GET|HEAD  fr/organizations/{organization} ....................................................................................................... fr.organizations.show › OrganizationController@show

Additional context

Not applicable.

chinleung commented 2 years ago

Hey @greatislander,

I was on vacation the past weeks. I'll try to reproduce this bug later this week and let you know. 😄

chinleung commented 2 years ago

@greatislander What version of Laravel are you using? Also, can you make a repository with the issue so that I could test it please? :)

greatislander commented 2 years ago

@greatislander What version of Laravel are you using? Also, can you make a repository with the issue so that I could test it please? :)

I'm using Laravel 9— and yes, I can do that. I'll try to set up a repository with a minimal test case tomorrow for you to test out.

greatislander commented 2 years ago

@chinleung Okay, so I found a solution to the main issue while recreating it in this test case: https://github.com/greatislander/translatable-slugs

In my application, \ChinLeung\MultilingualRoutes\DetectRequestLocale::class was not the first entry in the web middleware array. In the documentation it would be helpful to clarify that the DetectRequestLocale middleware needs to be first. I will open a docs PR to add this information.

However, I did detect another issue— the current_route helper does not properly handle a translated slug. You can reproduce this as follows:

  1. Clone the test case repo from https://github.com/greatislander/translatable-slugs
  2. Run the migrations and seed some example Organization models artisan migrate --seed
  3. Navigate to /en/organizations/.
  4. Visit an organization. The URL pattern will be something like /en/organizations/smith-limited.
  5. Click the link to visit the page in French.

Expected

The current_route helper produces a link to the French version, e.g. /fr/organizations/smith-limitee.

Actual

The current_route helper produces a link to the French version using the English slug (/fr/organizations/smith-limited) which does not work.

chinleung commented 2 years ago

@greatislander I was able to reproduce it but I'm not sure if this should be a fix in the package or not.

I've updated the current_route helper like the following:

function current_route(string $locale = null, string $fallback = null, bool $absolute = true): string
{
    if (is_null($fallback)) {
        $fallback = url(request()->server('REQUEST_URI'));
    }

    $route = Route::getCurrentRoute();

    if (! $route) {
        return $fallback;
    }

    $name = Str::replaceFirst(
        locale().'.',
        "{$locale}.",
        $route->getName()
    );

    if (! $name || ! in_array($locale, locales()) || ! Route::has($name)) {
        return $fallback;
    }

    $current = locale();

    locale($locale);

    $url = route($name, array_merge(
        (array) $route->parameters,
        (array) request()->getQueryString()
    ), $absolute);

    locale($current);

    return $url;
}

The issue is that the slug is generated based on the application's current locale. Therefore, I'm changing the locale of the application temporarily to generate the route, and then switch back to the application's previous locale.

Let me think if there's a cleaner way to fix this issue but that could be a workaround for the moment if you want to overwrite the helper.