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

Separate routing from dispatching #2288

Merged
merged 4 commits into from Aug 17, 2017

Conversation

Projects
None yet
4 participants
@akrabat
Copy link
Member

commented Aug 13, 2017

Move the routing to middleware so that the user can add it wherever they want it to be within the middleware stack. This gives us the new capability to allow for some middleware to run before routing and some after.

If the Routing middleware isn't added by the user, then automatically call it in App::__invoke() as a last resort.

Also, remove determineRouteBeforeAppMiddleware setting as it's no longer necessary.

@akrabat akrabat requested review from geggleto and silentworks Aug 13, 2017

@akrabat akrabat added the Slim 4 label Aug 13, 2017

@akrabat akrabat force-pushed the akrabat:separate-routing-from-rendering branch from e7f909f to a170f2a Aug 13, 2017

@coveralls

This comment has been minimized.

Copy link

commented Aug 13, 2017

Coverage Status

Coverage increased (+0.03%) to 93.054% when pulling a170f2a on akrabat:separate-routing-from-rendering into ee32eb6 on slimphp:4.x.

@coveralls

This comment has been minimized.

Copy link

commented Aug 13, 2017

Coverage Status

Coverage increased (+0.03%) to 93.054% when pulling a170f2a on akrabat:separate-routing-from-rendering into ee32eb6 on slimphp:4.x.

akrabat added a commit to akrabat/Slim that referenced this pull request Aug 13, 2017

Remove Container
Container is now completely optional. App now creates its own request
and response in `run()` and the rest of the objects in
`DefaultServicesProvider` were already defined within `App`.

Also removed the magic `__call` method on `App` that allows access to
the container as there's no need for it.

Note that Pimple is a dev dependency for testing routing,
`CallableResolver` and `DeferredCallable`.

AppTest has also been tidied a little.

Closes slimphp#2288
@geggleto
Copy link
Contributor

left a comment

Also will this not only affect Slim currently when you use the invoke method. We would need to make more changes to affect run?

/**
* Perform routing and store matched route to the request's attributes
*/
class Routing

This comment has been minimized.

Copy link
@geggleto

geggleto Aug 14, 2017

Contributor

We need a new name for this.

This comment has been minimized.

Copy link
@akrabat

akrabat Aug 14, 2017

Author Member

Any preference? RoutingMiddleware ?

Though if we postfix with Middleware, we'll have to change the other Middleware classes too.

This comment has been minimized.

Copy link
@geggleto

geggleto Aug 14, 2017

Contributor

I would prefer to have everything suffixed ...

This comment has been minimized.

Copy link
@akrabat

akrabat Aug 15, 2017

Author Member

Renamed to RoutingMiddleware

* @param ServerRequestInterface $request PSR7 server request
* @return ServerRequestInterface
*/
public function doRouting(ServerRequestInterface $request)

This comment has been minimized.

Copy link
@geggleto

geggleto Aug 14, 2017

Contributor

.............. 👎 you can... doBetter

This comment has been minimized.

Copy link
@silentworks

silentworks Aug 14, 2017

Member

Whats wrong with this?

This comment has been minimized.

Copy link
@geggleto

geggleto Aug 14, 2017

Contributor

doRouting ... surely we can come up with a better name :P

This comment has been minimized.

Copy link
@akrabat

akrabat Aug 14, 2017

Author Member

performRouting() ?

This comment has been minimized.

Copy link
@geggleto

geggleto Aug 14, 2017

Contributor

dispatchRequest ?

This comment has been minimized.

Copy link
@akrabat

akrabat Aug 15, 2017

Author Member

Can't be dispatchRequest as this code doesn't dispatch - it only performs the routing work and then we dispatch later.

This comment has been minimized.

Copy link
@silentworks

silentworks Aug 17, 2017

Member

Maybe prepare or perform then?

@coveralls

This comment has been minimized.

Copy link

commented Aug 15, 2017

Coverage Status

Coverage increased (+0.03%) to 93.054% when pulling 68d4ba6 on akrabat:separate-routing-from-rendering into ee32eb6 on slimphp:4.x.

@akrabat akrabat force-pushed the akrabat:separate-routing-from-rendering branch from 68d4ba6 to eb71f27 Aug 17, 2017

@akrabat

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2017

Rebased

@coveralls

This comment has been minimized.

Copy link

commented Aug 17, 2017

Coverage Status

Coverage increased (+0.03%) to 93.054% when pulling eb71f27 on akrabat:separate-routing-from-rendering into a60a712 on slimphp:4.x.

@akrabat

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2017

Renamed doRouting() to performRouting()

@coveralls

This comment has been minimized.

Copy link

commented Aug 17, 2017

Coverage Status

Coverage increased (+0.03%) to 93.054% when pulling 058d684 on akrabat:separate-routing-from-rendering into a60a712 on slimphp:4.x.

akrabat added some commits Aug 13, 2017

Separate routing from dispatching
Move the routing to middleware so that the user can add it wherever they
want it to be within the middleware stack. This gives us the new
capability to allow for some middleware to run before routing and some
after.

If the `Routing` middleware isn't added by the user, then automatically
call it in `App::__invoke()` as a last resort.

Also, remove `determineRouteBeforeAppMiddleware` setting as it's no
longer necessary.
Rename doRouting to performRouting
Also update comment to be clearer.

@akrabat akrabat force-pushed the akrabat:separate-routing-from-rendering branch from 058d684 to 7ff1c47 Aug 17, 2017

@coveralls

This comment has been minimized.

Copy link

commented Aug 17, 2017

Coverage Status

Coverage increased (+0.03%) to 93.035% when pulling 7ff1c47 on akrabat:separate-routing-from-rendering into 2cac1ac on slimphp:4.x.

@coveralls

This comment has been minimized.

Copy link

commented Aug 17, 2017

Coverage Status

Coverage increased (+0.03%) to 93.035% when pulling 0c3eb5d on akrabat:separate-routing-from-rendering into 2cac1ac on slimphp:4.x.

akrabat added a commit to akrabat/Slim that referenced this pull request Aug 17, 2017

@akrabat akrabat merged commit 0c3eb5d into slimphp:4.x Aug 17, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 93.035%
Details

akrabat added a commit to akrabat/Slim that referenced this pull request Nov 3, 2017

Remove Container
Container is now completely optional. App now creates its own request
and response in `run()` and the rest of the objects in
`DefaultServicesProvider` were already defined within `App`.

Also removed the magic `__call` method on `App` that allows access to
the container as there's no need for it.

Note that Pimple is a dev dependency for testing routing,
`CallableResolver` and `DeferredCallable`.

AppTest has also been tidied a little.

Closes slimphp#2288

@akrabat akrabat added this to the 4.0 milestone Nov 26, 2017

@akrabat akrabat deleted the akrabat:separate-routing-from-rendering branch Apr 18, 2019

@l0gicgate l0gicgate referenced this pull request Apr 25, 2019

Merged

Slim 4 Alpha Release #2665

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.