laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.49k stars 11.01k forks source link

Optional route parameters doesn't work correctly. #51641

Closed AbdelrhmanSaid closed 5 months ago

AbdelrhmanSaid commented 5 months ago

Laravel Version

11.9.1

PHP Version

8.3.6

Database Driver & Version

No response

Description

When using optional route parameters like {locale?}, there's issues with routes not working correctly. For example, consider this route setup:

Route::prefix('{locale?}')->group(function () {
    Route::get('/login', fn () => 'Login');
});

That's only working if you visit /locale/login and will never work for direct /login.

The problem is due to how Laravel's \Illuminate\Routing\Matching\UriValidator creates patterns for routes. It will create a pattern like {^/(?P<locale>[^/]++)/login$}sDu, which expects a double backslash (\\) for the parameter if it's not provided, causing the route to break.

Steps To Reproduce

  1. Create a new laravel project
  2. In web.php add the following lines
Route::prefix('{locale?}')->group(function () {
    Route::get('/login', fn () => 'Login');
});
  1. Serve your project and try to visit /login, will not work.
driesvints commented 5 months ago

That's not an optional param. It's still a required one because it has a segment coming after it.

jeanmarinho529 commented 5 months ago

To solve this problem, you can simply duplicate the route, as follows:

# route.php

Route::prefix('{locale}')->group(function () {
    Route::get('/login', fn () => 'Login');
});

Route::get('/login', fn () => 'Login');

Alternatively, you can create a middleware and apply it to all routes to set a default locale, or you can use a package that already handles this, such as laravel-localization.

AbdelrhmanSaid commented 5 months ago

I didn't like duplicating the same route definition, so I update the Route::fallback to redirect to the corresponding route, something like that:

<?php

namespace App\Http\Controllers;

use Illuminate\Http\Request;
use Illuminate\Support\Facades\Route;

class FallbackController extends Controller
{
    /**
     * Handle the incoming request.
     */
    public function __invoke(Request $request)
    {
        if (! in_array($request->method(), ['GET', 'HEAD'])) {
            abort(404);
        }

        if (! config('redot.append_locale_to_url') || ! config('redot.redirect_non_locale_urls')) {
            abort(404);
        }

        $locale = app()->getLocale();
        $route = Route::getRoutes()->match(Request::create("/$locale" . $request->getPathInfo(), 'GET'));

        if ($route->isFallback) {
            abort(404);
        }

        return redirect()->to("/$locale" . $request->getPathInfo());
    }
}
jeanmarinho529 commented 5 months ago

Understood, thank you for sharing your solution. I was thinking about using middleware on all routes, and Laravel itself would handle the HTTP status code treatment, somewhat like this:

# SetLocaleMiddleware.php
public function handle(Request $request, Closure $next): Response
{
    $locale = $request->route('locale');

    if (!$locale) {
        return redirect(URL::to(app()->getLocale() . $request->getRequestUri()));
    }

    return $next($request);
}
# Kernel.php
protected $middlewareGroups = [
    'web' => [
        ...
        \App\Http\Middleware\SetLocaleMiddleware::class
    ],
    ...
];
AbdelrhmanSaid commented 5 months ago

You are welcome