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

Fix EZP-25960: Double layout using legacy /layout/set module and LegacyKernelController #59

Merged
merged 1 commit into from
Jul 18, 2016
Merged

Fix EZP-25960: Double layout using legacy /layout/set module and LegacyKernelController #59

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
25 changes: 23 additions & 2 deletions bundle/LegacyResponse/LegacyResponseManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use eZ\Bundle\EzPublishLegacyBundle\LegacyResponse;
use eZ\Publish\Core\MVC\ConfigResolverInterface;
use Symfony\Component\HttpFoundation\RedirectResponse;
use Symfony\Component\HttpFoundation\RequestStack;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\Security\Core\Exception\AccessDeniedException;
use Symfony\Component\Templating\EngineInterface;
Expand Down Expand Up @@ -44,11 +45,17 @@ class LegacyResponseManager
*/
private $legacyMode;

public function __construct(EngineInterface $templateEngine, ConfigResolverInterface $configResolver)
/**
* @var RequestStack
*/
private $requestStack;

public function __construct(EngineInterface $templateEngine, ConfigResolverInterface $configResolver, RequestStack $requestStack)
{
$this->templateEngine = $templateEngine;
$this->legacyLayout = $configResolver->getParameter('module_default_layout', 'ezpublish_legacy');
$this->legacyMode = $configResolver->getParameter('legacy_mode');
$this->requestStack = $requestStack;
}

/**
Expand All @@ -63,7 +70,7 @@ public function generateResponseFromModuleResult(ezpKernelResult $result)
{
$moduleResult = $result->getAttribute('module_result');

if (isset($this->legacyLayout) && !$this->legacyMode && !isset($moduleResult['pagelayout'])) {
if (isset($this->legacyLayout) && !$this->legacyMode && !$this->legacyResultHasLayout($result)) {
// Replace original module_result content by filtered one
$moduleResult['content'] = $result->getContent();

Expand Down Expand Up @@ -95,6 +102,20 @@ public function generateResponseFromModuleResult(ezpKernelResult $result)
return $response;
}

/**
* Checks if $result is already embedded in a layout.
* Typically checks if $moduleResult['pagelayout'] is set or if current request is using /layout/set/ route.
*
* @param ezpKernelResult $result
* @return bool
*/
public function legacyResultHasLayout(ezpKernelResult $result)
{
return
isset($result->getAttribute('module_result')['pagelayout'])
|| $this->requestStack->getMasterRequest()->attributes->get('_route') === '_ezpublishLegacyLayoutSet';
}

