Framework: Serialization of 'Closure' is not allowed on Route group and resources

[webmake]

• Laravel Version: 5.2.45
• PHP Version: 5.6.27

Description:

I got very strange case (THIS IS NOT related with https://github.com/laravel/framework/issues/7319), I don't use closures, but still got error:
develop.ERROR: exception 'Exception' with message 'Serialization of 'Closure' is not allowed' in /vagrant/laravel/vendor/laravel/framework/src/Illuminate/Foundation/Console/RouteCacheCommand.php:95

After some hours of debugging I noticed, seems $route->prepareForSerialization(); somehow failing, that cannot unset $this->router, $this->container parameters within route object.
More strange, that code below works perfectly: routes.php:

Route::group(['prefix' => 'api'], function () {
    Route::group(['middleware' => ['role:api']], function () {
        Route::get('/users', 'ApiUsersController@index');
    });
});

$routes = app('router')->getRoutes();
foreach ($routes as $route) {
    $route->prepareForSerialization(); // magical line
}
serialize($routes); // no error

But if I runphp artisan route:cache
and comment magical line - doesn't work and gives error.

One more detail that I noticed:
serialize($routes->getByAction('App\Http\Controllers\ApiUsersController@index'));
this fails, don't neccesary to serialize whole object (serialize($routes) ). I guess pointer to router object somewhere got lost, because actionList is not unset from router and container parameters.

Currently I cannot give more details, but if anyone is familiar how routing cache works, please guide me.
Thanks

[webmake]

Finally found peace of code, which causes such error (I guess when url and method is the same):

Route::group(['prefix' => 'api'], function () {
    Route::get('/users', 'ApiUsersController@index');
});

Route::group(['prefix' => 'api'], function () {
    Route::get('/users', 'Modules\ApiUsersController@index');
});

Please qualify, is it bug of library that gives not proper message, or bug, that cannot handle such case, because having multiple modules, when error occurs can't even determine error place.

[sisve]

The error message could perhaps be improved, but isn't the issue that you have duplicate routes? How is that expected to work?

I can reproduce this in 5.1.45, it is not limited to unsupported 5.2

[error] Exception: Serialization of 'Closure' is not allowed in ~/vendor/laravel/framework/src/Illuminate/Foundation/Console/RouteCacheCommand.php:95
Stack trace:
#0 ~/vendor/laravel/framework/src/Illuminate/Foundation/Console/RouteCacheCommand.php(95): serialize(Object(Illuminate\Routing\RouteCollection))
#1 ~/vendor/laravel/framework/src/Illuminate/Foundation/Console/RouteCacheCommand.php(65): Illuminate\Foundation\Console\RouteCacheCommand->buildRouteCacheFile(Object(Illuminate\Routing\RouteCollection))
#2 [internal function]: Illuminate\Foundation\Console\RouteCacheCommand->fire()

[sisve]

I've reduced the reproduction further. This happens if you have 1) two routes for same method+url, _and_ 2) at least one of the controllers is in a namespace, _and_ 2) they are not in the same namespace.

Route::get('/', 'Alfa\\NonExisting@index');
Route::get('/', 'Beta\\NonExisting@index');

This is reproducible in Laravel 5.1.45.

[webmake]

Well, I think validation before generating urls should prevent from such errors, because having huge application hard to track if url is in use, so it might redeclare controller for the same route even without deeper knowledge. I admit that was my mistake to declare twice, but message is really unhelpful to track where it fails for real

[webmake]

My proposal would be:
--- src/Illuminate/Routing/RouteCollection.php
+++ src/Illuminate/Routing/RouteCollection.php

    protected function addToCollections($route)
    {
        $domainAndUri = $route->domain().$route->getUri();

        foreach ($route->methods() as $method) {
+            if (isset($this->routes[$method][$domainAndUri])) {
+                throw new DuplicateKeyException('Method: ' . $method . ' route: ' . $domainAndUri);
+            }
            $this->routes[$method][$domainAndUri] = $route;
        }

        $this->allRoutes[$method.$domainAndUri] = $route;
    }

Does anyone agree with me? :)

[marky291]

I don't see a reason to bloat code and having this run every time you call routes.php.

It isn't that hard to see duplicate urls to be honest.

[webmake]

Well, reason is that exception doesn't refer to the real problem. Even more I didn't find any info in laravel docs, that duplicated url might cause not generated cache at all due this error. It cost a lot time to detect real place that causes such exception, having prefixed urls in different modules, etc. so I think additional one line checking isn't so much cost to prevent from future problems. It should inform at very begining of developing that something wrong, not at that time when trying to enable cache.

[marky291]

Maybe a check to see if application is running artisan route cache.

You should create a pull request and it will be either accepted or denied, more progressive.

[webmake]

Seems solved with https://github.com/laravel/framework/pull/18475 with route overload, not with suggested exception, but well done