New redirect strategy

[Danielss89]

I change the way the redirect strategy works.
The redirects from authenticate/logout actions are now callbacks.
This way the user can overwrite the default callback and decide exactly what to should happen.

ping @Ocramius

closes #479
closes #476
closes #473
closes #469

[ghost]

👍

[juriansluiman]

I would extract a variable here to make it more readble:

$url = $this->getRedirectUrl(
    $routeMatch->getMatchedRouteName(),
    $routeMatch->getParam('redirect', false)
);

Note I also changed getRedirect() to getRedirectUrl() to show it should return a url.

[juriansluiman]

Another thing: where do you the the route parameter from? I don't see a route definition in this PR which adds a parameter to the route called redirect, so I assume now this is always false?

[juriansluiman]

I have added a lot of comments, but one question remains: where is the callback? I don't see any callback here or option to provide a callback. What it does, is fetching whether you can redirect (module options) and then redirect to the route provided by the module options (based on the current route).

What I would call a callback is that I can set a callback somewhere which is triggered when a redirect needs to take place. You get the request object or route match whatsoever and can return a route name. This gives the flexibility of a callback. Currently I would call this a controller plugin "RedirectStrategy" or something.

[mtudor]

The first thing I thought when I saw this was that it might be a RedirectStrategy (http://en.wikipedia.org/wiki/Strategy_pattern) that can be extended, replaced or even aggregated to determine redirect behaviour. If we go down that path then it's probably worth looking at the operation of the View Strategies (PhpRendererStrategy / JsonStrategy etc.) as a reference. They even self-select using events, so perhaps there is a possibility here to have a RedirectStrategy that self-selects in some way (yes I can handle that redirect parameter, so redirect OR; no I can't handle that, fall through to the next).

Alternatively, perhaps Strategies are too complicated for what we're attempting to do here. In actual fact, how many different redirect use cases do we really think there will be? Perhaps a ZfcUser RedirectPlugin is a better fit. To extend or alter the ZfcUser functionality you just replace the shipped plugin (extending the original class if you so choose).

Whatever we do, I think we should be including a version of the plugin / strategy that will also handle straight urls (with whitelists for security) as per the previous behaviour as it is sure to be a common use case and better that than everyone who needs the functionality writing it from scratch!

[mtudor]

As an aside, if we triggered a redirect event and handled it using strategies in a similar way to the selection of renderers in the core of ZF, that would also have the added benefit of providing an event that could be hooked just before any redirect occurs in ZfcUser (and potentially prevent the redirect under certain conditions etc.).

[Danielss89]

@juriansluiman @mtudor

I've refactored to use a controller plugin, the code is much prettier now. I think i "thought" myself blind(so that's why i didn't use controller plugin in first place).

I decided to use the name ZfcUserRedirect.

@mtudor I will create a zfcUserRedirectUrl module which extends and allows for redirect to urls from a whitelist. This is not in the scope for this module though, we try to keep it simple(which is why v. 2.0 will have some removed functionality).

[juriansluiman]

@Danielss89 much better now!

Please consider this (now outdated) comment as well: #480 (comment)

Also, what I wrote here is also applicable to the new code (L37-L39) ;)

[ojhaujjwal]

How about doing something like:
return $this->zfcUserRedirect($this->getOptions()->getLogoutRedirectRoute());.

Where, the argument passed represents the default redirect route.

This way there is no need to use switch statements in ZfcUser\Controller\Plugin\ZfcUserRedirect::getRedirectResponse().

[mtudor]

Just had a think about this one @ojhaujjwal. On the surface it's a nice optimisation, but I think the issue arises because it would be making assumptions about the actions that the plugin will take. If a user replaces the plugin with one of their own, they may not require a default route at all, but it would still be being passed in from the controller.

[ojhaujjwal]

Yes, you are right. That's why we may add the argument and make it optional.

[juriansluiman]

Another comment about the usage of the redirect plugin, you can copy the code from the Redirect plugin itself, which is a little bit more safe than what you do now:

/**
 * Stolen from ZF2
 * @see https://github.com/zendframework/zf2/blob/master/library/Zend/Mvc/Controller/Plugin/Redirect.php#L36
 */
protected function getRedirector()
{
    $controller = $this->getController();
    if (!$controller || !method_exists($controller, 'plugin')) {
        throw new Exception\DomainException('Redirect plugin requires a controller that defines the plugin() method');
    }

    $redirectPlugin = $controller->plugin('redirect');
}

