search

codezero-be / laravel-localized-routes

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

drop middlewarePriority requirement #100

Closed stevethomas closed 7 months ago

stevethomas commented 1 year ago

Pretty trivial, feel free to close - but this simplifies setup and removes the need to keep $middlewarePriority in sync with Illuminate\Foundation\Http\Kernel.

I don't think the middlewarePriority is required for most use cases, because if your web group looks like https://github.com/laravel/laravel/blob/10.x/app/Http/Kernel.php#L38, you can just add the package middleware somewhere above SubstituteBindings and that will enforce the correct order, without needing to also set the priority.

ivanvermeyen commented 1 year ago

Hi,

Thank you for bringing this to my attention. It would be great to remove this extra setup step. I would make the previous step a bit more explicit though, as adding it between StartSession and SubstituteBindings is still required.

I hope I can add tests for this soon. I'm not sure it always worked like this.

ivanvermeyen commented 7 months ago

Thank you again for your PR. I did some manual testing with Laravel 10 and like you said, the middleware priority step is not needed. You do however have to add the middleware before SubstituteBinding if you have localized route keys/slugs.

But because Laravel 11 changes the Middleware setup by removing Kernel.php, I had to rewite the instructions entirely.