Refactor App Router Proxy Methods, RouteGroup, Routable and Route

[l0gicgate]

While doing work on #2555 I've noticed that App is being passed around in order to provide its proxy methods get(), post(), put(), patch(), delete(), options(), any() and map().

As seen here in App::group(), RouteGroup needs to be invoked so we can pass App to it in order for the callable to have access to the Router's proxy method:

    public function group(string $pattern, $callable): RouteGroupInterface
    {
        $router = $this->getRouter();

        /** @var RouteGroup $group */
        $group = $router->pushGroup($pattern, $callable);
        if ($this->callableResolver instanceof CallableResolverInterface) {
            $group->setCallableResolver($this->callableResolver);
        }

        $group($this);
        $router->popGroup();

        return $group;
    }

The reference flow of this is terrible, Router is in charge of handling groups, App shouldn't be. As you see here, $group($this); refers to RouteGroup::__invoke() which was just a hackish way to pass in App to the called function;

    public function __invoke(App $app = null)
    {
        /** @var callable $callable */
        $callable = $this->callable;
        if ($this->callableResolver) {
            $callable = $this->callableResolver->resolve($callable);
        }

        $callable($app);
    }

What I propose is that we create a RouterProxy that App can extend and that RouteGroup can extend as well so then we don't have to pass this reference in a hackish way:

abstract class RouterProxy {
    public function get(string $pattern, $callable): RouteInterface
    {
        return $this->map(['GET'], $pattern, $callable);
    }

    public function post(string $pattern, $callable): RouteInterface
    {
        return $this->map(['POST'], $pattern, $callable);
    }

    public function put(string $pattern, $callable): RouteInterface
    {
        return $this->map(['PUT'], $pattern, $callable);
    }

    public function patch(string $pattern, $callable): RouteInterface
    {
        return $this->map(['PATCH'], $pattern, $callable);
    }

    public function delete(string $pattern, $callable): RouteInterface
    {
        return $this->map(['DELETE'], $pattern, $callable);
    }

    public function options(string $pattern, $callable): RouteInterface
    {
        return $this->map(['OPTIONS'], $pattern, $callable);
    }

    public function any(string $pattern, $callable): RouteInterface
    {
        return $this->map(['GET', 'POST', 'PUT', 'PATCH', 'DELETE', 'OPTIONS'], $pattern, $callable);
    }

    public function map(array $methods, string $pattern, $callable): RouteInterface
    {
        // Bind route callable to container, if present
        if ($this->container instanceof ContainerInterface && $callable instanceof Closure) {
            $callable = $callable->bindTo($this->container);
        }
        return $this->router->map($methods, $pattern, $callable);
    }

    public function redirect(string $from, $to, int $status = 302): RouteInterface
    {
        $handler = function ($request, ResponseInterface $response) use ($to, $status) {
            return $response->withHeader('Location', (string)$to)->withStatus($status);
        };

        return $this->get($from, $handler);
    }

    public function group(string $pattern, $callable): RouteGroupInterface
    {
        $route = $this->router->pushGroup($pattern, $callable);
        $this->router->popGroup();
        return $route;
    }
}

This will clean up the code a lot. We can get rid of the Routable abstract class and Route will overtake all of those methods, then RouteGroup will extend the theoretical RouterProxy which will have Router injected in its constructor.

Need thoughts and comments on this.

Thanks y'all

[akrabat]

I agree that we can improve this. I'm not sure about extending Router and App from a common class though.

[l0gicgate]

@akrabat we’re not extending Router, that component stays the same.

What I’m proposing is creating a RouterProxy class that App and RouteGroup can extend. Perhaps we can just make RouterProxy a standalone object as well which App and RouteGroup can interact with instead of extending it.

Realistically all RouteGroup does is provide App and the group’s base of path to its callable when it is invoked.

I can probably do a PR with my concept once we merge #2555 and you’ll be able to see more clearly what I want to do.

[l0gicgate]

All changes proposed were executed in the following PRs:

• Decouple RouteGroup & Route from Routable - 4.x Refactor RouteGroup & Route #2612 Merged
• Decouple Router into RouteCollector and RouteResolver - 4.x - Split RouterInterface into RouteCollectorInterface and RouteResolverInterface #2622 Merged
• Add Route::setInvocationStrategy() to RouteInterface - 4.x - Route InvocationStrategy Setter #2634 Merged
• Add RouteCollector::fullUrlFor() as per Rewrite Router->urlFor() to generate complete Urls #2493 - 4.x - Port RouteCollector::fullUrlFor() #2638 Merged
• Introduce DispatcherInterface to decouple from FastRoute Dispatcher - 4.x - Decouple FastRoute Dispatcher From RouteResolver #2639 Merged
• Introduce RouteParserInterface to decouple from FastRoute RouteParser - 4.x - Decouple FastRoute RouteParser #2640 Merged
• Introduce RouteCollectorProxy - 4.x - RouteCollectorProxy #2641 & 4.x - Refactor RouteCollectorProxy #2642 Merged