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

[kudashevs]

Laravel Version

10.45.1

PHP Version

8.1.27

Database Driver & Version

No response

Description

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 discrepancy in the results for non-cached/cached routes seems a sort of bug to me.

This issue might be related with #43882 and #43997.

Steps To Reproduce

After installing a new Laravel project, just add a route group to the web routes (the /routes/web.php file). For example:

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

Then, use the tinker tool to get the registered routes and check the getPrefix() return values.
Let's check the non-cacned routes:

$routes = app('router')->getRoutes()->get('GET');

foreach ($routes as $route) {
    dump($route->getPrefix());
}

The result is going to be:

Then, we can cache the routes with artisan route:cache command and use tinker again.

$routes = app('router')->getRoutes()->get('GET');

foreach ($routes as $route) {
    dump($route->getPrefix());
}

The result is going to be:

[driesvints]

@kudashevs does the solution for #43932 work if you apply the patch locally? We could maybe try to revive it with proper tests.

[driesvints]

Actually, we also tried this at #44011 but that trims both sides and broke things for some people. So not sure if this one is solvable at all. Although I agree the behaviour should be the same in both cached and none-cached routes.

From a first glance I think the cached route value is the correct one (without the prefixed /) because that's the actual value of "prefix".

[kudashevs]

@kudashevs does the solution for #43932 work if you apply the patch locally? We could maybe try to revive it with proper tests.

@driesvints the patch adds a forward slash to both (cached and non-cached) routes

[kudashevs]

From a first glance I think the cached route value is the correct one (without the prefixed /) because that's the actual value of "prefix".

From the first glance I agree. But, it’s not so obvious.

In the documentation all the route examples with paths have a forward slash. On the other hand, the slash is optional because it is going to be removed during the parsing process. In the community people use both notations (with forward slash and without).

The only prefix example goes without a forward slash. However, due to the logic of the aforementioned route examples it should be expanded to something with a forward slash (which is going to be removed during the parsing process). But the {locale} is not a path. So, it shouldn’t start with a slash to me. This is really complicated :)

So, I think, to move forward we need someone who is in charge of making a final decision on this. I mean, the final decision on the expected behavior (should a route go with a forward slash or without a slash; in which cases a slash should be added). And, I would expect the behavior to be the same for cached and non-cached routes.

P.S. the things are even more complicated because the getPrefix() method has another discrepancy in the behavior for cached and non-cached prefixless routes. If you want, I can brought up this topic too.

[driesvints]

P.S. the things are even more complicated because the getPrefix() method has another discrepancy in the behavior for cached and non-cached prefixless routes. If you want, I can brought up this topic too.

What's that?

[kudashevs]

What's that?

When you create a route without a prefix, the getPrefix() method returns an empty string for a non-cached route and null for the cached route.

Steps To Reproduce

After installing a new Laravel project, just add a route group to the web routes (the /routes/web.php file). For example:

Route::get('/test', function () {
    return view('welcome');
});

Then, use the tinker tool to get the registered routes and check the getPrefix() return values.
Let's check the non-cacned routes:

Then, we can cache the routes with artisan route:cache command and use tinker again.

This discrepancy is not a big deal to me. However, if there is a plan to fix the discrepancy with prefixes, it might make sense to fix this one too.

[kudashevs]

Just in case, for those, who use the getPrefix() method in their code (I use it in a custom middleware). If you want to rely on the getPrefix() return value to make further decisions (I use a comparison for this), you can use these two simple workarounds:

• use routesAreCached() to check whether routes are cached before the comparison:

(app()->routesAreCached())
        ? $route->getPrefix() === '{locale}'
        : $route->getPrefix() === '/{locale}';

• trim the return value before the comparison:

ltrim($route->getPrefix(), '/') === '{locale}';

I would prefer the second one. However, you might want to keep the reason of using the different comparisons.

[kudashevs]

It's been almost three months without any progress.
@driesvints I wonder if I should provide any additional information?

[driesvints]

No. I'm sorry but I just haven't gotten to this yet. Honestly I do not know what the correct path forward is. I think cached routes should mimic what uncached routes do personally. But I don't have the time to work on a solution. If you could work on a PR and send it in then we can go from there. Thanks

[kudashevs]

I also don't know how to approach this issue. What makes the issue even worse is that it seems that this is the default behavior for all the versions (at least I checked 9, 10, 11).

So, I think, to move forward we need someone who is in charge of making a final decision on this. I mean, the final decision on the expected behavior (should a route go with a forward slash or without a slash; in which cases a slash should be added). And, I would expect the behavior to be the same for cached and non-cached routes.

As mentioned earlier, without having the requirements (or at least an explanatory post with a thorough explanation on how the cached and non-cahced routes should behave) approved by Taylor (or someone else who is in charge of making the final decision on the system's behavior) it is going to be a sort of shot in the dark.

[taylorotwell]

I also think cached routes should exactly mimic the behavior of uncached routes. In other words, if cached routes are currently different than uncached, they should be updated to match the uncached behavior.

[kudashevs]

Hello there,

I played around with the issue today, and I didn't find any way to write tests for this issue. What do I mean.

To check whether the cached routes behave in the correct way, they should be cached. The usual way to cache them is to use the artisan command. So, I used the testRouteGrouping method as a basis, but it turned out that you cannot just call an artisan command without a bootstrapped app (that makes sense). So, I thought to go in a different direction and use Orchestra\Testbench\TestCase which provides a bootstrapped app. And it kind of worked with the artisan command, but it doesn't store any cached routes (because the cache path is emulated).

I thought, that mocking a cache path might be an option here. I mocked the getCachedRoutesPath to provide the existing path and it turned out that the artisan command reads a list of routes from a route file. But, the route file doesn't exist in the bare framework. So, this pattern for providing routes, which is used in the current test code base,

        $router = $this->getRouter();
        $router->group(['prefix' => 'foo'], function () use ($router) {
            $router->get('bar', function () {
                return 'hello';
            });
        });
        $routes = $router->getRoutes();
        $routes = $routes->getRoutes();

doesn't work in this case. The command just doesn't get any routes.

Have I missed something? Do you have any ideas or suggestions on how the tests could be implemented? Or, do you have any code examples of the tests that work with cached routes (I didn't find any)? Is there a way to cache the routes without the artisan command and without duplicating its functionality? Any idea or advice might be helpful in this case.

[driesvints]

@crynobone would you maybe know how we can test cached routes in framework using test bench?

[github-actions]

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!