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

[9.x] Fixed RoueGroup::merge to format merged prefixes correctly #44011

Merged
merged 2 commits into from
Sep 5, 2022
Merged

[9.x] Fixed RoueGroup::merge to format merged prefixes correctly #44011

merged 2 commits into from
Sep 5, 2022

Conversation

JirakLu
Copy link
Contributor

@JirakLu JirakLu commented Sep 5, 2022

This PR fixes issue described in #43997

Problem this PR fixes

If route has parent RouteGroup with its prefix not set. FormatPrefix adds slash to either start or end of the actual prefix depending on the situtation.
This problem affects the whole web.php file, because the routes inside are loaded using group with empty $attributes array (so its prefix is empty).

Route::middleware('web')
                ->group(base_path('routes/web.php'));

Examples of the problem

Appending prefix

Route::middleware("auth")->group(function () { // parent RouteGroup prefix is empty
    Route::view("/", "page.test");

    Route::prefix("dashboard")->group(function () { // we add another group with its own prefix
        Route::view("/settings", "page.test");
    });
});

Expected behavior:

When user visits */dashboard/settings the Route->current()->getPrefix() should return dashboard

Actual behavior:

The route prefix is returned with slash at the start -> /dashboard

Prepending prefix

Route::middleware("web")->group(function () { // parent RouteGroup prefix is empty
    Route::prefix("dashboard")->get("/settings", TestController::class); // add route with its own prefix
});

Expected behavior:

When user visits */dashboard/settings the Route->current()->getPrefix() should return dashboard

Actual behavior:

The route prefix is returned with slash at the end -> dashboard/

Quick side note: I don't even know if this syntax is allowed. I didn't found any mentions about prepending route prefixes in the routing documentation, yet there is a test for this exact behavior.

/*
* nested with layer skipped
*/
$router = $this->getRouter();
$router->group(['prefix' => 'foo', 'as' => 'Foo::'], function () use ($router) {
$router->prefix('bar')->get('baz', ['as' => 'baz', function () {
return 'hello';
}]);
});
$routes = $router->getRoutes();
$route = $routes->getByName('Foo::baz');
$this->assertSame('bar/foo', $route->getAction('prefix'));

Here the bar prefix is before the group foo prefix. Event tho its defined inside the group.

Source of the problem

If i understood the problem correctly i think the issue is in the RouteGroup::formatPrefix method.

protected static function formatPrefix($new, $old, $prependExistingPrefix = true)
{
$old = $old['prefix'] ?? '';
if ($prependExistingPrefix) {
return isset($new['prefix']) ? trim($old, '/').'/'.trim($new['prefix'], '/') : $old;
} else {
return isset($new['prefix']) ? trim($new['prefix'], '/').'/'.trim($old, '/') : $old;
}
}

Here the $old variable is set to empty string, when the parent RouteGroup prefix is empty / not set.
And now when the old prefix is empty and new prefix is set.
Appending prefix
This is what happens in the code on line 69 -> "" . "/" . "newPrefix"
There is slash added to the start of the new prefix.
Prepending prefix
This is what happens in the code on line 71 -> "newPrefix" . "/" . ""
There is slash added to the end of the new prefix.

My solution

I managed to fix this issue by wrapping the return from formatPrefix in trim.
Reference:

protected static function formatPrefix($new, $old, $prependExistingPrefix = true)
{
$old = $old['prefix'] ?? '';
if ($prependExistingPrefix) {
return isset($new['prefix']) ? trim($old, '/').'/'.trim($new['prefix'], '/') : $old;
} else {
return isset($new['prefix']) ? trim($new['prefix'], '/').'/'.trim($old, '/') : $old;
}
}

My fix:

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

This should guarantee the prefix is always returned without slash at the start or end as expected.
I also tried running all the tests in the Routing folder after this fix and all of them have passed.

I hope my understanding of the problem is correct and the explanation was clear enough. :)

@taylorotwell taylorotwell merged commit 0fa80d1 into laravel:9.x Sep 5, 2022
driesvints added a commit that referenced this pull request Sep 9, 2022
@GrahamCampbell GrahamCampbell changed the title Fixed RoueGroup::merge to format merged prefixes correctly. [9.x] Fixed RoueGroup::merge to format merged prefixes correctly Nov 6, 2022
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.

RouteGroup::merge adds slash to the route prefix, when parent RouteGroup prefix is empty / not set.
2 participants