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

Add ability to disable controller middleware #53350

Closed

Conversation

ollieread
Copy link
Contributor

@ollieread ollieread commented Oct 30, 2024

This PR adds the ability to disable the collection of controller middleware.

The main reason behind this is to prevent the controller's constructor from being run before the route is being processed. Primarily for dependency-injection purposes.

This implementation is non-invasive and comes in two parts.

Middleware Config/Builder

When building the application controller, middleware can be disabled for the entire application.

    ->withMiddleware(function (Middleware $middleware) {
        $middleware->withoutControllerMiddleware();
    });

It can also be re-enabled.

    ->withMiddleware(function (Middleware $middleware) {
        $middleware->withControllerMiddleware();
    });

Note

As a side effect of this, the default HTTP Kernel implementation now has a Kernel::getRouter() method that serves as a getter for Kernel::$router.

Router

You can also disable or enable controller middleware directly on the router.

app('router')->skipControllerMiddleware();

And re-enable.

app('router')->allowControllerMiddleware();

Side Effect/Possible Bug-fix

Some tests failed the first time around, and I discovered it's because the OPTIONS routes that are automatically created are created without access to the router. This means that Route::$router is null, which doesn't seem to be allowed by its docblock.

To fix this, I moved CompiledRouteCollection::setRouter() to AbstractRouteCollection, and had the RouteCollection class call $this->router->newRoute() instead of manually creating a new instance of Route.

I suspect this was a side effect bug that hadn't been caught yet, as it's quite obscure,

@taylorotwell
Copy link
Member

I don't think instantiating the controller is required anymore in L11: https://laravel.com/docs/11.x/controllers#controller-middleware

@ollieread
Copy link
Contributor Author

I don't think instantiating the controller is required anymore in L11: laravel.com/docs/11.x/controllers#controller-middleware

Omg, you're absolutely correct. Sorry, I'd completely missed that!

@ollieread ollieread deleted the feat/disable-controller-middleware branch October 31, 2024 20:04
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.

2 participants