Slim: Refactor container out of Router

Created on 10 Dec 2016  Â·  12Comments  Â·  Source: slimphp/Slim

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.

Slim 4

Most helpful comment

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.

All 12 comments

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.

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

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

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.

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.

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.

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

Something to look at…

Explicit tasks:

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

WIP PR #2102

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

Closing

Was this page helpful?
0 / 5 - 0 ratings

Related issues

basuke picture basuke  Â·  3Comments

codeguy picture codeguy  Â·  4Comments

codeguy picture codeguy  Â·  4Comments

enygma picture enygma  Â·  3Comments

codeguy picture codeguy  Â·  3Comments