codezero-be / laravel-localized-routes

⭐️ A convenient way to set up and use localized routes in a Laravel app.
MIT License
505 stars 46 forks source link

Laravel 6 issue #13

Closed dentom closed 4 years ago

dentom commented 5 years ago

Hi, first of all thank you for making this, it's a nice little time saver for i18n apps and websites.

There's one small issue when using this in Laravel 6. Routes with model bindings (when using slugs instead of IDs) don't work out of the box.

For example let's say your default locale is set to "en" and you have this route: Route::get(Lang::uri('articles/{article}'), 'ArticlesController@show')

Now this is what happens: en/articles/en-slug - works de/artikel/de-slug - doesn't work de/artikel/en-slug - works

This happens because \Illuminate\Routing\Middleware\SubstituteBindings::class middleware is executed before your middleware that sets the locale.

In order to make it work properly you have to add \CodeZero\LocalizedRoutes\Middleware\LocalizedRouteLocaleHandler::class to $middlewarePriority array inside Http/Kernel.php and place it before \Illuminate\Routing\Middleware\SubstituteBindings::class

    protected $middlewarePriority = [
...
        \CodeZero\LocalizedRoutes\Middleware\LocalizedRouteLocaleHandler::class,
        \Illuminate\Routing\Middleware\SubstituteBindings::class,
...
    ];

Hope this helps someone :)

ivanvermeyen commented 5 years ago

Thanks for reporting this! 👍

I'll take not to put this in the readme and perhaps review the current middleware feature. Maybe it makes sense to always run the middleware instead of enabling it per route anyway.

dentom commented 5 years ago

Thanks for reporting this! 👍

I'll take not to put this in the readme and perhaps review the current middleware feature. Maybe it makes sense to always run the middleware instead of enabling it per route anyway.

Just to be clear, this happens even with 'use_locale_middleware' => true. Your middleware runs, but it runs "too late" for this to work.

ivanvermeyen commented 4 years ago

Ok, I'm working on a few tweaks to this and another package.

I want to make this package responsible for registering routes only. OR Merge this package with my other package that detects and sets the app locale with a middleware. See: https://github.com/codezero-be/laravel-localizer

The only thing I have to do is make it work nicely with this package. So I wrote a new "detector" for that. (not online yet)

If I keep them separate, I have to make the configuration play nicely. I would hate it if I need to define the supported locales multiple times (for each package), so I would somehow like to centralize this. But... You can't call config('other_package') in a config file. You can't put arrays in .env files...

Any thoughts anyone?

ivanvermeyen commented 4 years ago

I wrote a test for the route model binding and updated the README. 👍