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.

Sign up for GitHub

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

Jump to bottom

Make redirect url fallback configurable #190

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Make redirect url fallback configurable #190

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2bfbe41
Make redirect url fallback configurable
Jun 7, 2018
e961202
Fix unit tests
Jun 13, 2018
92ba4e6
Use / as fallback when there is no route given
Jun 13, 2018
3b49d2c
Fix missing $this…
Jun 13, 2018
ef72908
Make statement CS compliant
Jun 13, 2018
741ca10
Remove redundant code
Jun 13, 2018
ab6eb14
Set default value of redirect_fallback to null
Jun 13, 2018
be1e60d
Draft for routing test
Jun 15, 2018
7537a58
fixed tests by removing the Request mock
lsmith77 Jun 16, 2018
08aef0e
Merge pull request #1 from liip/AsfalothDE-master
AsfalothDE Jun 18, 2018
068dbb0
Add documentation for redirect_fallback
Jun 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 25 additions & 2 deletions Controller/ThemeController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\RedirectResponse;
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
use Symfony\Component\Routing\RouterInterface;

/**
* Theme controller.
Expand All @@ -40,18 +41,32 @@ class ThemeController
*/
protected $cookieOptions;

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

/**
* @var string|null
*/
protected $defaultRoute;

/**
* Theme controller construct.
*
* @param ActiveTheme $activeTheme active theme instance
* @param array $themes Available themes
* @param array|null $cookieOptions The options of the cookie we look for the theme to set
* @param RouterInterface $router
* @param string|null $defaultRoute
*/
public function __construct(ActiveTheme $activeTheme, array $themes, array $cookieOptions = null)
public function __construct(ActiveTheme $activeTheme, array $themes, array $cookieOptions = null, RouterInterface $router, $defaultRoute = null)
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 the $router parameter nullable to maintain BC?

{
$this->activeTheme = $activeTheme;
$this->themes = $themes;
$this->cookieOptions = $cookieOptions;
$this->router = $router;
$this->defaultRoute = $defaultRoute;
}

/**
Expand All @@ -73,7 +88,15 @@ public function switchAction(Request $request)

$this->activeTheme->setName($theme);

$url = $request->headers->get('Referer', '/');

if ($this->defaultRoute) {
Copy link
Contributor

Choose a reason for hiding this comment

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

once the router property is nullable, you need to check if its set here as well

$redirect = $this->router->generate($this->defaultRoute);
Copy link
Contributor

Choose a reason for hiding this comment

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

because we cannot use homepage as default route, and will probably use hardcoded / (see comment to Configuration), you no longer need router in this controller

Copy link
Author

Choose a reason for hiding this comment

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

The point of this change is not to use a hardcoded route back to "/", because in some cases Symfony may not run in root domain. In a later commit I added "/" as a fallback in case there is no route set in config, so we should have that covered.

Copy link
Author

Choose a reason for hiding this comment

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

Default configuration for route is now "null". When there is a route given in the configuration file, the controller uses the router to redirect back to that route. If no route is given in configuration, it defaults to "null" and the redirect goes to "/" as before. :)

} else {
$redirect = '/';
}

$url = $request->headers->get('Referer', $redirect);

$response = new RedirectResponse($url);

if (!empty($this->cookieOptions)) {
Expand Down
1 change: 1 addition & 0 deletions DependencyInjection/Configuration.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ public function getConfigTreeBuilder()
->prototype('scalar')->end()
->end()
->scalarNode('active_theme')->defaultNull()->end()
->scalarNode('redirect_fallback')->defaultValue('homepage')->end()
Copy link
Contributor

Choose a reason for hiding this comment

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

homepage may not exist for other people who are using this bundle, it's better to put /

Copy link
Author

Choose a reason for hiding this comment

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

Okay, fair point, I will change that to use null as default as prepared in ThemeController.php already.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

->arrayNode('path_patterns')
->addDefaultsIfNotSet()
->children()
Expand Down
2 changes: 1 addition & 1 deletion DependencyInjection/LiipThemeExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public function load(array $configs, ContainerBuilder $container)
{
$config = $this->processConfiguration(new Configuration(), $configs);

foreach (array('themes', 'active_theme', 'path_patterns', 'cache_warming') as $key) {
foreach (array('themes', 'active_theme', 'path_patterns', 'cache_warming', 'redirect_fallback') as $key) {
$container->setParameter($this->getAlias().'.'.$key, $config[$key]);
}

Expand Down
2 changes: 2 additions & 0 deletions Resources/config/controller.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
<argument type="service" id="liip_theme.active_theme" />
<argument>%liip_theme.themes%</argument>
<argument>%liip_theme.cookie%</argument>
<argument type="service" id="router"/>
<argument>%liip_theme.redirect_fallback%</argument>
</service>
</services>
</container>
13 changes: 12 additions & 1 deletion Tests/UseCaseTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
use Liip\ThemeBundle\EventListener\ThemeRequestListener;
use Liip\ThemeBundle\Controller\ThemeController;
use Liip\ThemeBundle\ActiveTheme;
use Symfony\Component\Routing\Route;
use Symfony\Component\Routing\Router;

/**
* Bundle Functional tests.
Expand Down Expand Up @@ -133,9 +135,13 @@ public function testThemeAction($config, $assertion, $hasAlreadyACookie = true)
$request = $this->getMockRequest('cookie');
}

$router = $this->getMockBuilder(Router::class)
->disableOriginalConstructor()
->getMock();

$controller = false;
if ($config['load_controllers']) {
$controller = new ThemeController($activeTheme, $config['themes'], $config['cookie']);
$controller = new ThemeController($activeTheme, $config['themes'], $config['cookie'], $router, $config['redirect_fallback']);
}

$listener = new ThemeRequestListener($activeTheme, $config['cookie'], $device);
Expand Down Expand Up @@ -178,6 +184,7 @@ public function dataProvider()
// all-in Controller wins over Cookie and Autodetection
array(
'themes' => array('default', 'controller', 'cookie', 'autodetect'),
'redirect_fallback' => 'homepage',
'active_theme' => 'default',
'autodetect_theme' => true,
'load_controllers' => true,
Expand All @@ -194,6 +201,7 @@ public function dataProvider()
array(
array(
'themes' => array('default', 'controller', 'cookie', 'autodetect'),
'redirect_fallback' => 'homepage',
'active_theme' => 'default',
'autodetect_theme' => true,
'load_controllers' => true,
Expand All @@ -210,6 +218,7 @@ public function dataProvider()
array(
array(
'themes' => array('default', 'controller', 'cookie', 'autodetect'),
'redirect_fallback' => 'homepage',
'active_theme' => 'default',
'autodetect_theme' => true,
'load_controllers' => false,
Expand All @@ -226,6 +235,7 @@ public function dataProvider()
array(
array(
'themes' => array('default', 'controller', 'cookie', 'autodetect'),
'redirect_fallback' => 'homepage',
'active_theme' => 'default',
'autodetect_theme' => false,
'load_controllers' => true,
Expand All @@ -242,6 +252,7 @@ public function dataProvider()
array(
array(
'themes' => array('default', 'controller', 'cookie', 'autodetect'),
'redirect_fallback' => 'homepage',
'active_theme' => 'default',
'autodetect_theme' => false,
'load_controllers' => true,
Expand Down