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

[11.x] Fix the error with the prefix in non-cached routes #51176

Closed
wants to merge 2 commits into from

Conversation

AtmosphereMao
Copy link

Fixes : #50239

The getPrefix() method of the Illuminate\Routing\Route returns different values for non-cached and cached routes:

/{locale} - the return value of the getPrefix() method for a non-cached route
{locale} - the return value of the getPrefix() method for a cached route

This PR fixes this issue by modifying the formatPrefix function in the RouteGroup class when initializing the route model

image

AtmosphereMao and others added 2 commits April 22, 2024 23:58
laravel#50239 
Fix:The getPrefix() method of the Illuminate\Routing\Route returns different values for non-cached and cached routes:

/{locale} - the return value of the getPrefix() method for a non-cached route
{locale} - the return value of the getPrefix() method for a cached route
@taylorotwell
Copy link
Member

Do we have a way to write a test for this?

@AtmosphereMao
Copy link
Author

Do we have a way to write a test for this?

Thank you for your replay, I don't have a better testing method. I observed these Route models by traversing them and found that when their Prefix property is initialized, the first initialization of $old = $old['prefix'] ?? ''; will get an empty string, which causes a single Prefix Route to have an extra '/' added. If a new Prefix is added, the extra '/' will be removed through trim()

Class RouteGroup:

protected static function formatPrefix($new, $old, $prependExistingPrefix = true)
{
    $old = $old['prefix'] ?? '';

    if ($prependExistingPrefix) {
        dump("old: " . $old);
        
        dump("original: " . (isset($new['prefix']) ? trim($old, '/').'/'.trim($new['prefix'], '/') : $old));

        dump("new: " . (isset($new['prefix'])
            ? ($old ? trim($old, '/') . '/' . trim($new['prefix'], '/') : trim($new['prefix'], '/'))
            : $old));

        return isset($new['prefix'])
            ? ($old ? trim($old, '/') . '/' . trim($new['prefix'], '/') : trim($new['prefix'], '/'))
            : $old;
    }

    return isset($new['prefix']) ? trim($new['prefix'], '/').'/'.trim($old, '/') : $old;
}

My Routing Definition:

Route::group([
    'prefix' => '{locale}',
    'where' => ['locale' => 'en|fr|de'],
], function () {
    Route::get('/', function () {
        return view('welcome');
    });
});

Initialize routing:
image

Tinker Testing:

$routes = app('router')->getRoutes()->get('GET');  
foreach ($routes as $route) {                                                                                                                                                                                                     
    dump($route->getPrefix());
} 

image

@driesvints driesvints changed the title [11.x] Fix the error with the prefix in non-cached routes. [11.x] Fix the error with the prefix in non-cached routes Apr 23, 2024
@taylorotwell
Copy link
Member

This seems to do the opposite of what we want. It makes non-cached routes not prefixed. TBH I'm not sure any of this is even worth fixing.

@AtmosphereMao AtmosphereMao deleted the patch-1 branch April 23, 2024 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Different getPrefix() behavior for cached and non-cached routes
2 participants