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.2] Reverse a breaking addition in Route.php #12991

Merged
merged 1 commit into from
Apr 3, 2016
Merged

[5.2] Reverse a breaking addition in Route.php #12991

merged 1 commit into from
Apr 3, 2016

Conversation

themsaid
Copy link
Member

@themsaid themsaid commented Apr 2, 2016

In an earlier PR #12899, controller routes were included in the Route:middleware() method output by calling ControllerDispatcher, this had two effects:

  1. All middlewares registered inside the controller's constructor will be executed twice, once from ControllerDispatcher::callWithinStack() when the controller is actually being dispatched and another in Router::runRouteWithinStack() while the Router is dispatching the request to the route.
  2. All code inside the controller constructor will be executed while the ControllerDispatcher is instantiating the controller, some of this code is considered protected by a middleware registered in routes.php.

For example a code inside the controller constructor that logs a user's presence inside the dashboard area, this code will be executed although the controller is protected under the auth controller for example, or auth.type.admin

Also calls to Request::user() && Auth::user() will return null if the user is not logged in, although the developer added the auth middleware to this controller inside the routes file, this will call a PHP error when Controller dispatches is called instead of executing the auth middleware and redirect the user to the log in screen.


Terminable middlewares are useful, however I say we keep the old behaviour of firing them only when registered otherwhere than the controller for 5.2 in order not to break things.

And I suggest in 5.3 we remove the Controller::middleware() method and make people use a static method for example that we can run from outside to collect the middlewares without executing the constructor code.

And since we will be already collecting controller middlewares data before firing the controller actions, we won't need to read the controller routes from inside the controller itself.

@taylorotwell taylorotwell merged commit c7ffc33 into laravel:5.2 Apr 3, 2016
@themsaid themsaid deleted the reverse-terminiable-middleware-pr branch April 24, 2016 09:52
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