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

Refactor App Router Proxy Methods, RouteGroup, Routable and Route #2583

Closed
l0gicgate opened this issue Feb 17, 2019 · 3 comments

Comments

Projects
None yet
2 participants
@l0gicgate
Copy link
Contributor

commented Feb 17, 2019

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

@l0gicgate l0gicgate added the Slim 4 label Feb 17, 2019

@l0gicgate l0gicgate added this to the 4.0 milestone Feb 17, 2019

@l0gicgate l0gicgate self-assigned this Feb 17, 2019

@akrabat

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

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

@l0gicgate

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2019

All changes proposed were executed in the following PRs:

  • Decouple RouteGroup & Route from Routable - #2612 Merged
  • Decouple Router into RouteCollector and RouteResolver - #2622 Merged
  • Add Route::setInvocationStrategy() to RouteInterface - #2634 Merged
  • Add RouteCollector::fullUrlFor() as per #2493 - #2638 Merged
  • Introduce DispatcherInterface to decouple from FastRoute Dispatcher - #2639 Merged
  • Introduce RouteParserInterface to decouple from FastRoute RouteParser - #2640 Merged
  • Introduce RouteCollectorProxy - #2641 & #2642 Merged

@l0gicgate l0gicgate closed this Apr 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.