Follow-up to #2447049: Add a render filter to twig

Problem/Motivation

We have {{ render_var(foo) }}, but {{ render(foo) }} would be so much nicer for the use cases that make sense (more like set tags or other use cases, {{ render_var(foo) }} is the same as {{ foo }}).

Proposed resolution

Add render in addition to render_var. Keep render_var for BC.

Remaining tasks

  • Patch
  • Review

User interface changes

N/A

API changes

New function available in Twig templates: render

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task, small functional addition
Issue priority Normal, helps with some use cases
Prioritized changes This is not a prioritized change for the beta phase.
Disruption Not disruptive at all for core or contrib, but potentially disruptive for Symfony applications that use Drupal 8 services as there is a namespace conflict on 'render'.
Files: 
CommentFileSizeAuthor
#9 interdiff.txt3.12 KBCottser
#9 2455233-8.patch2.65 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,707 pass(es). View

Comments

Cottser’s picture

Assigned: Unassigned » Cottser
Issue summary: View changes

Working on this, by the way :)

joelpittet’s picture

Status: Active » Reviewed & tested by the community

Looks good.

joelpittet’s picture

Status: Reviewed & tested by the community » Active

Just kidding, making vuvuzela

Cottser’s picture

Assigned: Cottser » Unassigned
Issue summary: View changes
Status: Active » Needs review
Issue tags: -Needs tests
FileSize
1.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,264 pass(es), 0 fail(s), and 1 exception(s). View
2.39 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,269 pass(es). View

Patch!

Edit: By the way I don't know if it's cool to use t() in tests or not.

Cottser’s picture

FileSize
2.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,258 pass(es). View
529 bytes

twig_theme_test dependency no longer needed for the test, in an earlier version I did a whole hook_theme() and template and everything.

joelpittet’s picture

Status: Needs review » Needs work

Ok for real this time;)