Furthermore I am thinking about your method structure. Logically, the code is performing two steps:

1. Figure out the route name to redirect to
2. Perform the redirect

So why not:

class ZfcUserRedirect
{
    public function __invoke()
    {
        $route = $this->getRedirectRoute();
        return $this->getRedirectResponse($route);
    }

    public function getRedirectRoute()
    {
        $redirect = null;
        if ($useRedirect) {
            $redirect = $this->getRedirectRouteFromRequest();
        }

        $currentRoute = $this->routeMatch->getMatchedRouteName();
        switch ($currentRoute) {
            case 'zfcuser/login':
                $route = $redirect ?: $this->options->getLoginRedirectRoute();
                break;
            case 'zfcuser/logout':
                $route = $redirect ?: $this->options->getLogoutRedirectRoute();
                break;
            default:
                $route = 'zfcuser';
        }
    }

    public function getRedirectRouteFromRequest()
    {
        $request  = $this->getRequest();
        $redirect = $request->getQuery('redirect');
        if ($redirect && $this->routeExists($redirect)) {
            return $redirect;
        }

        $redirect = $request->getPost('redirect');
        if ($redirect && $this->routeExists($redirect)) {
            return $redirect;
        }
    }

    public function getRedirectResponse($route)
    {
        return $this->getRedirector()->toRoute($route):
    }
}

[shipleyr]

I think I feel happier with where this is going. Having this as a plugin that could be easily overwritten... I'm good with that.

One thing that I do feel is quite core and is missing at the moment is supporting routes that take parameters. The current way of working will not allow construction of a segment route that requires parameters as there is no way of these being passed in. I think we need to consider that some how? What do you think?

[Danielss89]

@shipleyr I will create another module: ZfcUserRedirectUrl which will allow the user to just send a url as the redirect param. Then it will check a whitelist to see if the redirect is allowed.

[juriansluiman]

@shipleyr +1, thinking about that too. Not sure though how to guess when you need the param

[Danielss89]

@juriansluiman you like better now? i do :P

[mtudor]

Should this now be $routeMatch, as referenced on L27? Although having said that, L105 still references $router although I'm not sure where this is set now?

[Danielss89]

Thx, yeah should me $routeMatch!

Mvh.
Daniel Strøm

On Wed, Jul 2, 2014 at 4:34 PM, Mark Tudor [email protected] wrote:

In src/ZfcUser/Controller/Plugin/ZfcUserRedirect.php:

+<?php
+
+namespace ZfcUser\Controller\Plugin;
+
+use Zend\Http\Response;
+use Zend\Mvc\Controller\Plugin\Redirect;
+use Zend\Mvc\Exception\DomainException;
+use Zend\Mvc\Exception\RuntimeException;
+use Zend\Mvc\Router\RouteMatch;
+use ZfcUser\Options\ModuleOptions;
+use Zend\Mvc\Controller\Plugin\AbstractPlugin;
+
+class ZfcUserRedirect extends AbstractPlugin
+{

• /** @var RouteMatch */
• protected $router;

Should this now be $routeMatch, as referenced on L27? Although having said
that, L105 still references the router although I'm not sure where this is
set now?

—
Reply to this email directly or view it on GitHub
https://github.com/ZF-Commons/ZfcUser/pull/480/files#r14459854.

[Ocramius]

callable

[Ocramius]

Newline before this line if possible

[Ocramius]

@mtudor depending on the CURRENT controller makes this classes' state unusable when it is either shared across multiple application instances or used within a forward() plugin's context.

[juriansluiman]

@mtudor depending on the CURRENT controller makes this classes' state unusable when it is either shared across multiple application instances or used within a forward() plugin's context.

@Ocramius incorrect. The plugin manager does inject the currently used controller in the PluginManager. You can perfectly use plugins inside plugins to get DRY code, no need to copy/paste existing plugin features into this plugin imho.

[Ocramius]

@juriansluiman I argue that the plugin system is a mess there. Let's avoid using broken stuff :P

[Ocramius]

Not descriptive

[jroedel]

It's been a few years since this has thread has received attention and I feel a little out-of-my-league speaking here, but I just made some adjustments to the 3.x branch in my fork that seem to be working fine for me (sorry I'm no expert in unit testing to provide tests).

I needed support for route parameters because I'm using SlmLocale for locale-based urls, and I was able to make it work by "matching" the redirect parameter in the router.

Also, I think it's real important that the $redirect param gets escaped in the _form.phtml view helper. That's a serious XSS vulnerability.

jroedel@aac1d7b