From 30f22b047702b306ef65bb1e6f4a9fb163efb8e0 Mon Sep 17 00:00:00 2001 From: Josh Lockhart Date: Sun, 11 Dec 2016 11:55:33 -0500 Subject: [PATCH 1/4] WIP on refactoring container out of router --- Slim/App.php | 12 ++------ Slim/Interfaces/RouterInterface.php | 7 +++++ Slim/Route.php | 30 +++++++++++++++++-- Slim/Router.php | 46 ++++++++++++++++++----------- tests/AppTest.php | 2 ++ tests/RouteTest.php | 2 ++ tests/RouterTest.php | 1 - 7 files changed, 69 insertions(+), 31 deletions(-) diff --git a/Slim/App.php b/Slim/App.php index d539ef3c2..0d9fe09b1 100644 --- a/Slim/App.php +++ b/Slim/App.php @@ -177,10 +177,6 @@ public function getRouter() { if (! $this->router instanceof RouterInterface) { $router = new Router(); - $container = $this->getContainer(); - if ($container instanceof ContainerInterface) { - $router->setContainer($container); - } $resolver = $this->getCallableResolver(); if ($resolver instanceof CallableResolverInterface) { $router->setCallableResolver($resolver); @@ -304,13 +300,13 @@ public function any($pattern, $callable) */ public function map(array $methods, $pattern, $callable) { - // TODO: Should we bind route callable to container elsewhere? + // Bind route callable to container, if present if ($this->container instanceof ContainerInterface && $callable instanceof \Closure) { $callable = $callable->bindTo($this->container); } - $router = $this->getRouter(); - $route = $router->map($methods, $pattern, $callable); + // Create route + $route = $this->getRouter()->map($methods, $pattern, $callable); // TODO: Set route output buffering without a container // TODO: Refactor app settings out of container @@ -318,8 +314,6 @@ public function map(array $methods, $pattern, $callable) $route->setOutputBuffering($this->container->get('settings')['outputBuffering']); } - // TODO: Route callable binding and output buffering should be handled within router - return $route; } diff --git a/Slim/Interfaces/RouterInterface.php b/Slim/Interfaces/RouterInterface.php index dd2d86455..d569492d9 100644 --- a/Slim/Interfaces/RouterInterface.php +++ b/Slim/Interfaces/RouterInterface.php @@ -104,4 +104,11 @@ public function relativePathFor($name, array $data = [], array $queryParams = [] * @throws InvalidArgumentException If required data not provided */ public function pathFor($name, array $data = [], array $queryParams = []); + + /** + * Set default route invocation strategy + * + * @param InvocationStrategyInterface $strategy + */ + public function setDefaultInvocationStrategy(InvocationStrategyInterface $strategy); } diff --git a/Slim/Route.php b/Slim/Route.php index 4446c7457..039f3a44f 100644 --- a/Slim/Route.php +++ b/Slim/Route.php @@ -9,12 +9,10 @@ namespace Slim; use Exception; -use Slim\Interfaces\CallableResolverInterface; use Throwable; use InvalidArgumentException; use Psr\Http\Message\ServerRequestInterface; use Psr\Http\Message\ResponseInterface; -use Slim\Exception\SlimException; use Slim\Handlers\Strategies\RequestResponse; use Slim\Interfaces\InvocationStrategyInterface; use Slim\Interfaces\RouteInterface; @@ -65,6 +63,11 @@ class Route extends Routable implements RouteInterface */ protected $outputBuffering = 'append'; + /** + * @var \Slim\Interfaces\InvocationStrategyInterface + */ + protected $routeInvocationStrategy; + /** * Route parameters * @@ -88,6 +91,27 @@ public function __construct($methods, $pattern, $callable, $groups = [], $identi $this->callable = $callable; $this->groups = $groups; $this->identifier = 'route' . $identifier; + $this->routeInvocationStrategy = new RequestResponse(); + } + + /** + * Set route invocation strategy + * + * @param InvocationStrategyInterface $strategy + */ + public function setInvocationStrategy(InvocationStrategyInterface $strategy) + { + $this->routeInvocationStrategy = $strategy; + } + + /** + * Get route invocation strategy + * + * @return InvocationStrategyInterface + */ + public function getInvocationStrategy() + { + return $this->routeInvocationStrategy; } /** @@ -331,7 +355,7 @@ public function __invoke(ServerRequestInterface $request, ResponseInterface $res } /** @var InvocationStrategyInterface $handler */ - $handler = isset($this->container) ? $this->container->get('foundHandler') : new RequestResponse(); + $handler = $this->routeInvocationStrategy; // invoke route callable if ($this->outputBuffering === false) { diff --git a/Slim/Router.php b/Slim/Router.php index 34cd3f307..f47e0d43f 100644 --- a/Slim/Router.php +++ b/Slim/Router.php @@ -9,7 +9,6 @@ namespace Slim; use FastRoute\Dispatcher; -use Interop\Container\ContainerInterface; use InvalidArgumentException; use RuntimeException; use Psr\Http\Message\ServerRequestInterface; @@ -17,6 +16,7 @@ use FastRoute\RouteParser; use FastRoute\RouteParser\Std as StdParser; use Slim\Interfaces\CallableResolverInterface; +use Slim\Interfaces\InvocationStrategyInterface; use Slim\Interfaces\RouteGroupInterface; use Slim\Interfaces\RouterInterface; use Slim\Interfaces\RouteInterface; @@ -31,13 +31,6 @@ */ class Router implements RouterInterface { - /** - * Container Interface - * - * @var ContainerInterface - */ - protected $container; - /** * Parser * @@ -52,6 +45,11 @@ class Router implements RouterInterface */ protected $callableResolver; + /** + * @var \Slim\Interfaces\InvocationStrategyInterface + */ + protected $routeInvocationStrategy; + /** * Base path used in pathFor() * @@ -101,6 +99,26 @@ public function __construct(RouteParser $parser = null) $this->routeParser = $parser ?: new StdParser; } + /** + * Set default route invocation strategy + * + * @param InvocationStrategyInterface $strategy + */ + public function setDefaultInvocationStrategy(InvocationStrategyInterface $strategy) + { + $this->routeInvocationStrategy = $strategy; + } + + /** + * Get default route invocation strategy + * + * @return InvocationStrategyInterface|null + */ + public function getDefaultInvocationStrategy() + { + return $this->routeInvocationStrategy; + } + /** * Set the base path used in pathFor() * @@ -152,14 +170,6 @@ public function setCallableResolver(CallableResolverInterface $resolver) $this->callableResolver = $resolver; } - /** - * @param ContainerInterface $container - */ - public function setContainer(ContainerInterface $container) - { - $this->container = $container; - } - /** * Add route * @@ -227,8 +237,8 @@ protected function createRoute($methods, $pattern, $callable) if ($this->callableResolver) { $route->setCallableResolver($this->callableResolver); } - if ($this->container) { - $route->setContainer($this->container); + if ($this->routeInvocationStrategy) { + $route->setInvocationStrategy($this->routeInvocationStrategy); } return $route; diff --git a/tests/AppTest.php b/tests/AppTest.php index e44fec458..b9445a067 100644 --- a/tests/AppTest.php +++ b/tests/AppTest.php @@ -1119,6 +1119,7 @@ public function testInvokeWithMatchingRouteWithNamedParameterRequestResponseArgS }; $app = new App($c); + $app->getRouter()->setDefaultInvocationStrategy($c['foundHandler']); $app->get('/foo/{name}', function ($req, $res, $name) { return $res->write("Hello {$name}"); }); @@ -1380,6 +1381,7 @@ public function testCurrentRequestAttributesAreNotLostWhenAddingRouteArgumentsRe }; $app = new App($c); + $app->getRouter()->setDefaultInvocationStrategy($c['foundHandler']); $app->get('/foo/{name}', function ($req, $res, $name) { return $res->write($req->getAttribute('one') . $name); }); diff --git a/tests/RouteTest.php b/tests/RouteTest.php index 874156ec6..6839173c3 100644 --- a/tests/RouteTest.php +++ b/tests/RouteTest.php @@ -412,6 +412,7 @@ public function testInvokeDeferredCallable() $route = new Route(['GET'], '/', 'CallableTest:toCall'); $route->setContainer($container); $route->setCallableResolver($resolver); + $route->setInvocationStrategy($container['foundHandler']); $uri = Uri::createFromString('https://example.com:80'); $body = new Body(fopen('php://temp', 'r+')); @@ -448,6 +449,7 @@ public function testChangingCallable() $route = new Route(['GET'], '/', 'CallableTest:toCall'); //Note that this doesn't actually exist $route->setContainer($container); $route->setCallableResolver($resolver); + $route->setInvocationStrategy($container['foundHandler']); $route->setCallable('CallableTest2:toCall'); //Then we fix it here. diff --git a/tests/RouterTest.php b/tests/RouterTest.php index b34339f1e..c43ab51b8 100644 --- a/tests/RouterTest.php +++ b/tests/RouterTest.php @@ -8,7 +8,6 @@ */ namespace Slim\Tests; -use Slim\Container; use Slim\Router; class RouterTest extends \PHPUnit_Framework_TestCase From b267f24ed73d266fed883aa41da6debe0c274d14 Mon Sep 17 00:00:00 2001 From: Josh Lockhart Date: Sun, 11 Dec 2016 12:05:34 -0500 Subject: [PATCH 2/4] Remove container from Routable abstract class --- Slim/App.php | 4 ++-- Slim/Routable.php | 6 +++--- tests/RouteTest.php | 7 ++----- 3 files changed, 7 insertions(+), 10 deletions(-) diff --git a/Slim/App.php b/Slim/App.php index 0d9fe09b1..06f9de1ac 100644 --- a/Slim/App.php +++ b/Slim/App.php @@ -335,9 +335,9 @@ public function group($pattern, $callable) $router = $this->getRouter(); $group = $router->pushGroup($pattern, $callable); // TODO: Set group container and resolver inside Router - if ($this->container instanceof ContainerInterface) { + /*if ($this->container instanceof ContainerInterface) { $group->setContainer($this->container); - } + }*/ if ($this->callableResolver instanceof CallableResolverInterface) { $group->setCallableResolver($this->callableResolver); } diff --git a/Slim/Routable.php b/Slim/Routable.php index 153d8e333..2ede4d892 100644 --- a/Slim/Routable.php +++ b/Slim/Routable.php @@ -36,7 +36,7 @@ abstract class Routable * * @var ContainerInterface */ - protected $container; + //protected $container; /** * Route middleware @@ -99,11 +99,11 @@ public function getCallableResolver() * * @return self */ - public function setContainer(ContainerInterface $container) + /*public function setContainer(ContainerInterface $container) { $this->container = $container; return $this; - } + }*/ /** * Prepend middleware to the middleware collection diff --git a/tests/RouteTest.php b/tests/RouteTest.php index 6839173c3..13141d5c7 100644 --- a/tests/RouteTest.php +++ b/tests/RouteTest.php @@ -192,8 +192,8 @@ public function testAddMiddlewareAsString() $container = new Container(); $container['MiddlewareStub'] = new MiddlewareStub(); - - $route->setContainer($container); + $resolver = new CallableResolver($container); + $route->setCallableResolver($resolver); $route->add('MiddlewareStub:run'); $env = Environment::mock(); @@ -223,7 +223,6 @@ public function testControllerInContainer() $deferred = new DeferredCallable('CallableTest:toCall', $resolver); $route = new Route(['GET'], '/', $deferred); - $route->setContainer($container); $route->setCallableResolver($resolver); $uri = Uri::createFromString('https://example.com:80'); @@ -410,7 +409,6 @@ public function testInvokeDeferredCallable() }; $route = new Route(['GET'], '/', 'CallableTest:toCall'); - $route->setContainer($container); $route->setCallableResolver($resolver); $route->setInvocationStrategy($container['foundHandler']); @@ -447,7 +445,6 @@ public function testChangingCallable() }; $route = new Route(['GET'], '/', 'CallableTest:toCall'); //Note that this doesn't actually exist - $route->setContainer($container); $route->setCallableResolver($resolver); $route->setInvocationStrategy($container['foundHandler']); From 602f0969d219e213c733c8011115245400b331d7 Mon Sep 17 00:00:00 2001 From: Josh Lockhart Date: Sun, 11 Dec 2016 12:07:41 -0500 Subject: [PATCH 3/4] Remove old code --- Slim/App.php | 4 ---- Slim/Routable.php | 21 --------------------- 2 files changed, 25 deletions(-) diff --git a/Slim/App.php b/Slim/App.php index 06f9de1ac..46971d523 100644 --- a/Slim/App.php +++ b/Slim/App.php @@ -334,10 +334,6 @@ public function group($pattern, $callable) /** @var RouteGroup $group */ $router = $this->getRouter(); $group = $router->pushGroup($pattern, $callable); - // TODO: Set group container and resolver inside Router - /*if ($this->container instanceof ContainerInterface) { - $group->setContainer($this->container); - }*/ if ($this->callableResolver instanceof CallableResolverInterface) { $group->setCallableResolver($this->callableResolver); } diff --git a/Slim/Routable.php b/Slim/Routable.php index 2ede4d892..553e04741 100644 --- a/Slim/Routable.php +++ b/Slim/Routable.php @@ -8,7 +8,6 @@ */ namespace Slim; -use Interop\Container\ContainerInterface; use Slim\Interfaces\CallableResolverInterface; /** @@ -31,13 +30,6 @@ abstract class Routable */ protected $callableResolver; - /** - * Container - * - * @var ContainerInterface - */ - //protected $container; - /** * Route middleware * @@ -92,19 +84,6 @@ public function getCallableResolver() return $this->callableResolver; } - /** - * Set container for use with resolveCallable - * - * @param ContainerInterface $container - * - * @return self - */ - /*public function setContainer(ContainerInterface $container) - { - $this->container = $container; - return $this; - }*/ - /** * Prepend middleware to the middleware collection * From f850464db53e441d7925c930c3bb9abdd10edf01 Mon Sep 17 00:00:00 2001 From: Josh Lockhart Date: Sun, 11 Dec 2016 12:11:35 -0500 Subject: [PATCH 4/4] Update UPGRADING.md --- UPGRADING.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/UPGRADING.md b/UPGRADING.md index e4508d5b6..ea1c11f2f 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -1,5 +1,7 @@ # How to upgrade * [2098] - You need to add the App's router to the container for a straight upgrade. If you've created your own router factory in the container though, then you need to set it into the $app. +* [2102] - You must inject custom route invocation strategy with `$app->getRouter()->setDefaultInvocationStrategy($myStrategy)` -[2098]: 92612999931799571752400592 \ No newline at end of file +[2098]: https://github.com/slimphp/Slim/pull/2098 +[2102]: https://github.com/slimphp/Slim/pull/2102 \ No newline at end of file