Has tests

  1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -69,6 +69,9 @@ public function setLinkGenerator(LinkGeneratorInterface $link_generator) {
    +      // render and render_var do the same thing. render_var is only kept for
    

    Should the filters be quoted or something in the docs to distinguish them from other words?

  2. +++ b/core/modules/system/src/Tests/Theme/TwigFunctionTest.php
    @@ -0,0 +1,57 @@
    +      '#template' => '{% set cats = render(animals.some_cats_i_know) %}{{ cats }}',
    ...
    +        'some_dogs_i_know' => [
    

    Who left the dogs out?

joelpittet’s picture

+++ b/core/modules/system/src/Tests/Theme/TwigFunctionTest.php
@@ -0,0 +1,57 @@
+          '#title' => t('Some cats I know'),
...
+          '#title' => t('Some dogs I know'),

The answer is actually don't put the t function in here because we aren't testing it:) Minimum viable code to test the feature.

The last submitted patch, 4: 2455233-2-tests.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
FileSize
2.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,707 pass(es). View
3.12 KB

Incorporates #6 and #7, thanks for the reviews :)

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

That looks like the ticket. Has tests, tests only fails in #1 as it should, very simple addition for TX/DX. Beta evaluation is in the Issue Summary.

Fabianx’s picture

Just food for thought, the reason render was changed to render_var, was that Symfony already has a 'render' function:

http://symfony.com/doc/current/reference/twig_reference.html#functions

So we have a name clash here. As the symfony thing is not a filter, it is less of a nameclash.

Maybe we need to use drupal_render? (which is known from D7 at least) as name?

Fabianx’s picture

Status: Reviewed & tested by the community » Needs review
Cottser’s picture

Status: Needs review » Reviewed & tested by the community
Related issues: +#2097735: Twig: Make it possible to show blocks, containers and regions

Thanks for that @Fabianx, didn't know about that. For others that didn't know, the Symfony render is used to embed controllers: http://symfony.com/doc/current/reference/twig_reference.html#render. I'm also excavating a related issue to this discussion that talks about a use case for a Symfony-esque render in Drupal-land as a data point.

Personally I think it's OK for us to have our own render function in Twig because so far our rule has been not to change too much from the upstream Twig library (only additive whenever possible in our TwigExtension) but Symfony's Twig bundle is not upstream, and I think already our url and potentially others are already different than Symfony's.

One of the reasons I like render is people might think to use it based on their D7 templates, and I don't think we can ignore that fact. drupal_render is in my experience not known by themers that live in templates all day, but render is known.

We already have |render in so consistency should rule and since the naming conflict is not in Twig itself but in Symfony's Twig bundle, I'm fine with going ahead here, so tentatively setting back to RTBC.


Just as a note, when working on this issue I discovered Twig does have a useful fallback when you call a nonexistent function:

Twig_Error_Syntax: The function "render" does not exist. Did you mean "render_var" in "core/themes/bartik/templates/page.html.twig" at line 1 in Twig_ExpressionParser->getFunctionNodeClass() (line 553 of core/vendor/twig/twig/lib/Twig/ExpressionParser.php).

(You can see that by adding {{ render(content) }} to your active theme's page.html.twig.

Fabianx’s picture

Assigned: Unassigned » alexpott
Related issues: +#1873442: Refactored core Twig integration

Assigning to alexpott for core committer feedback:

#1873442: Refactored core Twig integration renamed render to render_var for this exact reason.

If we rename it back now its a regression to our Symfony integration and while I am not a fan (for various reasons), I don't think its good to undo a commit of the opposite effect.

Cottser’s picture

Basically if this won't fly, we should still add the tests and probably rename the just-added render filter to render_var as well IMO.

I don't really understand the practicalities of the Symfony "integration".

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

Why do we have the expectation that our twig functions and filters won't clash with https://github.com/symfony/TwigBridge? If we do have that expectation things seem really fragile.

It seems even render_var() is a namespace clash given:

new \Twig_SimpleFunction('render_*', array($this, 'renderFragmentStrategy'), array('is_safe' => array('html'))),

From https://github.com/symfony/TwigBridge/blob/e848d30c148ceb0a0dc573bba383d...

I talked to @Fabianx about this in IRC - he said:

I was under the impression we supported symfony running while Drupal is running with the same Kernel.

I not sure I understand what this means and whether or not this is a reasonable expectation.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

This fine to go in, symfony should use render_esi, render_[etc.] instead when they need it or provide a symfony drupal bridge to use symfonyRender instead.

As there are ways of symfony projects using twig bridge to circumvent this and as usually a strategy is chosen, this is fine to go in.

alexpott’s picture

I still feel odd about reverting a change that the lead developer of twig implemented.

Fabianx’s picture

So some more background:

The main concern was especially about the DrupalTwigExtension in regards to the automatic adding of passing all renderable output through 'twig_render_var'.

Because the NodeVisitor would also run for Symfony integrated code, it would have called the Symfony 'render' then (if the extension was registered afterwards).

Also this was at a time where the function was registered always in twig and not in a pluggable extension, so maybe Symfony now does not load the twig extension at all anymore ...

However in this patch 'render' is only added as a convenience / consistency function, so it would at least be possible to override all contrib/ custom/ themes templates with one that has no 'render' function in it.

Given our coding standards advise to use the filter |render, this is likely not too big a problem either.

Edit:

So this is not actually reverting the original change, but adding a conflicting function name back into our namespace.

alexpott’s picture

I'm leaning towards closing this as won't fix - but before I do I want to talk with @Cottser on IRC.

Fabianx’s picture

Issue summary: View changes
Cottser’s picture

Assigned: alexpott » Unassigned
Status: Reviewed & tested by the community » Postponed

Had a quick chat with @alexpott in IRC, let's postpone for now. The Twig parser will tell people about render_var (#13), it's not a super nice name but at least we avoid conflicts and see how this shakes out. Potentially we could add this in a later release.

mgifford’s picture

What is this postponed on? It isn't clear to me from @Cottser's message.

joelpittet’s picture

Version: 8.0.x-dev » 8.1.x-dev

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.