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

Add option to allow Twig template to override rendering of legacy requests #264

Merged
merged 3 commits into from Mar 18, 2013

Conversation

@joekepley
Copy link
Contributor

joekepley commented Mar 16, 2013

See https://jira.ez.no/browse/EZP-20576
See also http://share.ez.no/forums/ez-publish-5-platform/module-page-not-found-error

This is a small change that allow the Symfony/Twig stack to render the pagelayout for requests handled through the LegacyKernelController, reducing the workload required in developing a Twig-based site by remove the need for duplicating pagelayout development on both stacks.

The legacy ezpkernelweb is modified to pass back module_result as an attribute. The LegacyKernelController has been extended with an option to specify a legacy layout template in the same way that the default layout is specified in the parameters.yml. If specified, the module result information would be passed to this template for rendering rather than using the full output from the legacy stack.

An example parameters.yml would look like this:

parameters:
    secret: mysecrethere
    ezpublish_legacy.view.default_layout: BlendPartialContentBundle::pagelayout.html.twig
    ezpublish_legacy.view.legacy_layout: BlendPartialContentBundle::legacylayout.html.twig

And a sample legacylayout.html.twig:

{% extends noLayout ? viewbaseLayout : "BlendPartialContentBundle::pagelayout.html.twig" %}
{% block pagetitle %}{% spaceless %}
    {% for segment in module_result.path %}
        {{ segment.text }}
        {% if loop.last != true %} - {% endif %}
    {% endfor %}
{% endspaceless %}{% endblock %}
{% block content %}
    {{ module_result.content|raw }}
{% endblock %}

By utilizing pagelayout.html.twig, this means that pages like user/login and 'module not found' use the pagelayout provided by Twig instead of the eZTpl. Therefore, while work still needs to be done in the tpl to customize these pages, none of it is duplicate work from the Twig templates.

@lolautruche

This comment has been minimized.

Copy link
Contributor

lolautruche commented Mar 16, 2013

Very nice one @joekepley 👍, thanks !
This is somewhat related to EZP-20518 since it touches ezpublish_legacy.view.default_layout, which needs to be configurable by siteaccess.

@@ -31,16 +31,19 @@ class LegacyKernelController
*/
private $kernel;

private $legacyLayout;

This comment has been minimized.

Copy link
@lolautruche

lolautruche Mar 16, 2013

Contributor

Please provide PHPDoc here, with little description + expected type.

@@ -31,16 +31,19 @@ class LegacyKernelController
*/
private $kernel;

private $legacyLayout;

