Use a middleware to protect ignition routes #93
Conversation
…s should be available
| use Closure; | ||
| use Illuminate\Http\Request; | ||
|
|
||
| class IgnitionEnabled |
There was a problem hiding this comment.
Maybe add a test for this middleware, so we're 100% sure it works correctly.
|
|
||
| Route::get('scripts/{script}', ScriptController::class); | ||
| Route::get('styles/{style}', StyleController::class); | ||
| }); |
There was a problem hiding this comment.
Shouldn't routes be registered in boot()? That goes for almost every service in the IoC container - don't resolve them until all other service providers had a chance to modify / extend / swap them.
There was a problem hiding this comment.
Yeah, in our case this would introduce some issues if an exception would occur in a service providers boot method prior to Ignition.
Because our whoops handler would already be registered, but the routes would not. That's why we are registering the routes here too.
There was a problem hiding this comment.
What about $this->app->resolving('router', function () { ... });?
There was a problem hiding this comment.
Marcel and I discussed this. We don't think it is necessary. The router will always have been registered when our provider gets called.
There was a problem hiding this comment.
The point is not so much your provider (although you shouldn't rely on the provider ordering). You also should be a good citizen and allow other packages to swap out the router / extend it in some way, before you access it.
That's at least 50% of the point of the IoC container and contracts, as I understand them. 😉
By always registering our routes we avoid issues like #87
The middleware then takes care of choosing whether or not the route should be reachable.