/**
* Generates proper RedirectResponse from $redirectResult.
*
Expand Down
7 changes: 7 additions & 0 deletions bundle/Resources/config/routing.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,10 @@ _ezpublishLegacyRest:
_controller: ezpublish_legacy.rest.controller:restAction
requirements:
path: .*

_ezpublishLegacyLayoutSet:
path: /layout/set/{path}
Copy link
Contributor

@andrerom andrerom Jul 5, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

does this work with uri SA? is that what requirements is about just below?

Copy link
Contributor Author

@lolautruche lolautruche Jul 5, 2016 •
edited by andrerom
Loading

Choose a reason for hiding this comment

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

does this work with uri SA

Yes, as we override default router to use semanticPathinfo. So this will always work.
The requirements is just to allow / for {path}

defaults:
_controller: ezpublish_legacy.controller:indexAction
requirements:
path: .+
2 changes: 1 addition & 1 deletion bundle/Resources/config/services.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ services:

ezpublish_legacy.response_manager:
class: %ezpublish_legacy.response_manager.class%
arguments: ["@templating", "@ezpublish.config.resolver"]
arguments: ["@templating", "@ezpublish.config.resolver", "@request_stack"]

ezpublish_legacy.controller:
class: %ezpublish_legacy.controller.class%
Expand Down
52 changes: 41 additions & 11 deletions bundle/Tests/LegacyResponse/LegacyResponseManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
use eZ\Bundle\EzPublishLegacyBundle\LegacyResponse;
use eZ\Publish\Core\MVC\ConfigResolverInterface;
use PHPUnit_Framework_TestCase;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\RequestStack;
use Symfony\Component\Templating\EngineInterface;
use ezpKernelResult;
use ezpKernelRedirect;
Expand Down Expand Up @@ -42,7 +44,7 @@ protected function setUp()
public function testGenerateResponseAccessDenied($errorCode, $errorMessage)
{
$this->setExpectedException('Symfony\Component\Security\Core\Exception\AccessDeniedException', $errorMessage);
$manager = new LegacyResponseManager($this->templateEngine, $this->configResolver);
$manager = new LegacyResponseManager($this->templateEngine, $this->configResolver, new RequestStack());
$content = 'foobar';
$moduleResult = array(
'content' => $content,
Expand All @@ -63,6 +65,23 @@ public function generateResponseAccessDeniedProvider()
);
}

public function testLegacyResultHasLayout()
{
$requestStack = new RequestStack();
$requestStack->push(new Request());
$manager = new LegacyResponseManager($this->templateEngine, $this->configResolver, $requestStack);
self::assertFalse($manager->legacyResultHasLayout(new ezpKernelResult()));

$resultWithLegacyLayout = new ezpKernelResult('', ['module_result' => ['pagelayout' => 'foo.tpl']]);
self::assertTrue($manager->legacyResultHasLayout($resultWithLegacyLayout));

$request = new Request();
$request->attributes->set('_route', '_ezpublishLegacyLayoutSet');
$requestStack->pop();
$requestStack->push($request);
self::assertTrue($manager->legacyResultHasLayout($resultWithLegacyLayout));
}

/**
* Tests response generation when no custom layout can be applied:
* - Custom layout provided, but in legacy mode
Expand All @@ -72,10 +91,11 @@ public function generateResponseAccessDeniedProvider()
* @param string|null $customLayout Custom Twig layout being used, or null if none.
* @param bool $legacyMode Whether legacy mode is active or not.
* @param bool $moduleResultLayout Whether if module_result from legacy contains a "pagelayout" entry.
* @param bool $isLayoutSetModule Whether current request is using /layout/set/ route.
*
* @dataProvider generateResponseNoCustomLayoutProvider
*/
public function testGenerateResponseNoCustomLayout($customLayout, $legacyMode, $moduleResultLayout)
public function testGenerateResponseNoCustomLayout($customLayout, $legacyMode, $moduleResultLayout, $isLayoutSetModule)
{
$this->configResolver
->expects($this->any())
Expand All @@ -92,7 +112,14 @@ public function testGenerateResponseNoCustomLayout($customLayout, $legacyMode, $
->expects($this->never())
->method('render');

$manager = new LegacyResponseManager($this->templateEngine, $this->configResolver);
$requestStack = new RequestStack();
$request = new Request();
if ($isLayoutSetModule) {
$request->attributes->set('_route', '_ezpublishLegacyLayoutSet');
}
$requestStack->push($request);

$manager = new LegacyResponseManager($this->templateEngine, $this->configResolver, $requestStack);
$content = 'foobar';
$moduleResult = array(
'content' => $content,
Expand All @@ -113,11 +140,12 @@ public function testGenerateResponseNoCustomLayout($customLayout, $legacyMode, $
public function generateResponseNoCustomLayoutProvider()
{
return array(
array(null, false, false),
array('foo.html.twig', true, false),
array('foo.html.twig', false, true),
array(null, false, true),
array(null, true, true),
array(null, false, false, false),
array('foo.html.twig', true, false, false),
array('foo.html.twig', false, true, false),
array(null, false, true, false),
array(null, true, true, false),
array(null, false, false, false, true),
);
}

Expand Down Expand Up @@ -149,7 +177,9 @@ public function testGenerateResponseWithCustomLayout($customLayout, $content)
->with($customLayout, array('module_result' => $moduleResult))
->will($this->returnValue($contentWithLayout));

$manager = new LegacyResponseManager($this->templateEngine, $this->configResolver);
$requestStack = new RequestStack();
$requestStack->push(new Request());
$manager = new LegacyResponseManager($this->templateEngine, $this->configResolver, $requestStack);

$kernelResult = new ezpKernelResult($content, array('module_result' => $moduleResult));

Expand Down Expand Up @@ -179,7 +209,7 @@ public function generateResponseWithCustomLayoutProvider()
public function testGenerateRedirectResponse($uri, $redirectStatus, $expectedStatusCode, $content)
{
$kernelRedirect = new ezpKernelRedirect($uri, $redirectStatus, $content);
$manager = new LegacyResponseManager($this->templateEngine, $this->configResolver);
$manager = new LegacyResponseManager($this->templateEngine, $this->configResolver, new RequestStack());
$response = $manager->generateRedirectResponse($kernelRedirect);
$uriInContent = htmlspecialchars($uri);
$expectedContent = <<<EOT
Expand Down Expand Up @@ -222,7 +252,7 @@ public function testMapHeaders()

// Partially mock the manager to simulate calls to header_remove()
$manager = $this->getMockBuilder('eZ\Bundle\EzPublishLegacyBundle\LegacyResponse\LegacyResponseManager')
->setConstructorArgs(array($this->templateEngine, $this->configResolver))
->setConstructorArgs(array($this->templateEngine, $this->configResolver, new RequestStack()))
->setMethods(array('removeHeader'))
->getMock();
$manager
Expand Down