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

Refactor container out of Router #2100

Closed
codeguy opened this issue Dec 10, 2016 · 12 comments
Closed

Refactor container out of Router #2100

codeguy opened this issue Dec 10, 2016 · 12 comments
Assignees
Labels
Milestone

Comments

@codeguy
Copy link
Member

codeguy commented Dec 10, 2016

The router currently needs a reference to the app container only to bind the Route callable to the container if the callable is a \Closure. Each route instance currently needs the container to locate the app's FoundHandler strategy if available, else it falls back to a default RequestResponseStrategy.

We should remove the container from the router and route instance and, instead, inject the FoundHandler strategy into the router during router creation while also adding FoundHandler getter/setter methods on the router should the end user wish to change it during runtime.

In App::map(), we are setting the route's output buffering setting. This should really be encapsulated in the router, imo. We can set this preference when we instantiate the router (discovered from app settings), and the router can assign the output buffering preference to each new route in Router::createRoute().

These changes will completely decouple the app container from router and route instances.

@codeguy codeguy added the Slim 4 label Dec 10, 2016
@codeguy codeguy added this to the 4.0 milestone Dec 10, 2016
@codeguy codeguy self-assigned this Dec 10, 2016
@codeguy
Copy link
Member Author

codeguy commented Dec 10, 2016

We can also add getter/setter methods to the Route instance to control individual route output buffering settings so that the end user can easily chain the method calls during route definition.

$app->get('/foo', 'callable')->setOutputBuffering(true);

Same methods will be used inside of Router instance based on the router's default output buffering preference set during router creation.

@codeguy
Copy link
Member Author

codeguy commented Dec 10, 2016

Another related concern are route groups, which need a resolver and they need to bind their callables to the app instance itself.

@geggleto
Copy link
Member

Output buffering should be part of a view renderer not part of the router

@geekish
Copy link
Contributor

geekish commented Dec 10, 2016

Can't output buffering be handled in a middleware? Users could simply register the middleware on the routes, groups, etc. that you want it on... it'd have to be registered first though I suppose.

@akrabat
Copy link
Member

akrabat commented Dec 10, 2016

There's an argument that we just remove the output buffering thing altogether.

We'll need to ensure that a var_dump() still works though.

@codeguy
Copy link
Member Author

codeguy commented Dec 11, 2016

I'm all for removing output buffering, but I'm not sure how you omit that and allow var_dumping from a route callable at the same time.

@geggleto
Copy link
Member

var_dump et al will also end up causing header problems as well. but there is no way around it ;)

@akrabat
Copy link
Member

akrabat commented Dec 11, 2016

Something to look at…

@codeguy
Copy link
Member Author

codeguy commented Dec 11, 2016

Explicit tasks:

  • Let end-user inject found handler into router instance, else default to RequestResponse strategy
  • Let router inject found handler into each generated route inside of Router::createRoute()
  • Bind route callable to container outside of router
  • Remove container from router and route
  • Make adjustments to route group as necessary

@codeguy
Copy link
Member Author

codeguy commented Dec 11, 2016

WIP PR #2102

@codeguy
Copy link
Member Author

codeguy commented Dec 11, 2016

@akrabat Pretty sure the PR is ready for review. Is there anything I'm missing from the checklist above?

@codeguy
Copy link
Member Author

codeguy commented Mar 8, 2017

Closing

@codeguy codeguy closed this as completed Mar 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants