Remove UrlGenerator dependency from Redirect

[armpogart]

I propose to remove UrlGenerator dependency from Redirect middleware as it is impossible currently to use the middleware while defining the routes for the router as the UrlGenerator can not be instantiated without having the Router.

I have a use case when I want to redirect to other url on some simple condition/check in the router, I specify closure middleware, where I do my check and based on that check I chain Redirect middleware.

My suggestion would be just to document the usage, where you can pass UrlGenerator generated router to toUrl method.

[samdark]

You mean removing toRoute() altogether?

[armpogart]

@samdark If toRoute can somehow be implemented without UrlGenerator which I doubt, it can remain. toRoute is very useful, but the named routes are not available until router in instantiated, so currently Redirect can not be used while specifying the routes, and it can not be also used in any other middleware which is running before router.

toRoute can be removed with UrlGenerator dependency and simply document toUrl($urlGenerator->generate('name', $params)) usage. I don't think it is a good practice at all to have dependency in middleware which is another middleware (UrlGenerator is not a middleware, but it can not be instantiated without router middleware, so it is almost the same as when Redirect had Router dependency). When one middleware depends on other middleware it can create many problems and would be important to document precisely when and how such middleware can be used and the order of chaining middlewares also becomes important.

Ideally any middleware can be called anywhere in the middlewares chain.

P.S. Yes, I know that it is not always possible to have such middlewares only, but in this specific case I think toRoute is not that much important as it can be easily replaced with a call to toUrl($alreadyGeneratedUrl) where appropriate in the code. And it is important to make such decisions early not to make BC changes later.

Another way would be just to have 2 redirect middleware. One simple redirect middleware without UrlGenerator dependency and another one (which may even extend from the first) e.g. NamedRedirect with UrlGenerator baked in and clearly documented that it can only be used after the router is instantiated.

[samdark]

Agree. I'm fine with removing toRoute().

[armpogart]

Ok, will send a PR soon.

[samdark]

@yiiliveext suggested alternative approach:

1. Make Redirect accept container instance.
2. Get URL generator from container when toRoute() called only.

This way it will be possible to use routes when defining redirects.

[samdark]

Or toRoute() can accept UrlGeneratorInterface:

Route::get('/superuser/profile')->addMiddleware((new Redirect($responseFactory))->toRoute('user/profile', [], $urlGenerator))

[samdark]

Or another alternative as suggested by @roxblnfk is to make Redirect a decorator around Response:

class Redirect implements ResponseInterface
{
    public function __construct(ResponseFactoryInterface $responseFactory)
    {
    }

     // ...
}

Then you can use it like this:

public function actionX()
{
    return (new Redirect($this->responseFactory))->toUrl('bla-bla');
}

I like this last approach.

[samdark]

From recent discussion. Usage:

Route::get('/superuser/profile')
->addMiddleware(fn (Redirect $redirect) => $redirect->toRoute('user/profile'))

Implementation:

class Redirect
{
    public function __construct(ResponseFactoryInterface $responseFactory)
    {
    }

    public function toRoute(string $route, array $params): ResponseInterface
    {
    }
}

[samdark]

@armpogart it actually works as is:

Route::get('/superuser/profile', [...])
    ->addMiddleware(fn (Redirect $redirect) => $redirect->toRoute('user/profile'));

[armpogart]

The classic middleware approach (which I tried) would be

Route::get('/redirect', [...])
    ->addMiddleware($container->get(Redirect::class)->toRoute('route'));

or

Route::get('/catch-all-route', $container->get(Redirect::class)->toRoute('route'));

to call the middleware directly. Any correct middleware must be able to be used that way.
On both cases it won't work, as UrlGenerator can not be instantiated that early as I said previously.

@samdark As I understand the only way that could work is that in your case the actual middleware is the closure, and it is lazily invoked at later stage (when actually it is needed) with Yiisoft/Injector which injects Redirect instance to closure and at that stage our router is already instantiated. But in that case I don't see any reason for Redirect to be middleware and implement MiddlewareInterface as in your example it is not used as middleware, it is just wrapped in closure middleware, and it can simply return ResponseInterface from toUrl and toRoute method, or better it can just implement ResponseInterface.

And if I think further it is more correct for the Redirect to implement ResponseInterface as it is just a response with redirect header and status code. And I don't see now any reason for the Redirect to be middleware anyways. In case if it just implements ResponseInterface it could be used as in your last example and it can also be used in controllers to return redirect response.

What do you think about implementing ResponseInterface instead of MiddlewareInterface and documenting correctly its usage.

[samdark]

I'm fine with it.