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

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 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->request = $application->getRequest();
$this->response = $application->getResponse();
Copy link
Member Author

Choose a reason for hiding this comment

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

@Ocramius set the request/response like this or set $this->application and then get the request/response in the methods through $this->application?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is absolutely NOT ok. The request/response should be fetched from the application during callback execution time. To give you an idea of why this may break, try using something like

$application->expects($this->at(0))->method('getRequest')->will($this->returnValue($request1));
$application->expects($this->at(1))->method('getRequest')->will($this->returnValue($request2));

$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([], ['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']);
}
}
}
31 changes: 19 additions & 12 deletions src/ZfcUser/Controller/UserController.php
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,20 @@ 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

{
$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 +130,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 +164,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
21 changes: 21 additions & 0 deletions src/ZfcUser/Factory/Controller/RedirectCallbackFactory.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<?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

$application = $serviceLocator->get('Application');
$options = $serviceLocator->get('zfcuser_module_options');

return new RedirectCallback($application, $router, $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;
}
}
Loading