mezzio / mezzio-fastroute

FastRoute integration for Mezzio
https://docs.mezzio.dev/mezzio/features/router/fast-route/
BSD 3-Clause "New" or "Revised" License
16 stars 11 forks source link

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

Open weierophinney opened 4 years ago

weierophinney commented 4 years ago

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 https://github.com/zendframework/zend-expressive-fastroute/issues/60

weierophinney commented 4 years ago

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 https://github.com/zendframework/zend-expressive-fastroute/issues/60#issuecomment-425995923

weierophinney commented 4 years ago

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


Originally posted by @MatthiasKuehneEllerhold at https://github.com/zendframework/zend-expressive-fastroute/issues/60#issuecomment-426150070