diff --git a/src/FastRouteRouter.php b/src/FastRouteRouter.php index b2db23d..851ce14 100644 --- a/src/FastRouteRouter.php +++ b/src/FastRouteRouter.php @@ -7,11 +7,11 @@ namespace Zend\Expressive\Router; -use Fig\Http\Message\RequestMethodInterface as RequestMethod; use FastRoute\DataGenerator\GroupCountBased as RouteGenerator; use FastRoute\Dispatcher\GroupCountBased as Dispatcher; use FastRoute\RouteCollector; use FastRoute\RouteParser\Std as RouteParser; +use Fig\Http\Message\RequestMethodInterface as RequestMethod; use Psr\Http\Message\ServerRequestInterface as Request; use Zend\Expressive\Router\Exception; use Zend\Stdlib\ArrayUtils; @@ -28,17 +28,14 @@ class FastRouteRouter implements RouterInterface createRouter(); } - $this->router = $router; + $this->router = $router; $this->dispatcherCallback = $dispatcherFactory; $this->loadConfig($config); @@ -179,6 +157,7 @@ public function __construct( * Load configuration parameters * * @param array $config Array of custom configuration options. + * * @return void */ private function loadConfig(array $config = null) @@ -188,11 +167,11 @@ private function loadConfig(array $config = null) } if (isset($config[self::CONFIG_CACHE_ENABLED])) { - $this->cacheEnabled = (bool) $config[self::CONFIG_CACHE_ENABLED]; + $this->cacheEnabled = (bool)$config[self::CONFIG_CACHE_ENABLED]; } if (isset($config[self::CONFIG_CACHE_FILE])) { - $this->cacheFile = (string) $config[self::CONFIG_CACHE_FILE]; + $this->cacheFile = (string)$config[self::CONFIG_CACHE_FILE]; } if ($this->cacheEnabled) { @@ -216,6 +195,7 @@ public function addRoute(Route $route) /** * @param Request $request + * * @return RouteResult */ public function match(Request $request) @@ -248,11 +228,8 @@ public function match(Request $request) * Generate a URI based on a given route. * * Replacements in FastRoute are written as `{name}` or `{name:}`; - * this method uses a regular expression to search for substitutions that - * match, and replaces them with the value provided. - * - * It does *not* use the pattern to validate that the substitution value is - * valid beforehand, however. + * this method uses `FastRoute\RouteParser\Std` to search for the best route + * match based on the available substitutions and generates a uri. * * @param string $name Route name. * @param array $substitutions Key/value pairs to substitute into the route @@ -260,9 +237,10 @@ public function match(Request $request) * @param array $options Key/value option pairs to pass to the router for * purposes of generating a URI; takes precedence over options present * in route used to generate URI. + * * @return string URI path generated. - * @throws Exception\InvalidArgumentException if the route name is not - * known. + * @throws Exception\InvalidArgumentException if the route name is not known + * or a parameter value does not match its regex. */ public function generateUri($name, array $substitutions = [], array $options = []) { @@ -277,41 +255,94 @@ public function generateUri($name, array $substitutions = [], array $options = [ } $route = $this->routes[$name]; - $path = $route->getPath(); $options = ArrayUtils::merge($route->getOptions(), $options); if (! empty($options['defaults'])) { $substitutions = array_merge($options['defaults'], $substitutions); } - foreach ($substitutions as $key => $value) { - $pattern = sprintf( - '~%s~x', - sprintf(self::VARIABLE_REGEX, preg_quote($key)) - ); - $path = preg_replace($pattern, $value, $path); - } + $routeParser = new RouteParser(); + $routes = array_reverse($routeParser->parse($route->getPath())); + $missingParameters = []; + + // One route pattern can correspond to multiple routes if it has optional parts + foreach ($routes as $parts) { + // Check if all parameters can be substituted + $missingParameters = $this->missingParameters($parts, $substitutions); + + // If not all parameters can be substituted, try the next route + if (! empty($missingParameters)) { + continue; + } + + // Generate the path + $path = ''; + foreach ($parts as $part) { + if (is_string($part)) { + // Append the string + $path .= $part; + continue; + } - // 1. remove optional segments' ending delimiters - // and remove leftover regex char classes like `:[` and `:prefix-[` (issue #18) - // 2. split path into an array of optional segments and remove those - // containing unsubstituted parameters starting from the last segment - $path = preg_replace('/\]|:.*\[/', '', $path); - $segs = array_reverse(explode('[', $path)); - foreach ($segs as $n => $seg) { - if (strpos($seg, '{') !== false) { - if (isset($segs[$n - 1])) { - throw new Exception\InvalidArgumentException( - 'Optional segments with unsubstituted parameters cannot ' - . 'contain segments with substituted parameters when using FastRoute' - ); + // Check substitute value with regex + if (! preg_match('~^' . $part[1] . '$~', $substitutions[$part[0]])) { + throw new Exception\InvalidArgumentException(sprintf( + 'Parameter value for [%s] did not match the regex `%s`', + $part[0], + $part[1] + )); } - unset($segs[$n]); + + // Append the substituted value + $path .= $substitutions[$part[0]]; } + + // Return generated path + return $path; } - $path = implode('', array_reverse($segs)); - return $path; + // No valid route was found: list minimal required parameters + throw new Exception\InvalidArgumentException(sprintf( + 'Route `%s` expects at least parameter values for [%s], but received [%s]', + $name, + implode(',', $missingParameters), + implode(',', array_keys($substitutions)) + )); + } + + /** + * Checks for any missing route parameters + * + * @param array $parts + * @param array $substitutions + * + * @return array with minimum required parameters if any are missing or + * an empty array if none are missing + */ + private function missingParameters(array $parts, array $substitutions) + { + $missingParameters = []; + + // Gather required parameters + foreach ($parts as $part) { + if (is_string($part)) { + continue; + } + + $missingParameters[] = $part[0]; + } + + // Check if all parameters exist + foreach ($missingParameters as $param) { + if (! isset($substitutions[$param])) { + // Return the parameters so they can be used in an + // exception if needed + return $missingParameters; + } + } + + // All required parameters are available + return []; } /** @@ -331,7 +362,8 @@ private function createRouter() * (which should be derived from the router's getData() method); this * approach is done to allow testing against the dispatcher. * - * @param array|object $data Data from RouteCollection::getData() + * @param array|object $data Data from RouteCollection::getData() + * * @return Dispatcher */ private function getDispatcher($data) @@ -341,6 +373,7 @@ private function getDispatcher($data) } $factory = $this->dispatcherCallback; + return $factory($data); } @@ -363,6 +396,7 @@ private function createDispatcherCallback() * methods to the factory. * * @param array $result + * * @return RouteResult */ private function marshalFailedRoute(array $result) @@ -370,6 +404,7 @@ private function marshalFailedRoute(array $result) if ($result[0] === Dispatcher::METHOD_NOT_ALLOWED) { return RouteResult::fromRouteFailure($result[1]); } + return RouteResult::fromRouteFailure(); } @@ -378,6 +413,7 @@ private function marshalFailedRoute(array $result) * * @param array $result * @param string $method + * * @return RouteResult */ private function marshalMatchedRoute(array $result, $method) @@ -502,7 +538,7 @@ private function loadDispatchData() )); } - $this->hasCache = true; + $this->hasCache = true; $this->dispatchData = $dispatchData; } @@ -510,6 +546,7 @@ private function loadDispatchData() * Save dispatch data to cache * * @param array $dispatchData + * * @return int|false bytes written to file or false if error * @throws Exception\InvalidCacheDirectoryException If the cache directory * does not exist. @@ -550,6 +587,7 @@ private function cacheDispatchData(array $dispatchData) * @param string $method * @param string $path * @param Dispatcher $dispatcher + * * @return false|array False if no match found, array representing the match * otherwise. */ @@ -564,6 +602,7 @@ private function probeIntrospectionMethod($method, $path, Dispatcher $dispatcher return $result; } } + return false; } } diff --git a/test/FastRouteRouterTest.php b/test/FastRouteRouterTest.php index edf61e9..bd183d6 100644 --- a/test/FastRouteRouterTest.php +++ b/test/FastRouteRouterTest.php @@ -1,7 +1,7 @@ assertEquals('foo-route', $result->getMatchedRouteName()); } - public function testGenerateUriRaisesExceptionForIncompleteUriSubstitutions() - { - $router = new FastRouteRouter(); - $route = new Route('/foo[/{param}[/optional-{extra}]]', 'foo', ['GET'], 'foo'); - $router->addRoute($route); - - $this->expectException(InvalidArgumentException::class); - $this->expectExceptionMessage('unsubstituted parameters'); - - $router->generateUri('foo', ['extra' => 'segment']); - } - public function uriGeneratorDataProvider() { return [ @@ -631,4 +619,19 @@ public function testFastRouteCache() unlink($cache_file); } + + /** + * Test for issue #30 + */ + public function testGenerateUriRaisesExceptionForMissingMandatoryParameters() + { + $router = new FastRouteRouter(); + $route = new Route('/foo/{id}', 'foo', ['GET'], 'foo'); + $router->addRoute($route); + + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('expects at least parameter values for'); + + $router->generateUri('foo'); + } } diff --git a/test/UriGeneratorTest.php b/test/UriGeneratorTest.php new file mode 100644 index 0000000..575c216 --- /dev/null +++ b/test/UriGeneratorTest.php @@ -0,0 +1,165 @@ + '/test/{param:\d+}', + 'test_param_regex_limit' => '/test/{ param : \d{1,9} }', + 'test_optional' => '/test[opt]', + 'test_optional_param' => '/test[/{param}]', + 'param_and_opt' => '/{param}[opt]', + 'test_double_opt' => '/test[/{param}[/{id:[0-9]+}]]', + 'empty' => '', + 'optional_text' => '[test]', + 'root_and_text' => '/{foo-bar}', + 'root_and_regex' => '/{_foo:.*}', + ]; + } + + /** + * @return array + */ + public function provideRouteTests() + { + return [ + // path // substitutions[] // expected // exception + ['/test', [], '/test', null], + + ['/test/{param}', ['param' => 'foo'], '/test/foo', null], + [ + '/test/{param}', + ['id' => 'foo'], + InvalidArgumentException::class, + 'expects at least parameter values for', + ], + + ['/te{ param }st', ['param' => 'foo'], '/tefoost', null], + + ['/test/{param1}/test2/{param2}', ['param1' => 'foo', 'param2' => 'bar'], '/test/foo/test2/bar', null], + + ['/test/{param:\d+}', ['param' => 1], '/test/1', null], + //['/test/{param:\d+}', ['param' => 'foo'], 'exception', null], + + ['/test/{ param : \d{1,9} }', ['param' => 1], '/test/1', null], + ['/test/{ param : \d{1,9} }', ['param' => 123456789], '/test/123456789', null], + ['/test/{ param : \d{1,9} }', ['param' => 0], '/test/0', null], + [ + '/test/{ param : \d{1,9} }', + ['param' => 1234567890], + InvalidArgumentException::class, + 'Parameter value for [param] did not match the regex `\d{1,9}`', + ], + + ['/test[opt]', [], '/testopt', null], + + ['/test[/{param}]', [], '/test', null], + ['/test[/{param}]', ['param' => 'foo'], '/test/foo', null], + + ['/{param}[opt]', ['param' => 'foo'], '/fooopt', null], + + ['/test[/{param}[/{id:[0-9]+}]]', [], '/test', null], + ['/test[/{param}[/{id:[0-9]+}]]', ['param' => 'foo'], '/test/foo', null], + ['/test[/{param}[/{id:[0-9]+}]]', ['param' => 'foo', 'id' => 1], '/test/foo/1', null], + ['/test[/{param}[/{id:[0-9]+}]]', ['id' => 1], '/test', null], + [ + '/test[/{param}[/{id:[0-9]+}]]', + ['param' => 'foo', 'id' => 'foo'], + InvalidArgumentException::class, + 'Parameter value for [id] did not match the regex `[0-9]+`', + ], + + ['', [], '', null], + + ['[test]', [], 'test', null], + + ['/{foo-bar}', ['foo-bar' => 'bar'], '/bar', null], + + ['/{_foo:.*}', ['_foo' => 'bar'], '/bar', null], + ]; + } + + protected function setUp() + { + $this->fastRouter = $this->prophesize(RouteCollector::class); + $this->dispatcher = $this->prophesize(Dispatcher::class); + $this->dispatchCallback = function () { + return $this->dispatcher->reveal(); + }; + + $this->router = new FastRouteRouter( + $this->fastRouter->reveal(), + $this->dispatchCallback + ); + } + + /** + * @param $path + * @param $substitutions + * @param $expected + * @param $message + * + * @dataProvider provideRouteTests + */ + public function testRoutes($path, $substitutions, $expected, $message) + { + $this->router->addRoute(new Route($path, 'foo', ['GET'], 'foo')); + + if ($message !== null) { + // Test exceptions + $this->expectException($expected); + $this->expectExceptionMessage($message); + + $this->router->generateUri('foo', $substitutions); + + return; + } + + $this->assertEquals($expected, $this->router->generateUri('foo', $substitutions)); + + // Test with extra parameter + $substitutions['extra'] = 'parameter'; + $this->assertEquals($expected, $this->router->generateUri('foo', $substitutions)); + } +}