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
127 changes: 127 additions & 0 deletions src/ZfcUser/Controller/RedirectCallback.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
<?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\Application;
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.

Not descriptive

* @package ZfcUser\Controller
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for @package

*/
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 Application */
protected $application;

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

/**
* @param Application $application
* @param RouteInterface $router
* @param ModuleOptions $options
*/
public function __construct(Application $application, RouteInterface $router, ModuleOptions $options)
{
$this->routeMatch = $application->getMvcEvent()->getRouteMatch();
$this->router = $router;
$this->application = $application;
$this->options = $options;
}

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

$response = $this->application->getResponse();
$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->application->getRequest();
$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([], ['name' => $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']);
}
}
}
33 changes: 22 additions & 11 deletions src/ZfcUser/Controller/UserController.php
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,22 @@ class UserController extends AbstractActionController
*/
protected $options;

/**
* @var callable $redirectCallback
*/
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.

/**
* @param callable $redirectCallback
*/
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

{
if (!is_callable($redirectCallback)) {
throw new \InvalidArgumentException('You must supply a callable redirectCallback');
}
$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 +132,9 @@ 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);
}
$redirect = $this->redirectCallback;

return $this->redirect()->toRoute($this->getOptions()->getLogoutRedirectRoute());
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 +167,16 @@ 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
29 changes: 29 additions & 0 deletions src/ZfcUser/Factory/Controller/RedirectCallbackFactory.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
<?php
namespace ZfcUser\Factory\Controller;

use Zend\Mvc\Application;
use Zend\Mvc\Router\RouteInterface;
use Zend\ServiceManager\FactoryInterface;
use Zend\ServiceManager\ServiceLocatorInterface;
use ZfcUser\Controller\RedirectCallback;
use ZfcUser\Options\ModuleOptions;

class RedirectCallbackFactory implements FactoryInterface
{
/**
* {@inheritDoc}
*/
public function createService(ServiceLocatorInterface $serviceLocator)
{
/* @var RouteInterface $router */
$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


/* @var Application $application */
$application = $serviceLocator->get('Application');

/* @var ModuleOptions $options */
$options = $serviceLocator->get('zfcuser_module_options');

return new RedirectCallback($application, $router, $options);
}
}
29 changes: 29 additions & 0 deletions src/ZfcUser/Factory/Controller/UserControllerFactory.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
<?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\RedirectCallback;
use ZfcUser\Controller\UserController;

class UserControllerFactory implements FactoryInterface
{
/**
* {@inheritDoc}
*/
public function createService(ServiceLocatorInterface $controllerManager)
{
/* @var ControllerManager $controllerManager*/
$serviceManager = $controllerManager->getServiceLocator();

/* @var RedirectCallback $redirectCallback */
$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


/* @var UserController $controller */
$controller = new UserController($redirectCallback);

return $controller;
}
}
Loading