fix(laravel): Prevent overwriting existing routes on the router #6941
Conversation
|
I think the best way to go is to define the routes in a separate routes file like most packages and load them in using the SP helpers. This will take care of determining if they are cached yet etc and will cache them if called by the user. https://laravel.com/docs/11.x/packages#routes TLDR; I don't think it's good practice to completely reset all the routes on the router as it does now but allow them to me merged naturally by the framework. |
There was a problem hiding this comment.
I like this it indeed looks better to load routes in a separated file, I'm just wondering why the 404 test is broken. To run tests you can follow instructions at https://github.com/api-platform/core/blob/main/src/Laravel/CONTRIBUTING.md#tests.
Also, please run our cs fixer as it looks like your IDE has a different ruleset then ours which makes the diff larger then it should.
Thanks very much for the patch!
Thanks for the reply! Testing this out in an actual prod environment and I am liking the way it's working. Adding the ability to specify a domain is paramount for many apps. The 404 is because I forgot to add the index where constraint to the route. Will update. I will make sure to lint and format before marking this ready for review. |
jonerickson
force-pushed
the
feature/router-fix
branch
from
d66fd00 to
c75edee
Compare
jonerickson
force-pushed
the
feature/router-fix
branch
from
940f274 to
07ac55f
Compare
|
@soyuka I believe this is ready for review.
|
|
don't worry about commitlint, it takes the first commit of the patch not the PR title but I'll squash anyways and it won't be an issue, awesome patch many thanks @jonerickson ! |
| Route::group(['prefix' => $prefix], function (): void { | ||
| Route::group(['middleware' => ApiPlatformMiddleware::class], function (): void { | ||
| Route::get('/contexts/{shortName?}{_format?}', ContextAction::class) | ||
| ->middleware(ApiPlatformMiddleware::class) |
There was a problem hiding this comment.
Can I safely remove this call as we're in a Route::group call that already has the middleware?
There was a problem hiding this comment.
Yes, as well as the 3 below it as well. thanks for catching that.
Problem
When the API Platform service provider is not auto-discovered, and manually added as a provider, the SP would overwrite any route that had been registered up until that point.
Because of the Eloquent ORM introspection that happens when discovering metadata, a DB service is required to perform the introspection. This happens whenever composer
php artisan package:discoveris run. This is not optimal for CI/CD environments where you often don't have those services running to perform chores like linting and static analysis.Solution
By allowing the developer to add API Platforms SP to composer's
dont-discoverarray and manually registering the SP at a later time, we can prevent the needs of the DB specifically whenphp artisan package:discoveris run. We then register API Platform's SP in our providers array and let Laravel do the rest. Because the SP is now being registered AFTER the defaultRouteServiceProvider, this PR will no longer overwrite the existing routes as it did before.