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

[5.4] Fix overwriting of routes #18475

Merged
merged 4 commits into from
Mar 25, 2017
Merged

[5.4] Fix overwriting of routes #18475

merged 4 commits into from
Mar 25, 2017

Conversation

josiasmontag
Copy link
Contributor

Background

Currently, two routes with the same path and method are breaking the route cache. A routes file containing the following routes

Route::get('home', 'HomeController@index');
Route::get('home', 'NewHomeController@index');

will break the artisan route:cache command with the following exception:

exception 'Exception' with message 'Serialization of 'Closure' is not allowed' in ..\vendor\laravel\framework\src\Illuminate\Foundation\Console\RouteCacheCommand.php:95

This is a problem if you want to overwrite routes from a vendor package (for example Spark). Also see: #7452

The underlaying problem

Adding the second route to the RouteCollection overwrites the first route of $routes and $allRoutes. However, the old route is still in the action look-up table $actionList (and in $nameList, if the route has a name).
The RouteCacheCommand iterates over $routes of RouteCollection and serializes all routes. It cannot serialize the first, old route because it is not in $routes anymore. This causes the exception.

Solution

The clean solution is to to keep the $actionList and $nameList always up to date and avoid keeping outdated routes. There is already a method refreshNameLookups() to refresh the name look-up table. I added the same method for the action look-up table (refreshActionLookups()). I changed the RouteCacheCommand to use these methods.

@taylorotwell
Copy link
Member

I still get the same error even with this patch applied.

@josiasmontag
Copy link
Contributor Author

I just did the following to verify:

  • Checked out current laravel/laravel repository (master branch)
  • Cleared routes/web.php and routes/api.php (The example routes are using closures, which are not supported by route caching)
  • Added the two example routes to routes/web.php
  • Run artisan route:cache
  • It fails with the mentioned exception
  • Apply my patch
  • Run artisan route:cache
  • It works

@taylorotwell
Copy link
Member

OK sorry I had the api.php file still in place. However, caching works for me even without your patch right now?

@taylorotwell taylorotwell merged commit 81b46d8 into laravel:5.4 Mar 25, 2017
@josiasmontag josiasmontag deleted the fix-route-overwrite branch March 25, 2017 00:44
@jordonbaade
Copy link

Applied this to my issue in the Spark repo about overriding routes, I tested the patch and it works with my team's route file now. Looking forward to this in 5.4.17! Thanks @josiasmontag & @taylorotwell

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.

3 participants