/**
* @todo Maybe following dependencies should be mutualized in an abstract controller
* Injection can be done through "parent service" feature for DIC : http://symfony.com/doc/master/components/dependency_injection/parentservices.html
* @param \Closure $kernelClosure
* @param \Symfony\Component\Templating\EngineInterface $templateEngine

This comment has been minimized.

Copy link
@lolautruche

lolautruche Mar 16, 2013

Contributor

Don't forget to update constructor's phpDoc with the new argument.

);
$moduleResult = $result->getAttribute('module_result');

if ($this->legacyLayout)

This comment has been minimized.

Copy link
@lolautruche

lolautruche Mar 16, 2013

Contributor

CS : missing WS within parenthesis

This comment has been minimized.

Copy link
@lolautruche

lolautruche Mar 16, 2013

Contributor

I'd rather prefer an isset() test, assuming $this->legacyLayout default value is null instead of false.

@joekepley

This comment has been minimized.

Copy link
Contributor Author

joekepley commented Mar 16, 2013

Good comments, I'll make those adjustments. The related pull on legacy is:
ezsystems/ezpublish-legacy#576

I considered making this siteaccess-configurable, but wanted to be consistent with default_layout. Ideally this config could be moved in concert with EZP-20518

{
return $this->render(
$this->legacyLayout,
array('module_result'=>$moduleResult)

This comment has been minimized.

Copy link
@lolautruche

lolautruche Mar 16, 2013

Contributor

CS : missing WS within parenthesis + before/after =>

array('module_result'=>$moduleResult)
);
}
else

This comment has been minimized.

Copy link
@lolautruche

lolautruche Mar 16, 2013

Contributor

else is not needed as it is implicit. just return the default value 😃

@@ -0,0 +1,3 @@
parameters:
#Override this with a Twig template definition to have Twig render legacy pagelayouts

This comment has been minimized.

Copy link
@lolautruche

lolautruche Mar 16, 2013

Contributor

CS : 4 spaces for indentation + WS needed after #

@@ -0,0 +1,3 @@
parameters:
#Override this with a Twig template definition to have Twig render legacy pagelayouts
ezpublish_legacy.view.legacy_layout: false

This comment has been minimized.

Copy link
@lolautruche

lolautruche Mar 16, 2013

Contributor

I'd prefer nullas a default value (set as ~ in YAML syntax).

@lolautruche

This comment has been minimized.

Copy link
Contributor

lolautruche commented Mar 16, 2013

@joekepley : EZP-20518 should be addressed in a few days, but we might pull this PR in before and rework the SA based config. What do you think @andrerom @dpobel ?

@lolautruche

This comment has been minimized.

Copy link
Contributor

lolautruche commented Mar 16, 2013

Note that Travis fail is not related to this PR.

@joekepley

This comment has been minimized.

Copy link
Contributor Author

joekepley commented Mar 17, 2013

** NOTE: Do not merge this request without EZP-20518 **
Since this isn't considered by legacy_mode, it causes problems with the admin if it's in use. (the default null setting will not have an effect).

Let me know if I should modify LegacyKernelController to add a check for legacy_mode, or if it would be better just to merge this in with the work for 20518.

@lolautruche

This comment has been minimized.

Copy link
Contributor

lolautruche commented Mar 17, 2013

Right, please add this legacy_mode check. Makes sense. Thanks !

@joekepley

This comment has been minimized.

Copy link
Contributor Author

joekepley commented Mar 17, 2013

Given the complexity of the constructor now, it may make more sense for LegacyKernelController to be parented by eZ\Publish\Core\MVC\Symfony\Controller\Controller in the CoreBundle. Would that be a good refactor, or does it introduce an unwanted dependency?

@lolautruche

This comment has been minimized.

Copy link
Contributor

lolautruche commented Mar 18, 2013

@joekepley Please don't add a dependency on CoreBundle's controller as it is made to be a base for 3rd party controllers. Constructor is not that complex, you should see some in the API 😄 !

@lolautruche

This comment has been minimized.

Copy link
Contributor

lolautruche commented Mar 18, 2013

Looks good enough to me now 👍

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Mar 18, 2013

+1, good job guys :)

lolautruche added a commit that referenced this pull request Mar 18, 2013
Add option to allow Twig template to override rendering of legacy requests
@lolautruche lolautruche merged commit acaa763 into ezsystems:master Mar 18, 2013
1 check failed
1 check failed
default The Travis build failed
Details
@lolautruche

This comment has been minimized.

Copy link
Contributor

lolautruche commented Mar 18, 2013

Thanks @joekepley !

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Mar 19, 2013

Regression:

Argument 3 passed to eZ\Bundle\EzPublishLegacyBundle\Controller\LegacyKernelController::__construct() must be an instance of eZ\Bundle\EzPublishLegacyBundle\Controller\ConfigResolverInterface, instance of eZ\Bundle\EzPublishCoreBundle\DependencyInjection\Configuration\ChainConfigResolver given, called in /var/www/apache2php53/ezp5/ezpublish/cache/prod/ezpublishProdProjectContainer.php on line 1195 and defined in /var/www/apache2php53/ezp5/vendor/ezsystems/ezpublish-kernel/eZ/Bundle/EzPublishLegacyBundle/Controller/LegacyKernelController.php on line 51

Reported in: https://jira.ez.no/browse/EZP-20590

@lolautruche

This comment has been minimized.

Copy link
Contributor

lolautruche commented Mar 19, 2013

Fixed in 65a8754

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Mar 19, 2013

Thanks @lolautruche :)

@dpobel

This comment has been minimized.

Copy link
Contributor

dpobel commented Mar 28, 2013

FYI, the setting added in this pull request will be renamed so that it's possible to set this option by siteaccess and to make it consistent with the one related to the pagelayout used while rendering content in the legacy.
See #277

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.