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

FastRouteRouter->addRoute() does not throw a RuntimeException of called after either match() or geerateUri() #2

Open
2 tasks done
weierophinney opened this issue Dec 31, 2019 · 2 comments

Comments

@weierophinney
Copy link
Contributor

The Zend\Expressive\Router\RouterInface states on addRoute():

    /**
     * ...
     *
     * The method MUST raise Exception\RuntimeException if called after either `match()`
     * or `generateUri()` have already been called, to ensure integrity of the
     * router between invocations of either of those methods.
     *
     * @throws Exception\RuntimeException when called after match() or
     *     generateUri() have been called.
     */
    public function addRoute(Route $route) : void;

But no RuntimeException will be thrown / raised.

Code to reproduce the issue

class TestMiddleware implements \Psr\Http\Server\MiddlewareInterface
{
  public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface {
    return $handler->handle($request);
  }
}

$router = new \Zend\Expressive\Router\FastRouteRouter();
$router->addRoute('/test', new TestMiddleware());
$router->match($request);
$router->addRoute('/test2', new TestMiddleware());

Expected results

A Zend\Expressive\Router\Exception\RuntimeException is thrown indicating that no routes may be added after calling match().

Actual results

Scripts run without an exception.

Consequences

Is this a bug in the RouterInterface of "zend-expressive-router" or in the FastRouteRouter of this repo?
(Same "error" (?) is in the "zend-expressive-zendrouter").


Originally posted by @MatthiasKuehneEllerhold at zendframework/zend-expressive-fastroute#60

@weierophinney
Copy link
Contributor Author

I guess this is a typo or something that's never implemented. I've checked all routers and they all do this (including older versions):

    public function addRoute(Route $route) : void
    {
        $this->routesToInject[] = $route;
    }

I guess you can even call it a feature to add more routes after you matched a route already :)

And to be honest, why would you have that check? When adding a lot of routes it only creates overhead.


Originally posted by @geerteltink at zendframework/zend-expressive-fastroute#60 (comment)

@weierophinney
Copy link
Contributor Author

So it'd be fine to remove this part of the docblock from zend-expressive?


Originally posted by @MatthiasKuehneEllerhold at zendframework/zend-expressive-fastroute#60 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant