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

New redirect strategy #480

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
7 changes: 5 additions & 2 deletions config/module.config.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,17 @@
),
),
'controllers' => array(
'invokables' => array(
'zfcuser' => 'ZfcUser\Controller\UserController',
'factories' => array(
'zfcuser' => 'ZfcUser\Factory\Controller\UserControllerFactory',
),
),
'service_manager' => array(
'aliases' => array(
'zfcuser_zend_db_adapter' => 'Zend\Db\Adapter\Adapter',
),
'factories' => array(
'zfcuser_redirect_callback' => 'ZfcUser\Factory\Controller\RedirectCallbackFactory'
)
),
'router' => array(
'routes' => array(
Expand Down
122 changes: 122 additions & 0 deletions src/ZfcUser/Controller/RedirectCallback.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
<?php

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing headers

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, this may be done later

namespace ZfcUser\Controller;

use Zend\Mvc\Router\RouteInterface;
use Zend\Mvc\Router\RouteMatch;
use Zend\Mvc\Router\Exception;
use Zend\Http\PhpEnvironment\Request;
use Zend\Http\PhpEnvironment\Response;
use ZfcUser\Options\ModuleOptions;

class RedirectCallback
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing docblock

{
/** @var RouteMatch */
protected $routeMatch;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RouteMatch should not be part of the object's state

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also make all properties private?


/** @var RouteInterface */
protected $router;

/** @var Response */
protected $response;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request and response should not be localized as properties.


/** @var Request */
protected $request;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above


/** @var ModuleOptions */
protected $options;

/**
* @param RouteMatch $routeMatch
* @param RouteInterface $router
* @param Response $response
* @param Request $request
* @param ModuleOptions $options
*/
public function __construct(RouteMatch $routeMatch, RouteInterface $router, Response $response, Request $request, ModuleOptions $options)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't be built with the RouteMatch, nor with the Request or the Response.

Those should all be fetched from either the application (passed in as a dependency) or from an MvcEvent passed in as callback parameter.

This allows usage of the RedirectCallback multiple times.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean in UserController __construct, or what do you mean by "application"?

{
$this->routeMatch = $routeMatch;
$this->router = $router;
$this->request = $request;
$this->response = $response;
$this->options = $options;
}

/**
* @return Response
*/
public function __invoke()
{
$redirect = $this->getRedirect($this->routeMatch->getMatchedRouteName(), $this->getRedirectRouteFromRequest());

$response = $this->response;
$response->getHeaders()->addHeaderLine('Location', $redirect);
$response->setStatusCode(302);
return $response;
}

/**
* Return the redirect from param.
* First checks GET then POST
* @return string
*/
protected function getRedirectRouteFromRequest()
{
$request = $this->request;
$redirect = $request->getQuery('redirect');
if ($redirect && $this->routeExists($redirect)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would you allow passing in a redirect route name?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is mostly because of BC. It is possible with current versions, and i want to backport this into 1.x and 0.x branches.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, wondering if it's that important - this is additional useless complexity :(

return $redirect;
}

$redirect = $request->getPost('redirect');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would you need to check post params?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current versions checks both GET and POST for a redirect param.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above. I understand the BC concerns, but this is really overkill. Maybe get rid of it later (2.x)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is only for 1.x and 0.x BC.
Will definitely be removed in 2.X

if ($redirect && $this->routeExists($redirect)) {
return $redirect;
}

return false;
}

/**
* @param $route
* @return bool
*/
protected function routeExists($route)
{
try {
$this->router->assemble($route);
} catch (Exception\RuntimeException $e) {
return false;
}
return true;
}

/**
* Returns the url to redirect to based on current route.
* If $redirect is set and the option to use redirect is set to true, it will return the $redirect url.
*
* @param string $currentRoute
* @param bool $redirect
* @return mixed
*/
protected function getRedirect($currentRoute, $redirect = false)
{
$useRedirect = $this->options->getUseRedirectParameterIfPresent();
$routeExists = ($redirect && $this->routeExists($redirect));
if (!$useRedirect || !$routeExists) {
$redirect = false;
}

switch ($currentRoute) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you make this thing a controller plugin (which I would advice), you can also reuse the url and/or redirect controller plugins. This just duplicates logic of assembling and returning responses

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, this should be a controller plugin. I am refactoring as we speak.

case 'zfcuser/login':
$route = ($redirect) ?: $this->options->getLoginRedirectRoute();
return $this->router->assemble([], ['name' => $route]);
break;
case 'zfcuser/logout':
$route = ($redirect) ?: $this->options->getLogoutRedirectRoute();
return $this->router->assemble([], ['name' => $route]);
break;
default:
return $this->router->assemble([], ['name' => 'zfcuser']);
}
}
}
27 changes: 15 additions & 12 deletions src/ZfcUser/Controller/UserController.php
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,16 @@ class UserController extends AbstractActionController
*/
protected $options;

/**
* @var Callable $redirectCallback
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

callable

*/
protected $redirectCallback;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this empty newline

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was 2 before, 1 now as it should be.

public function __construct($redirectCallback)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does ZfcUser require 5.3? Otherwise, callable can be given as a hint. Also, missing docblock

{
$this->redirectCallback = $redirectCallback;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check against is_callable() here

}

/**
* User page
*/
Expand Down Expand Up @@ -116,13 +126,8 @@ public function logoutAction()
$this->zfcUserAuthentication()->getAuthAdapter()->logoutAdapters();
$this->zfcUserAuthentication()->getAuthService()->clearIdentity();

$redirect = $this->params()->fromPost('redirect', $this->params()->fromQuery('redirect', false));

if ($this->getOptions()->getUseRedirectParameterIfPresent() && $redirect) {
return $this->redirect()->toRoute($redirect);
}

return $this->redirect()->toRoute($this->getOptions()->getLogoutRedirectRoute());
$redirect = $this->redirectCallback;
return $redirect();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Newline before this line if possible

}

/**
Expand Down Expand Up @@ -155,17 +160,15 @@ public function authenticateAction()
);
}

if ($this->getOptions()->getUseRedirectParameterIfPresent() && $redirect) {
return $this->redirect()->toRoute($redirect);
}

$route = $this->getOptions()->getLoginRedirectRoute();

if (is_callable($route)) {
$route = $route($this->zfcUserAuthentication()->getIdentity());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is a callback support needed here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we have this redirect callback, it's not needed, but it would be BC to remove.
Now that i think about it, this PR is against 2.x branch, so i could probably remove.

return $this->redirect()->toRoute($route);
}

return $this->redirect()->toRoute($route);
$redirect = $this->redirectCallback;
return $redirect();
}

/**
Expand Down
26 changes: 26 additions & 0 deletions src/ZfcUser/Factory/Controller/RedirectCallbackFactory.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?php
namespace ZfcUser\Factory\Controller;

use Zend\ServiceManager\FactoryInterface;
use Zend\ServiceManager\ServiceLocatorInterface;
use ZfcUser\Controller\RedirectCallback;

class RedirectCallbackFactory implements FactoryInterface
{
/**
* {@inheritDoc}
*/
public function createService(ServiceLocatorInterface $serviceLocator)
{
$router = $serviceLocator->get('Router');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add some IDE hints here? They help with static analysis

$response = $serviceLocator->get('Response');
$request = $serviceLocator->get('Request');
$options = $serviceLocator->get('zfcuser_module_options');

/** @var MvcEvent $mvcEvent */
$mvcEvent = $serviceLocator->get('Application')->getMvcEvent();
$routeMatch = $mvcEvent->getRouteMatch();

return new RedirectCallback($routeMatch, $router, $response, $request, $options);
}
}
25 changes: 25 additions & 0 deletions src/ZfcUser/Factory/Controller/UserControllerFactory.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<?php
namespace ZfcUser\Factory\Controller;

use Zend\Mvc\Controller\ControllerManager;
use Zend\ServiceManager\FactoryInterface;
use Zend\ServiceManager\ServiceLocatorInterface;
use ZfcUser\Authentication\Adapter;
use ZfcUser\Controller\UserController;

class UserControllerFactory implements FactoryInterface
{
/**
* {@inheritDoc}
*/
public function createService(ServiceLocatorInterface $controllerManager)
{
/** @var ControllerManager $controllerManager*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/*, not /**

$serviceManager = $controllerManager->getServiceLocator();

$redirectCallback = $serviceManager->get('zfcuser_redirect_callback');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above - hints

$controller = new UserController($redirectCallback);

return $controller;
}
}
71 changes: 13 additions & 58 deletions tests/ZfcUserTest/Controller/UserControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,15 @@ class UserControllerTest extends \PHPUnit_Framework_TestCase

protected $options;

protected $redirectCallback;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing docblock


public function setUp()
{
$controller = new Controller;
$this->redirectCallback = $this->getMockBuilder('ZfcUser\Controller\RedirectCallback')
->disableOriginalConstructor()
->getMock();

$controller = new Controller($this->redirectCallback);
$this->controller = $controller;

$this->zfcUserAuthenticationPlugin = $this->getMock('ZfcUser\Controller\Plugin\ZfcUserAuthentication');
Expand Down Expand Up @@ -362,50 +368,11 @@ public function testLogoutAction($withRedirect, $post, $query)
));


$params = $this->getMock('Zend\Mvc\Controller\Plugin\Params');
$params->expects($this->any())
->method('__invoke')
->will($this->returnSelf());
$params->expects($this->once())
->method('fromPost')
->will($this->returnCallback(function ($key, $default) use ($post) {
return $post ?: $default;
}));
$params->expects($this->once())
->method('fromQuery')
->will($this->returnCallback(function ($key, $default) use ($query) {
return $query ?: $default;
}));
$this->pluginManagerPlugins['params'] = $params;

$response = new Response();

$redirect = $this->getMock('Zend\Mvc\Controller\Plugin\Redirect');
$redirect->expects($this->any())
->method('toRoute')
->will($this->returnValue($response));

if ($withRedirect) {
$expectedLocation = $post ?: $query ?: false;
$this->options->expects($this->once())
->method('getUseRedirectParameterIfPresent')
->will($this->returnValue((bool) $withRedirect));
$redirect->expects($this->any())
->method('toRoute')
->with($expectedLocation)
->will($this->returnValue($response));
} else {
$expectedLocation = "/user/logout";
$this->options->expects($this->once())
->method('getLogoutRedirectRoute')
->will($this->returnValue($expectedLocation));
$redirect->expects($this->any())
->method('toRoute')
->with($expectedLocation)
->will($this->returnValue($response));
}

$this->pluginManagerPlugins['redirect']= $redirect;
$this->redirectCallback->expects($this->once())
->method('__invoke')
->will($this->returnValue($response));

$result = $controller->logoutAction();

Expand Down Expand Up @@ -513,21 +480,9 @@ public function testAuthenticateAction($wantRedirect, $post, $query, $prepareRes
->will($this->returnValue('user/login'));
$this->pluginManagerPlugins['url'] = $url;

} elseif ($wantRedirect && $hasRedirect) {
$redirect->expects($this->once())
->method('toRoute')
->with(($post ?: $query ?: false))
->will($this->returnValue($response));
} else {

$redirect->expects($this->once())
->method('toRoute')
->with('zfcuser')
->will($this->returnValue($response));

$this->options->expects($this->once())
->method('getLoginRedirectRoute')
->will($this->returnValue('zfcuser'));
$this->redirectCallback->expects($this->once())
->method('__invoke');
}

$this->options->expects($this->any())
Expand Down Expand Up @@ -1010,7 +965,7 @@ public function testSetterGetterServices(
$serviceName,
$callback = null
) {
$controller = new Controller;
$controller = new Controller($this->redirectCallback);

$controller->setPluginManager($this->pluginManager);

Expand Down