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.5] Add extension point for custom ControllerDispatcher #20031

Merged
merged 3 commits into from
Jul 13, 2017
Merged

[5.5] Add extension point for custom ControllerDispatcher #20031

merged 3 commits into from
Jul 13, 2017

Conversation

sisve
Copy link
Contributor

@sisve sisve commented Jul 13, 2017

This introduces an extension point to write a custom dispatcher. This allows me to write a custom resolving logic that can use functionality not originally available in Laravel; like handling of nullable type hints (PHP 7.1) and optional route parameters. Laravel currently calls app::make on those parameters, even if they are neither bound, nor buildable

Changes

  1. Added Illuminate\Contracts\Routing\ControllerDispatcher
  2. Changed ControllerDispatcher::getMiddleware from static to an instance method.
  3. Added Route::controllerDispatcher() that resolves the dispatcher if bound, and falls back to the original if a dispatcher isn't bound.

Point 1 and 2 are breaking changes if someone has already replaced their dispatcher by replacing the Route (hardcoded new ControllerDispatcher in runController) and the Router (hardcoded new Route in newRoute).

Point 2 isn't necessary for my use-case, but seemed to be within the scope of the ControllerDispatcher contract.

@@ -54,7 +55,7 @@ public function dispatch(Route $route, $controller, $method)
* @param string $method
* @return array
*/
public static function getMiddleware($controller, $method)
public function getMiddleware($controller, $method)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will make it a breaking change, should probably target 5.5 instead

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appears to have been changed to laravel:master

@sisve sisve changed the base branch from 5.4 to master July 13, 2017 10:37
@sisve sisve changed the title [5.4] Add extension point for custom ControllerDispatcher [5.5] Add extension point for custom ControllerDispatcher Jul 13, 2017
* @param string $method
* @return mixed
*/
public function dispatch(Route $route, $controller, $method);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is slightly unfortunate as typically a contract can't depend on any outside library. This depends on Illuminate\Routing.

@taylorotwell taylorotwell merged commit ae87e2d into laravel:master Jul 13, 2017
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.

None yet

4 participants