Integrate Twig into core: Implementation issue

Small moves.

Tagging.

I haven't fully tested this yet but I saw that #1290694: Provide consistency for attributes and classes arrays provided by template_preprocess() just got committed.

Reading the patch, some questions came up:

Can twig() have a more descriptive name? It's also setting a 'cache' key to two different values.

    $twig = new Twig_Environment($loader, array(
        'cache' => DRUPAL_ROOT . '/compilation_cache',
        'cache' => FALSE,
        // @TODO: REMOVE ME!!!!!!!!
        'autoescape' => FALSE,
    ));

Also can that @TODO link to a relevant d.o issue? That looks rather important.

Where is twig_extension() going to get used? Should it get called from twig_theme() which is current hard-coding '.twig' ?

You can't remove twig_extension(), it's necessary for the theme engine to pick up .twig files, $extension_function = $theme_engine . '_extension'; in theme(). The fatals are IMO a bot fluke .

This brings back twig_extension().

There a lot of logic and looping in theme_item_list() that needs to get replicated in template_preprocess_item_list() for this to work. The previous patches were rendering a lot of "Array" into the markup.

I've done a copy-paste refactorof theme_item_list() to get template_preprocess_item_list() working with item_list.twig. However, I suspect this patch will fail testing because template_preprocess_item_list() will run also before theme_item_list(). So should we be taking that out now since I imagine it's coming out eventually anyway? Or should template_preprocess_item_list() just check if global $theme_engine is twig or not? I don't want to introduce a dependency on a global though. chx, do you have something in mind for that problem?

This patch also adds the wrapping div and title element present in theme_item_list to item_list.twig. I don't know if those were intentionally excluded. If so, I think that should be the topic of a separate issue.

This is not how this should be done, twig has the looping logic itself, we would like to see preprocess die eventually.

Great, the preprocess feels sub-optimal. Can you be more specific as to the Twig-way to get this working? The patches prior to the #11 print this:

<ul class="Array" Array>
      <li class="" >Array</li>
      <li class="" >Array</li>
      <li class="" >Array</li>
      <li class="" >Array</li>
    </ul>

It should print

<div class="item-list">
  <ul  class="pager">
    <li class="pager-current odd first" >1</li>
    <li class="pager-item even" ><a rel="last" title="Go to page 2" href="/aggregator/sources/1?page=1">2</a></li>
    <li class="pager-next odd" ><a rel="last" title="Go to next page" href="/aggregator/sources/1?page=1">next ›</a></li>
    <li class="pager-last even last" ><a rel="last" title="Go to last page" href="/aggregator/sources/1?page=1">last »</a></li>
  </ul>
</div class="item-list">

BTW, I am testing against Aggregator module which has a pager using item_list. To see this, I'm adding an RSS feed, running cron which imports stories, and going to /aggregator/sources/1

That's really weird. If you change twig to cache to a directory you can go into the generated php code and add a dump for those things, what's in those arrays?

The "Array" inside of <li> is because of the structure that's passed to theme('item_list'). Each item is an array with 'data' and (optionally) 'children.' See http://api.drupal.org/api/drupal/core%21includes%21theme.inc/function/th...

The "Array" inside of <ul> is because drupal_attributes() wasn't getting called anywhere on $variables['attributes'].

The link.twig needed a drupal_attributes() call.

Odd, I'm running tests locally and not getting those fails locally Block, Forum or Update modules. I haven't tried the rest yet. I thought clean urls were the problem but I ran the Block test with the new index.php/some/path structure and it still passed.

I don't think drupal_attributes() is necessary anymore, see http://drupal.org/node/1290694#comment-6273540 for more details.

chx or Jen, do you know how that should be implemented? Should all calls to theme_link() be turning $options['attributes'] into an Attributes object? Or can we do that directly in the twig file?

I'm concerned about theme_link() being the first example. That's the one that's treated specially by l() for performance reasons, and I don't think we want to make Stark needlessly slow. May I suggest theme_image() instead?

In order to initialize attributes for things that are currently theme functions, I suggest making template_preprocess_image() (if you accept my suggestion of using 'image' as the first example) call template_preprocess($variables, 'image') as its first line. The initializations done there, including initializing $variables['attributes'] is stuff we'll want to make available to all twig templates. We may need to micro-optimize that function a bit more before we can make theme() call it for every theme hook, but in the interim, most theme functions, including theme_image(), are not so frequently called as to make their preprocess function call template_preprocess() a problem.

Also, I unpostponed #1700382: Replace remaining references to drupal_attributes() with new Attributes() in case someone wants to work on that.

Or, if we want an example that demonstrates how twig should interact with check_plain(), there's also theme_datetime().

This is the first attempt to integrate twig into core.

This includes the twig engine and node.twig and datetime.twig as examples.

Limitations

• * autoescape is not enabled and doing so would need to touch almost all of core (followup)
• * to escape use {{ unsafe|e }} for now
• * caching is not enabled for now
• * strict variables are disabled for now

Strenghts

• * Functions available from templates are limited to hide, show, render, unset, url and t as filter
• * Very nice and easy templates
• * It just works (tm)

Explanation of functionality

The engine is straightforward besides some specialties that are needed to work around a twig limitation.

• * twig() is used as singleton to instantiate the TwigEnvironment
• * twig_render() is a wrapper around render() that can render strings, objects and render arrays
• * twig_hide() / twig_show() are wrappers around hide / show that do not return the content.
• * twig_render_template() renders the template via the TwigEnvironment

There are two special cases, which we need for Drupal and for which we are extending twig:

• * a) Render render arrays automatically on demand
• * b) Pass variables by reference

Render automatically

To render arrays automatically on demand every Twig_PrintNode (something that would be output) is wrapped in a call to Drupal\Core\Template\TwigReferenceFunctions::twig_render.

Internally twig converts:

{{ content.links }}

to

        if (isset($context["content"])) { $_content_ = $context["content"]; } else { $_content_ = null; }
        echo Drupal\Core\Template\TwigReferenceFunctions::twig_render($this->getAttribute($_content_, "links"));

Twig render detects that what it got from getAttribute is a render array and renders it.

In order to inject the function call, Twig is extended with TwigNodeVisitor.

Passing by reference

getAttribute above returns a value and not a reference. Such first hide / show and partial rendering were not working.

To solve this two things are done:

The variables passed to twig are encapsulated within TwigReference objects (which subclasses ArrayObject for performance).

Once getAttribute accesses the links key in TwigReference, TwigReference wraps a reference to the key in another TwigReference.

That way all twig ever is seeing is objects and such references are hold correctly.

To now get a real reference the Magic Drupal\Core\Template\TwigReferenceFunctions::my_function is used, which calls the getReference method on the TwigReference and calls the real function my_function with all arguments converted to array objects - if applicable.

The usage of a magic __callStatic class allows extending twig ny PHP functions without having to know that one needs to get a reference to the parameter first. Such the system is totally transparent to the called PHP function.

( The class was originally written when twig had much more functions it did support besides hide, show, render, unset. Now the &getReference could be also called directly in the wrapper functions though unset currently does not need one ... )

Fun? Fun! and Motivation

I think twig is lots of fun and seemingly really complicated templates get a piece of cake. To motivate another WIP example:

item-list.twig:

<div class="item-list">
  {% if title %}
  <h3> {{ title }} </h3>
  {% endif %}
  <{{ type }} class="{{ attributes.class }}" {{ attributes }}>
  {% for item in items %}
    <li {{ item.attributes }} >{{ item }}
    {# Append nested child list, if any. #}
    {% if children in item %}
        {% include "item-list" with {'items': item.children, 'attributes': item.children_attributes} %}
    {% endif %}
    </li>
  {% endfor %}
  </{{type}}>
</div>

And now compare this to:

http://api.drupal.org/api/drupal/core!includes!theme.inc/function/theme_...

Thanks goes to chx and jenlampton for mentoring!

Screenshot

#27 nice write-up as documentation
good to see it inside a summary - very nice documentation BTW

But why do we need integrate only two templates in this issue?
we have already much more converted and successfully working templates in sandbox

* autoescape is not enabled and doing so would need to touch almost all of core (followup)

One click option

* caching is not enabled for now

It just a one click option in twig.engine and it works like a charm when I`d test it - we just need to decide where the cache should be stored and create workaround for regenerating it

* strict variables are disabled for now

One click option

As for me - it looks like we need follow-up issue here for twig admin UI with making this options configurable via UI

* twig() is used as singleton to instantiate the TwigEnvironment

#1777532-9: Extend Twig_Loader_Filesystem instance to get .twig templates paths with hierarchy for theme dependencies looks like we have to decide this or other workaround

The fabolous reference magic is totally Fabian's work, I really like it. I never heard of exchangeArray before, that's truly handy! I thought ArrayObject was almost useless.

I can't RTBC this because it contains code written by me here and there. The visitor mostly and some small plumbing. I asked fabpot in July to review and fix the node visitor and he helped us by adding new Twig_Node(array($node->getNode('expr'))), $node->getLine()) to the new Twig_Node_Expression_Function part. So the visitor is guaranteed to be good :)

Edit: I mean, we should credit fabpot in here and we should ask FabianX who else.

#29 we are adding only two templates because we want to move rapidly. Once this is in we can send a rain of conversion issues in parallel .

#31 i got it, thanks!

+++ b/core/themes/stark/templates/node/node.twigundefined
@@ -0,0 +1,112 @@
+ * Available variables:
+ * - $title: the (sanitized) title of the node.

As i remember we should not use $syntax for variables in template help - it has to be just syntax

tagging

Here's an initial pass through. Overall I'm really impressed how little code is actually in here to make things work, it looks great, and it's frankly a relief to see this patch since there's not massive amounts of time left and I'm sure we'll find some issues as we get into the conversions.

+++ b/core/themes/engines/twig/twig.engineundefined
@@ -0,0 +1,172 @@
+/**
+ * Convert into a TwigReference object.
+ *
+ * This is needed to workaround a limitation
+ * of twig to only provide variables by value.
+ *
+ * By saving a reference to the original array
+ * the variable can be passed by reference.
+ *
+ * Only complex variables that are no attributes
+ * are converted.
+ *
+ * @see TwigReference
+ * @see TwigReferenceFunctions
+ *
+*/
+function twig_convert_to_reference(&$elements, &$original) {
+  $needs_selfreference = FALSE;
+
+  foreach ($elements as $key => $val) {
+    if (!is_array($val) || $key[0] == '#') {
+      continue;
+    }
+    if (!is_numeric($key)) {
+      $needs_selfreference = TRUE;
+    }
+  }
+  if ($needs_selfreference) {
+    $elements = new TwigReference();
+    $elements->setReference($original);
+  }

Is this worth making an upstream feature request for?

+++ b/core/themes/engines/twig/twig.engineundefined
@@ -0,0 +1,172 @@
+/**
+ * Singleton for the twig environment
+ *
+ * This is used for rendering templates with twig.
+ */
+function twig() {
+  $twig = &drupal_static(__FUNCTION__);

This seems like it could be a class that's put into the DIC. The DIC is more or less the new drupal_static() now.

+++ b/core/themes/engines/twig/twig.engineundefined
@@ -0,0 +1,172 @@
+    $twig = new Twig_Environment($loader, array(
+        'cache' => DRUPAL_ROOT . '/compilation_cache',
+        // @TODO Maybe enable cache, once rebuild has been sorted.
+        'cache' => FALSE,
+        // @TODO: Remove in followup issue.
+        'autoescape' => FALSE,
+        // @TODO: Remove in followup issue to handle exceptions gracefully.
+        'strict_variables' => FALSE,

Would be good to open the follow-up issues (or move them to the core queue) and link to them from the issue summary. I'm assuming we'll absolutely have to cache this to get acceptable performance since Twig is designed to work like that.

+++ b/core/themes/engines/twig/twig.engineundefined
@@ -0,0 +1,172 @@
+function twig_render($arg) {
+  // Keep Twig_Markup objects intact to prepare for later autoescaping support
+  if ($arg instanceOf Twig_Markup) {
+    return $arg;
+  }
+
+  if (is_string($arg) || (is_object($arg) && method_exists($arg, '__toString'))) {
+    return (string) $arg;
+  }
+
+  // This is a normal render array.
+  return render($arg);

If you pass something invalid, like an object that's not an instance of Twig_Markup and doesn't have a __toString method, then this will return NULL but otherwise fail silently. Should we through an exception or just let it fail horribly on the (string) cast instead?

+++ b/core/themes/stark/stark.infoundefined
@@ -3,4 +3,5 @@ description = This theme demonstrates Drupal's default HTML markup and CSS style
diff --git a/core/themes/stark/templates/node/node.twig b/core/themes/stark/templates/node/node.twig

diff --git a/core/themes/stark/templates/node/node.twig b/core/themes/stark/templates/node/node.twig
new file mode 100644
index 0000000..0986a52

index 0000000..0986a52
--- /dev/null

--- /dev/null
+++ b/core/themes/stark/templates/node/node.twigundefined

+++ b/core/themes/stark/templates/node/node.twigundefined
+++ b/core/themes/stark/templates/node/node.twigundefined
@@ -0,0 +1,112 @@

@@ -0,0 +1,112 @@
+{#
+ /**

Why adding these templates here instead of converting the ones provided by modules? Stark is supposed to represent Drupal's default output, which means no templates. So i'd expect to see the default node.tpl.php changed (or duplicated with a twig version until it can be removed), rather than put into Stark like this.

Leaving at needs review since it needs more reviews.

> Is this worth making an upstream feature request for?

No, it's a render array specific thing. All the problems we have come from the fact that render (and consequently show and hide) get their arguments by reference and change them.

> I'm assuming we'll absolutely have to cache this

In twig-speak yes but in fact we will need to get the php loader functionality we added for dic and twig to work with it.

> If you pass something invalid, like an object that's not an instance of Twig_Markup and doesn't have a __toString method, then this will return NULL but otherwise fail silently. Should we through an exception or just let it fail horribly on the (string) cast instead?

It won't fail silently at all as the default is to pass on to render which expects an array so it'll fatal out pretty soon. We can add an array typehint to render() be more explicit.

Passing NULL is valid for type hinting, so it's going to fail somewhere in render, can we just remove the __toString() check then so it fails right there?

In twig-speak yes but in fact we will need to get the php loader functionality we added for dic and twig to work with it.

Yep. Is there an issue for this against the sandbox? If not I'm happy to open one.

As for using our php writer mechanisms, here's fuel for a followup: We instantiate the Twig_Environment class now, instead we need our own class which extends Twig_Environment and overrides writeCacheFile. As this moves into the DIC as catch asked, then we can just pass in a Reference to a a php loader service.

But, followup. Just wanted to show it's doable and not hard.

#40: drupal-twig-in-core-1696786-40.patch queued for re-testing.

#40 look good for me

Nope it doesn't. We still need to move the twig static into the DIC.

+++ b/core/themes/engines/twig/twig.engineundefined
@@ -0,0 +1,172 @@
+function twig() {
+  $twig = &drupal_static(__FUNCTION__);

this one?
Are there any documentation that can be helpful to do that?
I`m not familiar with DIC workaround yet

ok, i`d found one http://symfony.com/doc/current/book/service_container.html

I have a working DIC version and will commit shortly.

Re-add tags ...

New patch:

• DIC in
• node.twig fixed
• Fixed twig_render to throw Exception on non-printable object
• Rebased on newest core

Still to be done:

• Wait for #1697854: Allow modules to provide both .twig and .tpl.php templates temporarily until twig is the default engine to get in
• Move templates to core/modules/node/node.twig and core/templates/theme.inc/datetime.twig
• Call datetime template in hook_theme of core/includes/render.inc
• Open followup issues in core for all @TODOs

Setting to needs review, because this needs more reviews ... and I want test bot to test this.

#48: core-Integrate-twig-into-core-1696786-48.diff queued for re-testing.

Adding "Needs tests" tag as per http://drupal.org/node/1697854#comment-6568622 (#1697854: Allow modules to provide both .twig and .tpl.php templates temporarily until twig is the default engine).

@TODO here:

• Open Followup issues for the @TODOs in the code and link to it in issue queue
• Merge #1697854: Allow modules to provide both .twig and .tpl.php templates temporarily until twig is the default engine
• Add Tests for #1697854: Allow modules to provide both .twig and .tpl.php templates temporarily until twig is the default engine
• Move templates to core/modules/node/node.twig and core/templates/theme.inc/datetime.twig
• Call datetime template in hook_theme of core/includes/theme.inc

This patch combines the patch in comment #18 on #1697854: Allow modules to provide both .twig and .tpl.php templates temporarily until twig is the default engine into the one in #48.

UPDATE: worth noting that we need to later remove parts of this patch: #1805592: Remove static double theme engine code once all module provided .tpl.php files are converted to twig and twig is default This was originally noted in comment #18 on the other issue.

Forgot to pull the latest... there was a merge conflict in core/lib/Drupal/Core/CoreBundle.php

Forgot to pull the latest... there was a merge conflict in core/lib/Drupal/Core/CoreBundle.php

Thanks jwilson3 for moving this forward. Note: most of #52 needs to be done yet.

Just for myself:

+++ b/core/includes/theme.incundefined
@@ -229,6 +229,12 @@ function _drupal_theme_initialize($theme, $base_theme = array(), $registry_callb
+  // @todo Make twig the default engine and remove this

Needs follow-up. Create issue to make twig the default engine with postponed status.

+++ b/core/lib/Drupal/Core/Template/TwigFactory.phpundefined
@@ -0,0 +1,57 @@
+    // @TODO: Maybe we will have our own loader later.

Needs follow-up, move http://drupal.org/node/1777532#comment-6569348 to Core.

+++ b/core/lib/Drupal/Core/Template/TwigFactory.phpundefined
@@ -0,0 +1,57 @@
+        // @TODO Maybe enable cache, once rebuild has been sorted.

Needs follow-up. Make use of DIC and the functionality in http://drupal.org/node/1747970.

+++ b/core/lib/Drupal/Core/Template/TwigFactory.phpundefined
@@ -0,0 +1,57 @@
+        // @TODO: Remove in followup issue.
+        'autoescape' => FALSE,

Create followup meta issue to move Core to auto escape everywhere. Will need code like:

$vars['css'] = new Drupal_Safe_Markup($vars['css'])

or

{{ css|raw }} in template. But this needs to be discussed, etc.

+++ b/core/lib/Drupal/Core/Template/TwigFactory.phpundefined
@@ -0,0 +1,57 @@
+        // @TODO: Remove in followup issue to handle exceptions gracefully.
+        'strict_variables' => FALSE,

Followup: Should be enabled, but the Exception thrown by the Twig Code is very hard, might want to handle Exceptions more gracefully. Needs discussions.

+++ b/core/themes/stark/templates/node/node.twigundefined
@@ -0,0 +1,110 @@
+  @todo: might be a good idea to remove the id attribute, because if that gets
+<article id="node-{{ node.nid }}" class="{{ attributes.class }} clearfix" {{- attributes }}>

Follow-up: Not related to the twig.engine, but should open issue in core for that tag with Twig and theme cleanup or so.

Was surprised to see that this issue wasn't listed on the issue summary on #1788918: [GTD] [META] Prepare for Twig main engine core inclusion until December 1, so I added it. Catching up on current work on the Twig in core effort is challenging!

A (recent, but historic) posts involved here, should be required reading: #1537050: [meta] Should we keep / improve multiple theme engine functionality?.

Also found this one: #1678808: Defining a new theme engine with templates without .tpl.[template extension] breaks template suggestions. Could be slightly relevant?

Notes: Drupal_Safe_Markup , no need, that exists already and is called Twig_Markup.

#60 , I closed the theme engine issue. The other I have no idea.

Updated issue summary.

Add follow-up issues

F'up issues

add f'ups

• Open Followup issues for the @TODOs in the code and link to it in issue queue
• Move templates to core/modules/node/node.twig and core/templates/theme.inc/datetime.twig
• Call datetime template in hook_theme of core/includes/theme.inc

Little progress in sandbox d8-core branch. Now on to writing tests.

modules/system/lib/Drupal/system/Tests/Theme/TwigReferenceUnitTest.php has landed in sandbox:

http://drupalcode.org/sandbox/pixelmord/1750250.git/commit/fb2d987

Need more tests for .twig and .tpl.php support.

Here is the new full patch that includes tests and also merges #1678808: Defining a new theme engine with templates without .tpl.[template extension] breaks template suggestions and fixes it. Tests are also added.

Antoine Lafontaine should be also credited for that.

This IMO is good to go. I wanted to remove the new function_exists call from theme() but phptemplate_render_template doesn't exist so you can't.

Order of f'ups

Oh :( hate to do this but there's a bunch of code style violations in the tests, like if (isset($context["content"])) { $_content_ = $context["content"]; } else { $_content_ = null; }

Ran through Coder, fixed all but the CamelCase warnings:

b/core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTest.php: @@ -187,4 +187,19 @@:
 +2: [style_camel_case, normal] do not use mixed case (camelCase), use lower case and _
 +9: [style_camel_case, normal] do not use mixed case (camelCase), use lower case and _

rewrote summary

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigReferenceUnitTest.php
@@ -0,0 +1,144 @@
+   *  Helper function to somehow simulate Twigs getAttribute function

Maybe the two preceding spaces before comment text?

From the test details:

[00:20:05] Command [php -l -f './core/modules/system/lib/Drupal/system/Tests/Theme/TwigReferenceUnitTest.php' 2>&1] failed
  Duration 0 seconds
  Directory [/var/lib/drupaltestbot/sites/default/files/checkout]
  Status [255]
 Output: [PHP Warning:  Unterminated comment starting line 144 in ./core/modules/system/lib/Drupal/system/Tests/Theme/TwigReferenceUnitTest.php on line 144

Warning: Unterminated comment starting line 144 in ./core/modules/system/lib/Drupal/system/Tests/Theme/TwigReferenceUnitTest.php on line 144
PHP Parse error:  syntax error, unexpected $end, expecting T_FUNCTION in ./core/modules/system/lib/Drupal/system/Tests/Theme/TwigReferenceUnitTest.php on line 145

Parse error: syntax error, unexpected $end, expecting T_FUNCTION in ./core/modules/system/lib/Drupal/system/Tests/Theme/TwigReferenceUnitTest.php on line 145
Errors parsing ./core/modules/system/lib/Drupal/system/Tests/Theme/TwigReferenceUnitTest.php].
[00:20:05] Encountered error on [syntax], details:
array (
  '@filename' => 'core/modules/system/lib/Drupal/system/Tests/Theme/TwigReferenceUnitTest.php',
)
[00:20:05] Review complete. test_id=356323 result code=6 details=Array
(
    [@filename] => core/modules/system/lib/Drupal/system/Tests/Theme/TwigReferenceUnitTest.php
)

OK, let's see about this one. As chx pointed out, TwigReferenceObjectTest class also needed to be in its own file as per PSR-0.

+<!-- node.twig - proudly powered by twig engine -->

I understand the feeling behind this -- agreed, Twig is great! -- but is it neccessary, especially when looking back to it in, say D9. Then it will sound like "proudly powered by PHP template engine".

Perhaps file a followup issue to remove this line before release -- when the feelings have been calmed down?

#72 looks good for me with exeption we need follow-ups from #58
agreed with #73

@chx asked me to do a docs review, and who am I to refuse! :)

added steveoliver

added amateescu

Add more explanation to summary.

+++ b/core/lib/Drupal/Core/CoreBundle.phpundefined
@@ -55,11 +55,13 @@ public function build(ContainerBuilder $container) {
     $container->register('typed_data', 'Drupal\Core\TypedData\TypedDataManager');
-    // Add the user's storage for temporary, non-cache data.
     $container->register('lock', 'Drupal\Core\Lock\DatabaseLockBackend');

I also hate to do that, but the patch in #75 removes a comment that probably should not be removed.

I wrongly merged those lines in #64.

Done, no interdiff needed.

Thanks guys for seriously prettifying this!

Better title. We are not simply adding examples here.

Small nitpick, we're integrating twig as part of core; now it matches suggested commit message.

Removed: Needs tests as tests are there now.

#77: core-Integrate-twig-into-core-1696786-77.patch queued for re-testing.

This seems like it should be at least major, but it is a feature not a task.

@chx and @stevector: Even though theme_item_list isn't included in this example, I wanted to point you to its issue, as it could use some review: #1800726: Convert theme_item_list to item-list.twig

Per request updated documentation per coding-standards and changed to new \class policy for global non-namespaced objects:

Cosmetical changes in this patch:

* Removed use statements in favor of \class for global objects (per namespacing policy)
* Added missing @param, @return and class descriptions
* Added further extensive documentation

Non-cosmetical changes in this patch:

* Fixed the constructor for TwigReference to never store the array argument as internal reference or pass to the parent. The preferred way is to create the TwigReference object and then explicitly call setReference. TwigReference should be instantiated like this:

$x = new TwigReference();
$x->setReference($variable);

The reason to do this explicitly besides readability is that the PHP docs state:

public ArrayObject::__construct() ([ mixed $input [, int $flags = 0 [, string $iterator_class = "ArrayIterator" ]]] )

Such $input is passed by value according to the API documentation, however in reality it is passed by reference, but as that could be fixed in PHP / change any time I decided to create a new Interface that explicitly passes by reference and document this interface.

New patch and interdiff attached.

As the interdiff is besides the doc and cosmetical changes trivial and just removes a non-used code path (i.e. TwigReference(some-non-empty-val) was never called), I leave this RTBC.

#84 looks good
removing tag

I'm mixed about Twig engine. The templates get slightly cleaner, but at the cost of significant code complexity. Consider all the wrapping and abstracting we do in TwigReference and TwigNodeVisitor. None of that is needed with PHPTemplate. And how much simpler are templates? We still have if statements and hide() calls. But now we do it in a relatively unpopular format (.twig) which is rarely syntax checked or syntax highlighted in an IDE.

+++ b/core/lib/Drupal/Core/Template/TwigFactory.php
@@ -0,0 +1,86 @@
+    // @todo Maybe we will have our own loader later.
+    $loader = new \Twig_Loader_Filesystem(DRUPAL_ROOT);
+    $twig = new \Twig_Environment($loader, array(
+        'cache' => DRUPAL_ROOT . '/compilation_cache',
+        // @todo Maybe enable cache, once rebuild has been sorted.
+        'cache' => FALSE,
+        // @todo Remove in followup issue.
+        'autoescape' => FALSE,
+        // @todo Remove in followup issue to handle exceptions gracefully.
+        'strict_variables' => FALSE,
+    ));

IMO, it is not OK to disable cache in the initial patch. One of the main complexities of Twig is it is a templating language on top of a templating language. To overcome this speed problem, it caches to raw PHP on disk, which is fine. IMO we need some experience with this, before we commit to it. If compilation adds little new code and complexity, then there is no problem. If it does add complexity, we need to know that before we start converting a boatload of templates. We already committed the Twig engine to core without having Twig having proven its worth. IMO buck stops here. For implementing the compilation cache, we should consider using #1675260: Implement PHP reading/writing secured against "leaky" script.

+++ b/core/lib/Drupal/Core/Template/TwigFactory.php
@@ -0,0 +1,86 @@
+    $functions = array(
+      'hide' => 'twig_hide',
+      'render' => 'twig_render',
+      'show' => 'twig_show',
+      'unset' => 'unset'
+    );
+    $filters = array(
+      't' => 't'
+    );

Do we want contrib to be able to add own functions and filters? Seems worthwhile.

+++ b/core/lib/Drupal/Core/Template/TwigNodeVisitor.php
@@ -0,0 +1,53 @@
+  /**
+   * Implements Twig_NodeVisitorInterface::getPriority().
+   */
+  function getPriority() {
+    return 0;
+  }

Expand Doxygen a bit more please. No idea what priority is or why we hard code it.

+++ b/core/lib/Drupal/Core/Template/TwigFactory.php
@@ -0,0 +1,86 @@
+    // @todo Maybe we will have our own loader later.
+    $loader = new \Twig_Loader_Filesystem(DRUPAL_ROOT);
+    $twig = new \Twig_Environment($loader, array(
+        'cache' => DRUPAL_ROOT . '/compilation_cache',
+        // @todo Maybe enable cache, once rebuild has been sorted.
+        'cache' => FALSE,
+        // @todo Remove in followup issue.
+        'autoescape' => FALSE,
+        // @todo Remove in followup issue to handle exceptions gracefully.
+        'strict_variables' => FALSE,
+    ));

IMO, it is not OK to disable cache in the initial patch. One of the main complexities of Twig is it is a templating language on top of a templating language. As such, it needs to cache to raw PHP. If compilation adds little new code and complexity, then there is no problem. If it does add complexity, we need to know that before we start converting a boatload of templates. We already committed Twig engine to core without having proven its worth. I think the buck stops here. For implementation, we might consider using #1675260: Implement PHP reading/writing secured against "leaky" script

+++ b/core/lib/Drupal/Core/Template/TwigFactory.php
@@ -0,0 +1,86 @@
+    $functions = array(
+      'hide' => 'twig_hide',
+      'render' => 'twig_render',
+      'show' => 'twig_show',
+      'unset' => 'unset'
+    );
+    $filters = array(
+      't' => 't'
+    );

Do we want contrib to be able to add own functions and filters? Seems worthwhile.

+++ b/core/lib/Drupal/Core/Template/TwigNodeVisitor.php
@@ -0,0 +1,53 @@
+  /**
+   * Implements Twig_NodeVisitorInterface::getPriority().
+   */
+  function getPriority() {
+    return 0;
+  }

Expand Doxygen a bit more please. No idea what priority is or why we hard code it.

+++ b/core/modules/system/templates/datetime.twig
@@ -0,0 +1,31 @@
+<time class="{{ attributes.class }}" {{ attributes }}>{{ html?text:(text|e) }}</time>

Wow. Dunno what html?text:(text|e) could possibly mean. I bet I would know if it were written in PHP :)

+++ b/core/modules/system/tests/themes/test_theme_twig/template.php
@@ -0,0 +1,19 @@
+ * The confusing function name here is due to this being an implementation of
+ * the alter hook invoked when the 'theme_test' module calls
+ * drupal_alter('theme_test_alter').
+ */
+function test_theme_theme_test_alter_alter(&$data) {
+  $data = 'test_theme_theme_test_alter_alter was invoked';
+}

Thanks for the Doxygen. Classic function name.

+++ b/core/themes/engines/twig/twig.engine
@@ -0,0 +1,162 @@
+/**
+ * Implements hook_init().
+ */
+function twig_init($template) {
+  $file = dirname($template->filename) . '/template.php';
+  if (file_exists($file)) {
+    include_once DRUPAL_ROOT . '/' . $file;
+  }
+}

It is pretty weird that we include this file during hook_init(). Seems like we could do that when we initiatize the theme system.

+++ b/core/themes/engines/twig/twig.engine
@@ -0,0 +1,162 @@
+ * All complex variables that are no attributes are converted

s/no/not

+++ b/core/themes/engines/twig/twig.engine
@@ -0,0 +1,162 @@
+  // This is a normal render array.
+  return render($arg);

We typically reserve render() for use inside templates. We should use drupal_render() here instead. At some future date we could move to a render() method and deprecate drupal_render() function.

I'll add that the current Issue Summary here and in the Meta issue do not bother to justify why we need this. I think such a big change merits a careful analysis of what we gain, and how that exceeds the complexity cost.

Sorry, multiple tabs.

I'll add that the current Issue Summary here and in the Meta issue do not bother to justify why we need this. I think such a big change merits a careful analysis of what we gain, and how that exceeds the complexity cost.

Twig has been suggested and generally lauded since March here: #1499460: [meta] New theme system

This present issue deals more with implementation itself, not whether to choose Twig is a good idea or not. It seems to me that philosophical concerns shouldn't be a blocker for the patches here.

Wow. Dunno what html?text:(text|e) could possibly mean. I bet I would know if it were written in PHP :)

so you understand ternary in php, but the same in twig is a mystery all of a sudden?? :)

echo ($html)?$text:escape($text);

Lets discuss this :-).

I'm mixed about Twig engine. The templates get slightly cleaner, but at the cost of significant code complexity. Consider all the wrapping and
abstracting we do in TwigReference and TwigNodeVisitor. None of that is needed with PHPTemplate. And how much simpler are templates? We still have if statements and hide() calls. But now we do it in a relatively unpopular format (.twig) which is rarely syntax checked or syntax highlighted in an IDE.

I do think that:

{{ content }}
{{ content.links }}

is easier than echo render($variables['content'); at least for front-end people. They don't _need_ to know what it is, they can just use it.

We also can get auto escaping and there is a proof-of-concept of this in the twig_engine branch of the sandbox.

Besides this, this should be a decision that the front-end'ers of drupal do, not the backend'ers and there seems to be huge support of front-enders for this transition. Themes can still use .tpl.php if they chose to.

Do we want contrib to be able to add own functions and filters? Seems worthwhile.

Yes, as its a factory class, contrib can integrate with the DIC to provide their own extensions and functions.

Expand Doxygen a bit more please. No idea what priority is or why we hard code it.

This is the default value. According to Doc standards I understood it that functions should not be documented that are only implementing the default and inherited methods. As the parent class is abstract we _need_ to implement getPriority.

Wow. Dunno what html?text:(text|e) could possibly mean. I bet I would know if it were written in PHP :)

e is short for escape, we can use also the full escape syntax. We can also write it in the long form. I should have asked a frontender before.

Thanks for the Doxygen. Classic function name.

This was copied from the default theme_test theme, I probably copied too much of it. I'll fix that and remove the function as this does not need re-testing. Please consider opening up an issue against theme_test doxygen :-).

It is pretty weird that we include this file during hook_init(). Seems like we could do that when we initiatize the theme system.

This is _straight_ copied from phptemplate_init():

/**
 * Implements hook_init().
 */
function phptemplate_init($template) {
  $file = dirname($template->filename) . '/template.php';
  if (file_exists($file)) {
    include_once DRUPAL_ROOT . '/' . $file;
  }
}

Please file a separate issue for that.

s/no/not

Will be fixed.

We typically reserve render() for use inside templates. We should use drupal_render() here instead. At some future date we could move to a render() method and deprecate drupal_render() function.

render() is correct here, because :-) this is only called from templates. That is why the documentation for the function says:

/**
 * Wrapper around render() for twig printed output.
 * [...]
*/

So in _only_ this case this is correct, because this is like a template doing {{ render(content) }}

Change commit message

I do not think this should held back. The complexity Moshe dislikes is because of integrating the render system with Twig.

I am updating the issue summary for why but the big one is: autoescape.

#90 might be better read if the ternary operators has spacing. Also to complies with twig coding standards. Disclosure: I'm not well versed with twig yet.

Put one (and only one) space before and after the following operators: comparison operators (==, !=, <, >, >=, <=), math operators (+, -, /, *, %, //, **), logic operators (not, and, or), ~, is, in, and the ternary operator (?:):

Sorry accidentally changed the status to need work.

seriously this makes me smile a lot - this is the way to go forward if we wanna include "frontend" / Themers or whatever we call my kinda people.

can i do a +2 ?

reasons.

Folks, what we definitely don't need in this issue is a piling of +1s from front-end developers. Please articulate why Twig is valuable, why the complexity concerns that Moshe is highlighting are not worrisome for you, etc. Links to other places this has been discussed before are great, and can be added to the issue summary.

Also, I find it funny that we have a grudge with the complexity found in a 40kb patch when we are about to use a full blown, AST-using compiler written in a language most unfit to write such a compiler -- PHP, that is. Let me repeat my reasoning which I just put in the issue summary: in my not so humble opinion just having a secure theme system which has the additional bonus of liked by frontend people is enough to justify any complexity in PHP.

If you want I can get all the frontend people who are working on Twig template conversion to this issue to express their support but instead just look http://drupal.org/node/1750250/committers here, just look at the number of people you never heard and are enthusiastic about Twig enough to contribute a template or two.

Sorry x-post but I think the reasoning is enough.

agree with #97
Yes - today #84 TWIG version looks not so valuable like PHPtemplate after a bunch of years after 2004(?) and with hidden background from reviewers that are familiar with tpl.php syntax, but not familiar with much more simple .twig ...
Anyway as described in roadmap http://drupal.org/sandbox/pixelmord/1750250 this is a really initial patch
We have many templates converted already in sandbox http://drupalcode.org/sandbox/pixelmord/1750250.git/tree/0c6519399921a9b...

the syntax in .twig is much more preffered for front-end developers and this is the main point decided before
The good point is that all of the theme functions will be removed and moved into .twig files for making possible to change output at the design layer without php skills needed

Autosanitization, caching, compiling from .twig into .php that can easely be integtated with APC(or any other) caching system.

And of couse - no more @sql select@ code in tpl.php files
...

more reasons

more

Add remove of double engine code for modules once twig is default issue

Updated issue summary.

I did some little xhprof-ing with cache enabled:

Twig is neither slower nor faster than phptemplate. It is roughly the same currently.

Also: The auto escape filter of the sandbox does not have that big of an impact because __construct and __toString, which is needed to wrap already escaped strings within Twig_Markup objects are really really fast and not double escaping makes most of the filter a trivial pass thru. This is for "normal" pages ...

Regarding the importance of security and auto escaping:

I consider myself well versed of the need to escape strings and check output carefully, but a simple usage of {{ title }} instead of {{ label }} in node.twig was enough to create the XSS. And the same is true for node.tpl.php in current core.

If I copy over a template from Drupal 7 for node.tpl.php I suddenly have a XSS hole. Yep, that is a bug. The label change seems to have been done without thinking over the consequences for security of older templates as $title was safe in node.tpl.php since 2007. The label change probably assumed that $variables['title'] is not set, but it is afterwards by the preprocess. (more about this in bug report issue: #1810710: Remove copying of node properties wholesale into theme variables)

If Drupal 8 had shipped with a node.tpl.php with $title insecure (and no auto escaping), I would probably have been the first victim ;-).

This example just shows how easy it is even for a non-themer to accidentally introduce a XSS, such I would support the statement that 100% (minus rounding errors) of custom templates are insecure and auto escape is a feature that can make drupal more secure.

The twig_engine branch of the sandbox automatically escapes the {{ title }} and the site is safe.

I just finished a first integration of Twig's cache with drupal_php_storage(). It is quite simple to do so and works like a charm.

Here is the diff of the current work in the sandbox:

http://drupalcode.org/sandbox/pixelmord/1750250.git/commitdiff/1541e5810...

Further things for this should be discussed in: #1806546: Twig: Performance Testing / Enable twig cache by using the Drupal PHP Loading/Writing.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Updated issue summary.

Re: benchmarks, on my machine twig is really slower. The CPU goes up and it's eating more memory. Now, of course, this could be because I just told bartik that its engine is now twig, might be that loading two engines causes overhead, but I just want to pass along the figures. Or am I missing something else ?

single node:
Before: Page execution time was 85.92 ms. Memory used at: devel_boot()=1.19 MB, devel_shutdown()=4.75 MB, PHP peak=5 MB.
After: Page execution time was 101.46 ms. Memory used at: devel_boot()=1.19 MB, devel_shutdown()=5.56 MB, PHP peak=6 MB.

Frontpage with 10 nodes:
Before: Page execution time was 115.5 ms. Memory used at: devel_boot()=1.18 MB, devel_shutdown()=5.05 MB, PHP peak=5.25 MB.
After: Page execution time was 131.58 ms. Memory used at: devel_boot()=1.18 MB, devel_shutdown()=5.89 MB, PHP peak=6.5 MB.

Expanded on downsides

swentel: Are those benchmarks with compiled Twig files, or recompiling on each request? Because recompiling Twig on each request is definitely going to be slower since the compiler is non-fast, but also not what anyone should be using day to day.

@Crell - by reading the issue summary above, it seems that caching in this patch is disabled, so this is probably recompiling every time.

From the summary

The patch currently disables compilation cache and disables autoescape

It seems ironic to me to commit a patch where auto escaping is disabled by default too, while saying that we need this because of security. Of course, this is the first patch, this call be done in follow ups, but it concerns me a little though.

Added Changes section

Updated issue summary.

Shortened autoescape downside

updated summary with Community Benefits

Updated issue summary.

Given how relatively small the diff in #1806546: Twig: Performance Testing / Enable twig cache by using the Drupal PHP Loading/Writing is, why not merge that into this issue?

Auto escaping, I agree can be a follow up, because while that's one benefit of Twig, it's not the only one. Even if we didn't solve auto escaping integration with Drupal (though I'm confident we will), we can escape in preprocess functions or document that escaping is needed in the templates, both of which we already do in all released versions of Drupal.

xpost

Updated issue summary. PHP

Updated issue summary.

Updated issue summary.

Updated issue summary.

html?text:(text|e)

I was equally confused by this when I reviewed this some days ago. The problem is that it looks like "must be some special new Twig command or something", because it does not adhere to our coding standards for ternaries; i.e., it lacks spaces:

html ? text : (text|e)

That way the confusion is reduced to (text|e), and I'm not familiar with Twig's template interpreter yet, but I hope you guys studied it, and so I guess the parenthesis are required since it would otherwise interpret it as (html ? text : text)|e.

Template variable filters generally are a good thing and simplify a themer's interaction with templates, so I don't see anything wrong with this per se.

However, I am slightly confused why we need the escape filter in this case? Wasn't one of the main ideas behind Twig that it is more secure, removes the burden from themers to think about security, and generally tries to keep the internals out of templates? In other words: Shouldn't the preprocessor escape 'text' when needed already, so the template only ever sees 'text' and just simply uses it?

Updated issue summary.

However, I am slightly confused why we need the escape filter in this case? Wasn't one of the main ideas behind Twig that it is more secure, removes the burden from themers to think about security, and generally tries to keep the internals out of templates? In other words: Shouldn't the preprocessor escape 'text' when needed already, so the template only ever sees 'text' and just simply uses it?

Yes, I agree that the sample template could be changed to include a preprocess function and I think we have something else in the sandbox, but this example was explicitly requested by effulgentsia as an example for how to escape.

With auto-escape on this can change to either:

html ? (text|raw) : text

Or in case of a proper preprocess function it could do the clean up in preprocess.

And mortendk would probably write (if he needed to change the raw links for some reason:

{% if html %}
<span class="html-link">{{ text|raw }}</span>
{% else %}
{{ text }}
{% endif %}

Note that even if the preprocess already applied a "safe" filter on the text the double raw still works.

Reverted syntax downside. Last edit was not clear, and there is a corresponding item in Benefits.

Added corrections to downsides

Updated issue summary.

Glad to see progress being made on Twig. I still believe Twig could be a really compelling feature for Drupal 8.

My preference would be for this patch to be expanded before it gets committed. If we commit this patch as is, it would create at least one critical follow-up patch (e.g. missing compilation step) and presumably at least one major follow-up patch (eg. missing security step). We're constantly at our major and critical tasks thresholds, and I don't think Twig should be blocking official initiatives (eg. web services) right now. By being a bit more strict here, it helps us prioritize.

Another reason for this is that as the patch stands, it doesn't bring huge benefits to Drupal as the syntax simplifications alone don't seem important enough to warrant the extra code complexity. It would be best if we could demonstrate that Twig can be fast and secure as part of this patch.

Specifically, I think we need to include both the caching and the security components into this first patch. Would that be doable? I'm therefore setting this back to 'needs work'.

As mentioned at the top of my comment, I remain excited about Twig in core and encourage us to keep working on this patch. Great work!

Just to give my impression on the Twig syntax as someone who is not a front-end developer, but who taught Drupal Theming 101 off and on for about 5 years with Lullabot...

-<article id="node-<?php print $node->nid; ?>" class="<?php print $attributes['class']; ?>
+<article id="node-{{ node.nid }}" class="{{ attributes.class }}

Themers not having to understand the difference between PHP array and PHP object syntax is absolutely huge. We used to need to give a half hour presentation just on how to navigate through something like $node->field_image[0][LANGUAGE_NONE]->file['alt']['text'] or whatever. "Just use dots everywhere" is SO much easier to explain.

However, it does bring up a really important question, which is "how does someone know what all the 'dot-thingies' are under 'node'"? And also, for that matter, "How do I know what all the possible 'node' like thingies there are?" We can no longer toss a var_dump(get_defined_vars()); at the top of the node.twig file. Alex pointed me to http://twig.sensiolabs.org/doc/functions/dump.html ... do we have that extension? Cos having an equivalent to that is going to be absolutely critical to themers understanding what info they have to work with. The docs are great, as far as core goes, but that completely goes away the second you install even one contributed module (or, for that matter, disable a module from standard profile, like comment).

Also, on the debugging note, what happens when I put:

{{ bananas }} # invalid entry

or

{{ node.nid } # syntax error

...in my twig file? I'm going to assume that in the first case it doesn't output anything, and in the second case it leaves the text as-is since it didn't complete a parse, but not sure, and don't have time to test atm. Will try and do so this weekend. But it's important to understand how Twig performs error handling, cos it's the difference between having a completely frustrating experience and being able to quickly remedy any mistakes.

Back to syntax again...

 clearfix" {{- attributes }}>

That '-' sign was weird, and I thought it was a mistake at first. Alex explained that it's short-hand for "if the attributes variable is empty, don't print a space before it" Kind of a neat short-hand, but will take some explaining to people, I think.

Do we have some general Twig docs in core that we can point themers to to help them understand these kinds of nuances?

-  <?php print render($title_prefix); ?>
+  {{ title_prefix }}
...
-    <h2<?php print $title_attributes; ?>><a href="<?php print $node_url; ?>" rel="bookmark"><?php print $label; ?></a></h2>
+    <h2 {{- title_attributes }}>
+      <a href="{{ node_url }}" rel="bookmark">{{ label }}</a>
+    </h2>

Another huge improvement: Themers no longer have to understand when they print $foo and when they print render($foo). And yet...

+    {# We hide the comments and links now so that we can render them later. #}
+    {{ hide(content.comments) }}
+    {{ hide(content.links) }}
+    {{ content }}

...we still retain the benefits of the render API, in that we don't need to make a template file change if we add a new field to a heavily-themed content type template. Plus. Freaking. One.

However, one thing that I do find kind of confusing...

{{ foo }}

seems to always mean "print 'foo' out the screen.

{% foo %}

seems to always mean "do this PHP logic."

So why then is it:

 {{ hide(content.links) }}

and not

 {% hide(content.links) %}

?

Oh, one other minor nit:

-  <?php if (!$page): ?>
+  {% if page != true %}

is {% if !page %} not possible? That'd definitely be more intuitive, IMO.

Btw, here is the diff I uploaded for reviewing 8.x's node.tpl.php vs. this patch's node.twig, since the patch doesn't have the removal of node.tpl.php in it for some reason: https://gist.github.com/3880359

is {% if !page %} not possible? That'd definitely be more intuitive, IMO.

Even better:

{% if not page %}

Oh, cool, that works too. :)

However, one thing that I do find kind of confusing...

{{ foo }}
seems to always mean "print 'foo' out the screen.

{% foo %}

seems to always mean "do this PHP logic."

So why then is it:

 {{ hide(content.links) }}

and not

 {% hide(content.links) %}

?

We could add back easily the {% hide(xyz) syntax ( I removed it for sake of simplicity of the first patch. )

Twig itself is not totally clear on that.

The basic rule is:

{{ }} means PRINT THIS.
{% %} means statement.

So, from that understanding we should add hide / show tags to twig instead of hide / show functions as those do in twig not return something nor should print something.

Thoughts on that from anyone else? For the policy around our syntax additions?

Alex explained that it's short-hand for "if the attributes variable is empty, don't print a space before it" Kind of a neat short-hand, but will take some explaining to people, I think.

Just to clarify, it's not conditional on the content of the variable. A "-" on the left side of the twig tag means remove whitespace preceeding that tag, so "foo" {{- bar }} is identical to "foo"{{ bar }}. The "-" allows a space to exist in the template for human readability but get removed from the output markup. This is explained in more detail in http://twig.sensiolabs.org/doc/templates.html#whitespace-control.

Do we have some general Twig docs in core that we can point themers to to help them understand these kinds of nuances?

Let's not replicate the Twig project's documentation. But finding a place in Drupal docs where we can link to that, and add any Drupal-specific isms (like that the value of "attributes" always contains its own leading space when not empty, so templates shouldn't output an additional leading space) seems like a good thing to do.

@all that are interested in how twig is integrated with caches, please follow:

#1806546: Twig: Performance Testing / Enable twig cache by using the Drupal PHP Loading/Writing

I am using a git two-patches in one approach now using the basic patch here as base and rebasing on this base as needed.

I'll update this issue here once we have something that is RTBC over there.

It would be great if we could have the heavy discussion in the other issue: #1806546: Twig: Performance Testing / Enable twig cache by using the Drupal PHP Loading/Writing

Adding Drupal-specific Twig extensions should be fine, in general, but note that it increases the work to support Twig.js in contrib (since I know core won't), so let's not do it willy nilly.

As far as documentation goes, we should primarily point people back to the official docs: http://twig.sensiolabs.org/documentation

Our own extensions we should definitely document but for things that are not Drupal specific, single-source and push upstream. If we find those docs lacking, pull requests are welcome. :-)

Sorry, to be clear I wasn't advocating us re-writing http://twig.sensiolabs.org/documentation in Drupal. More making sure that links to that were prominently placed in places folks would be looking for it. The first one I can think of is themes/README.txt maybe.

About missing variables, when in strict mode which we want you get an exception for missing variables too so that typos are caught.

the first performance test
#1806546-12: Twig: Performance Testing / Enable twig cache by using the Drupal PHP Loading/Writing
+51% speed-up

@webchick @chx:

We cannot use twig strict mode directly as it removes some really nice performance optimizations, but we can add a semi-strict mode that gives a warning when a var is not defined (and drupal error messages are on). I personally find throwing of an Exception too much ...

I however would like to do that as a follow up.

Okay, let me summarize the TODOs for _this_.

Current TODOs

• Add {% hide / show %} syntax back
• Enable debug mode and allow dump function
• Finish #1806546: Twig: Performance Testing / Enable twig cache by using the Drupal PHP Loading/Writing which includes the performance optimizations and caching
• Discuss safe mode and show performance characteristics (up next after the above is done)

New Followups:

• Enable semi strict mode that warns on usage of undefined var to catch typos, etc.

Anything I am missing here?

Anything I am missing here?

Demonstrate benefits and risks with autoescape enabled.

Demonstrate benefits and risks with autoescape enabled.

That was what I meant with "Discuss safe mode and show performance characteristics (up next after the above is done)", but yes, thanks! :-)

* On popular request merged #1806546: Twig: Performance Testing / Enable twig cache by using the Drupal PHP Loading/Writing into main patch.

Note that auto reload of templates only works if either

a) debug is enabled (from next patch on the case for enabling "dump")
b) options['auto_reload'] is set in twig_environment (default "null", defaults to "debug" setting).

In case that auto_reload is not enabled a user will need to clear the cache to have TWIG compile new templates.

* Added debug functions so that:
** {{ dump(label) }} works or any other variable
** {{ dump(_context) }} works - useful for printing all available variables

A general production / development mode switch would be really helpful here as well ...

New patch including re-adding of {% hide / show coming shortly.

As of now Twig is neither faster nor slower than Core, see #1806546: Twig: Performance Testing / Enable twig cache by using the Drupal PHP Loading/Writing for details.

And here is the new patch, which is optimized for very fast execution of Twig Templates.

In fact, once "Declare render variables in PHP via hook_...() and remove superfluous twig_render() calls" is in the templates look _almost_ the same like the normal node.tpl.php in compiled form :-D. (Just accessing of array elements is done via getAttribute, but that was the idea of the whole exercise ... )

But that can be left as followup as already now Twig is as fast as core for rendering 100 nodes on /node.

Still @todo:

• Remove superfluous test_theme functions found by Moshe
• Remove typo found by Moshe
• Fix docs, docs, docs
• Open up follow-up for: Declare render variables in PHP and remove superfluous twig_render() calls
• Investigate webchick's questions in regards to syntax errors / compile time errors / runtime errors / undefined vars.
• Show autoescape mode benefits and risks.

For testbot

Fabianx told me on IRC that ab -n 100 was 583+-12.8 for Twig and 591+-10.1 for core. That's indeed about the same.

+++ b/core/lib/Drupal/Core/Template/TwigReference.phpundefined
@@ -0,0 +1,131 @@
+ * @see twig_convert_to_reference

+++ b/core/lib/Drupal/Core/Template/TwigReferenceFunctions.phpundefined
@@ -0,0 +1,90 @@
+ * @see twig_convert_to_reference
+ * @see TwigFactory
...
+   * @see twig_convert_to_reference()

+++ b/core/themes/engines/twig/twig.engineundefined
@@ -0,0 +1,133 @@
+ * @see twig_convert_to_reference

twig_convert_to_reference no longer exists

Have most todos cleaned up, new patch coming soon :).

Updated issue summary.

Done @todo:

• Remove superfluous test_theme functions found by Moshe
• Remove typo found by Moshe
• Open up follow-up for: Declare render variables in PHP and remove superfluous twig_render() calls
• Investigate webchick's questions in regards to syntax errors / compile time errors / runtime errors / undefined vars.

Follow-up here: #1815250: Type twig variables in PHP and remove superfluous render_var() and getAttribute() calls from the generated code

To answer webchicks questions:

Syntax errors like {% hide content.links) %}: Give a hard exception with lineno and filename in regards to the .twig file
Undefined vars like: {{ non_existing_var }} give a warning right now if being printed (further warnings can be done as part of follow-up of #1806538: Remove @todo about enabling strict_variables in Twig)

Again I was able to get the lineno and filename and it's also very easy:

  $msg = new \Twig_Error(t('@item could not be found in _context.', array('@item' => $item)));
  drupal_set_message($msg->getMessage(), 'warning');

Twig_Error takes care of finding the template in the call stack and finding the right line number in the original .twig file.

I attached an example screenshot of how this looks.

Remaining @todo:

• Fix docs, docs, docs
• Show autoescape mode benefits and risks.

We are continuing in #1712444: Twig: Activate autoescape mode for now to work on the integration of auto escape on top of current patch like we did with Performance / Cache.

New patch attached.

Added new followup

#133 nice error handling here
all other looks good for me
adding tag

DesignDolphin: Not to put too fine a point on it, but that ship has sailed. Twig has been in discussion since March and has broader support from the front-end community than any other front-end change in the last 7 years. All of your questions have already been answered elsewhere. Please do not drag this issue off topic.

Edit: check #1499460: [meta] New theme system

#136 for review such longterm patches good to see interdiff
please...

@DesignDolpin
Please
this topic is for patch and integration - please - make Your own follow up and discuss there.

I have reopened the meta issue for further discussion.

Right, I agree the "review" in this issue should be focused on the patch at hand.

For review of the meta concept of making the theme system better and where Twig fits into that, probably follow-up at #1499460: [meta] New theme system

EDIT: Link copy/paste fail. :P

Pretty sure that's the direction we're going. As Crell pointed out, "Twig has been in discussion since March and has broader support from the front-end community than any other front-end change in the last 7 years."

DesignDolphin: There's nothing inherent about feature request vs. task that means review the concept vs. review the patch. I'm not sure where you're getting that from. Normally, "concept" type issues are prefixed with "meta" (like #1499460: [meta] New theme system) but everything else is about reviewing code.

Now, let's please move any further off-topic questions/points to off-topic issues. :) We're already at 145+ comments here, and we're not nearly done with reviewing the code yet.

#136 looks good for me
reviewed document changes

Documented 'em.

#150 good to see all @todo for documentation filled with a proper info
looks good
removing tag

For anyone not following, #1818266: [meta] A secure theme system (with twig) is discussing the auto-escape behaviour (that Dries wanted included here before commit as well as performance).

Doing some housekeeping. AFAIK this doesn't need to be assigned to Dries right now. Feel free to do so again once it hits RTBC!

Re-rolled #150 for testing against core with DIC / removed one conflict.

Update issue summary

Added link to twig coding standards

It's show time!

This patch merges all feedback and lessons learned from both #1712444: Twig: Activate autoescape mode and v3 from #1818266: [meta] A secure theme system (with twig).

The auto-escape for twig in this patch is fully enabled. To prevent double-escaping all strings in $variables are marked as secure by twig engine, which is a valid assumption given that we continue to support non autoescaped phptemplate files for now.

Very detailed benchmark results of the best approach when #1815250: Type twig variables in PHP and remove superfluous render_var() and getAttribute() calls from the generated code is in, can be found here: http://drupal.org/node/1818266#comment-6648238.

Compiled template performance

Before we get to the benchmarks, another concern was how compiled twig files look like. Here are the links:

Compiled Template: https://gist.github.com/3950182
Auto-escaped Template: https://gist.github.com/3950401

After #1815250: Type twig variables in PHP and remove superfluous render_var() and getAttribute() calls from the generated code is in:

Optimized compiled TwigTemplate: https://gist.github.com/3950254
Auto Escaped Optimized Twig Compiled Template: https://gist.github.com/3950411

The ternary (?:) operator is not used by Twig, because of a PHP 5.3 bug that makes it slower than if / else when encountering big arrays.

Note: No one ever will see this compiled code and for production mode it could be optimized even more (temporary variables would need to be checked just once).

Benchmarks

The following benchmarks are for the current status quo for 100 nodes on /node. Benchmarks are with APC on and APC ClassLoader enabled as per msonnabaums suggestion:

How to read these benchmarks;

These benchmarks show the Wall-Time (wt), used CPU time (cpu) and the Memory Usage (mu) plus the Peak Memory Usage (pmu). They are run 1000 times and the minimum is taken. For each line there are four numbers: Run 1 time, Run 2 time, Diff and Diff %. So if it shows core..twig, run 1 = core, run 2 = twig and the difference is twig - core.

The report is created via XHProf tools. They have been re-run several times to make sure the found minimum is accurate. The server is completely dedicated and has no other load during the benchmarks. URLs to XHProf raw data can be provided on a case by case basis. Just contact me for that, please.

100 nodes - /node

=== core..twig compared (508ae1d8981fd..508ae4b4be51c):

wt  : 773,189|805,205|32,016|4.1%
cpu : 772,048|800,050|28,002|3.6%
mu  : 15,065,904|15,765,824|699,920|4.6%
pmu : 16,869,344|17,588,080|718,736|4.3%


=== twig..twig-autoescape compared (508ae2673e94f..508ae5153b2e7):

wt  : 804,246|842,969|38,723|4.8%
cpu : 800,050|840,052|40,002|5.0%
mu  : 15,766,688|15,814,568|47,880|0.3%
pmu : 17,589,336|17,651,920|62,584|0.4%


=== core..twig-autoescape compared (508ae1d8981fd..508ae5153b2e7):

wt  : 773,189|842,969|69,780|9.0%
cpu : 772,048|840,052|68,004|8.8%
mu  : 15,065,904|15,814,568|748,664|5.0%
pmu : 16,869,344|17,651,920|782,576|4.6%

EDIT: Above with 1000 runs now - was not different.

Note that #1815250: Type twig variables in PHP and remove superfluous render_var() and getAttribute() calls from the generated code will make this faster as shown in http://drupal.org/node/1818266#comment-6648238.

Patch attached and needs review!

#155: core-Integrate-twig-into-core-1696786-155.diff queued for re-testing.

To elaborate on #155:

The patch as attached from #155 currently makes Drupal around 9% slower when 100 nodes are rendered on /node.

This can be improved to 6% with #1815250: Type twig variables in PHP and remove superfluous render_var() and getAttribute() calls from the generated code and possibly further with a production / development switch.

Discussed this a bit with Fabian. This is an obvious performance degradation as it stands at the moment, however I'm reasonably confident we can claw it back. Also I really, really appreciate the amount of work that's gone into both performance analysis and actually optimizing based on that for this patch, this makes me confident that this work will continue to be done after commit.

For comparison, #1599108: Allow modules to register services and subscriber services (events) introduced around a 5% performance degradation, went in before the degradation or potential fixes had been properly discussed, and the issue that was supposed to fix it (#1706064: Compile the Injection Container to disk) actually didn't, instead it's being tracked in #1716048: Do not boot bundle classes on every request. Registering bundles ought to be close to free once we're doing it in a sane way, so by comparison 8% for a new rendering systems doesn't feel so bad. It's also a lot less than the 30-40% overhead we introduced in Drupal 7 when converting the node body to a field - for 30 nodes, not 100.

Since Twig was originally put forward, I've been assuming we can remove a lot of the work we currently do in preprocess preparing various versions of different variables, and rely on the Twig template compilation to handle that stuff instead. Currently all kinds of variables get prepared/sanitized in preprocess that might never even be printed in a template. I've opened #1825952: Turn on twig autoescape by default to discuss this some more. Most Drupal 7 sites spend a lot of time on preprocess, how much of that we can save is going to be hard to quantify until it's actually done though.

I've not reviewed the latest patch yet, however I'll try to do it over the next day or so (so no comment on it's RTBC-ness in general). Assigning to Dries since I think he should also have a look a this sooner rather than later.

Good to see here plan for benefits that can speed-up twig after roadmap done

As for me these points are described at frontend of our sandbox

1. Update core to use two different theme engines, & fallback to PHPTemplate: #1697854: Allow modules to provide both .twig and .tpl.php templates temporarily until twig is the default engine
2. Add simple example of Twig to core (patch: #1696786: Integrate Twig into core)
3. Convert the Bartik theme to Twig (patch: [])
4. Create a patch against each module in core to switch it from PHPTemplate to Twig
• aggregator (patch: )
• block (patch: )
• ...
• xmlrpc
5. Switch core to use Twig as the default theme engine - regression




6. Cleanup of previous preprocess functions (removal of extra vars) - speedup
7. Replace all renderables in core with Twig data objects - need testing
8. Remove render() from all twig templates - need testing - possible speedup
9. (Maybe) Remove render() from core - speedup

anyway we should commit RTBC patch with a small regression into core for Get This Done #1788918: [GTD] [META] Prepare for Twig main engine core inclusion until December 1 and for possibility to create all follow-ups

PS. looks like it is the point to connect to VDC initiative due to dynamic templating engine in views theme layer and possibility make it more deeper integration with Twig, so taging

Added merged issue

Updated issue summary. Add performance related issues

Updated issue summary. Typo fixed

I think the assignee change was unintentional.

I think this really needs more reviews. I'm going to try to do one later today myself (assuming the power doesn't go out on the U.S. east coast...)

Also, I agree with @DesignDolphin: 99% of core issues are about reviewing the concept and code together, not just the code. For bigger issues (like this one) that can be different, but there's no way most people will ever know that. Changing the issue title a bit should help clarify things.

Updated issue summary.

OK, here's a review. And for starters (before I forget), I'm totally impressed with all the work that went into this patch.

node.twig

I started by jumping into node.twig directly and playing around (figuring that's how 99% of people will encounter this). A couple comments:

1. +<article id="node-{{ node.nid }}" class="{{ attributes.class }} clearfix" {{- attributes }}>
   

   The node.nid and attributes.class syntax was really, really nice to see :)

   In the second part, I had no idea what the "-" was in front of the attributes. I had a few guesses which turned out to be (laughably) incorrect, then eventually read the Twig documentation and also saw comments earlier in this issue which pointed out that it strips whitespace on the left.

   This syntax choice by the Twig project is... uh, interesting :)

   I think it adds significantly to the complexity of the templates because it shows up in random places and therefore gives the impression that it's a lot more important than it actually is. Do we really need to use it in Drupal? (Where is the whitespace being added anyway that it needs to be stripped here - is Twig adding it itself?)

2. ** {{ dump(label) }} works or any other variable
   ** {{ dump(_context) }} works - useful for printing all available variables

   I tried playing with these, and the first worked nicely, but the second dumped garbage to the screen. I think what might be going on here is that the output of dump() is itself being auto-escaped?... oops :)

3. +  {% if page != true %}
   

   Somewhere above it was suggested this could be {% if not page %} instead? I agree it would be more readable. (Minor point, though.)

4. Overall, node.tpl.php felt like too simple of an example to get a real sense of how the Twig conversion would work.

   I'd be curious to see something like page.tpl.php also (which has t() calls, theme() calls, and more complex logic embedded inside of it, that kind of thing)... it's closer to a realistic example of what themers actually deal with on moderately complex websites. I suspect there's already code for page.twig out there somewhere, but it's not in this patch :)

5. Along the same lines I was very curious how Twig would deal with something like #953034-2: [meta] Themes improperly check renderable arrays when determining visibility (because as far as I know that's the only way to fix that issue and others like it without introducing unwanted side effects).

   After some playing around, I got the impression that this would work:

   {% set sidebar_first = render(page.sidebar_first) %}
   {% if sidebar_first %}
   

   I guess that's not too bad, but it doesn't look like an improvement over standard PHP either. I was a bit thrown off that I had to call render() directly and that using {{page.sidebar_first}} in that statement apparently didn't work.

Security

I'm concerned that there hasn't been a full discussion of the consequences of auto-escaping. I'm personally not yet convinced that the way this patch is using it actually improves security.

Sure, if you do something like {{ node.body.und.0.value }} it will save you from security issues, but if you do {{ node.body.und.0.safe_value }} (which is a better variable to use anyway), it will incorrectly escape your HTML. Your site will have bugs until you figure out that you need to do {{ node.body.und.0.safe_value|raw }} instead. Which is fine until you apply this "lesson learned" again and start doing {{ node.body.und.0.value|raw }} too... Oops, no more security.

In short, this doesn't save template authors from having to think about text filtering. They're still responsible for making security decisions, only we're now making things very confusing because everywhere else in Drupal you do it one way (explicitly escape text), and here in the template files you do it the exact opposite way.

Second, it's worth pointing out that {{ title }} is still an XSS hole even with this patch (#100 mentioned that as something which auto-escaping might fix, but I think the implementation changed since then). As mentioned in that comment, this is an independent bug (#1810710: Remove copying of node properties wholesale into theme variables), but the point is that stupid security mistakes in the preprocess layer still leak into templates even with this patch. And keep in mind that by removing PHP from templates, we're going to force theme developers to work more in the preprocess layer whenever they need to get complex things done, so there will be plenty of opportunities for them to introduce security mistakes there...

Options #1 from #1818266: [meta] A secure theme system (with twig) avoids these problems and could provide actual security in template files (notably, it also has nothing to do with Twig directly, although it does work much more nicely with something like Twig than it would otherwise). It is also harder and might not be possible to do correctly. But I'm worried that the current patch implements solution #3 from that issue as a means of meeting the stated "security" goal and getting this patch committed, but it might actually be better simply to do nothing for now. (Especially because this also seems to be responsible for a lot of the performance hit, plus perhaps the debug() issue above, etc.)

We don't necessarily need "better security" in order to use Twig anyway; there are other benefits too (but see more on that below).

Big picture

Overall, I see a lot of good things about Twig and this patch but I share some of the concerns @moshe weitzman raised above. I think one way to phrase that (in the context of this issue) is that the issue summary provides a list of proposed benefits but the current implementation doesn't really deliver on all of them (in particular, security and performance), nor is it obvious that it ever will. So we're potentially adding a lot of code complexity for a smaller number of benefits than originally assumed. (And notably, a smaller number than assumed by many people in #1499460: [meta] New theme system as well; although the validity of those assumptions was questioned early on in that issue, my impression is that most people seemed to conclude "Twig = security" when they evaluated things and made their decision to support it, even though it was never actually that simple.)

Again, Twig may be worth it regardless (and I'm 100% in favor myself provided that the more complete solution from #1818266: [meta] A secure theme system (with twig) can get in) but I think the tradeoffs right now are really a lot more subtle than they've been given credit for.

Other issues

I didn't really review the whole patch, but a few things stood out. In no particular order (and some are relatively minor):

1. +  if (!$element && $element !== 0) {
   +    return null;
   +  }
   

   NULL, not null. (Actually lots of these all around, including FALSE vs. false.)

2. -class Attribute implements \ArrayAccess, \IteratorAggregate {
   +class Attribute extends \Twig_Markup implements \ArrayAccess, \IteratorAggregate {
   

   Hm, this seems to embed Twig very deeply into Drupal... what about other theme engines? Maybe in practice it doesn't matter much.

3. +{#
   +  @todo: might be a good idea to remove the id attribute, because if that gets
   +  rendered twice on a page this is invalid CSS
   +  for example: two lists in different view modes.
   +#}
   

   I don't see why we need this comment here; that's a bug, but it's been around a long, long time (see #405270: Strange duplicate IDs during node preview and when a node is displayed multiple times on a page) so it has nothing to do with this patch. Having it there is kind of a distraction given how many people we expect to use this file.

4. +{# @todo Revisit once autoescape is enabled: http://drupal.org/node/1712444 #}
   +<time class="{{ attributes.class }}" {{ attributes }}>{{ html ? text : (text|escape) }}</time>
   

   So presumably it should have been revisited? :) That code looks like it's going to be incorrect otherwise.

it might actually be better simply to do nothing for now

I still haven't reviewed the patch, but based on discussion with FabianX about this yesterday I'd mostly come to this conclusion as well. This was part of the intention of opening #1825952: Turn on twig autoescape by default (we might not want to do what that issue suggests in the end, but we'll not want to go with the approach currently in the patch whatever happens, and I definitely want to cut down on preprocess bloat - for me that's the main benefit of using Twig at all).

@David Rothstein:

Thanks for the very extensive review.

Let me answer the questions / remarks

node.twig

1. {{ attributes }} vs. {{- attributes }}

This was mainly decided by themers, etc. as part of the twig initiative itself. The Attribute class adds the whitespace to allow to do:

<h2<? print $attributes 

>

?>

in PHP or

<h2{{ attributes }}>

in Twig, but Twig themers did not like it, so chose to use:

<h2 {{- attributes }}>

instead. I think this could be changed easily in the Attribute class and I don't know the background / reasoning for this choice.

I do agree though that twig work arounds a syntax problem here that could be solved directly. But I don't think you want to do this as part of this patch ...

2. Dumping variables

I tried playing with these, and the first worked nicely, but the second dumped garbage to the screen. I think what might be going on here is that the output of dump() is itself being auto-escaped?... oops :)

Uhm, the second worked for me. Adding a <pre>{{ dump(_context) }}</pre> makes this more readable.

And yes, there are Twig_Markup objects for strings in there, but it is not worse than reading a render array. I can also use the $GLOBALS variant here, but I chose not to do so, because I then need to justify the usage of $GLOBALS as part of this original patch, which gets into other discussions and need to create my own twig_escape_filter function to not decrease performance even more. (Each function call / creation of Twig_Markup object costs ...)

That said if you only want all variable names, you can use:

<pre>{{ dump(_context|keys) }}</pre>

3. Syntax

Done: I fixed it with easier syntax.

4. page.tpl.php

@todo: I will provide a cleaned up and QA'ed page.twig as GIST. What is currently in the sandbox is still WIP.

Themes checking visibility

To solve this I need the following issues in:

* #1815250: Type twig variables in PHP and remove superfluous render_var() and getAttribute() calls from the generated code
* Create a render in-memory cache for render arrays, so that they are not rendered twice

Then I can compile time optimize that render arrays are always run through twig_render() - even if they are not printed, which can then reduce the code to:

{% if page.sidebar_first %}
{{ page.sidebar_first }}
{% endif %}

This would be compiled roughly to:

if (twig_render($this->getAttribute($this->getContextReference($context, 'page'), 'sidebar_first'))) {
  print twig_autoescape(twig_render($this->getAttribute($this->getContextReference($context, 'page'), 'sidebar_first')));
}

so having render store the rendered output somewhere is a must for performance reasons.

I might be able to optimize this also to:

$_page_sidebar_first = render($this->getAttribute($this->getContextReference($context, 'page'), 'sidebar_first'));
if ($_page_sidebar_first) {
  print $_page_sidebar_first;
}

But I am still pondering the consequences and while for the former, I have the code ready, I have not for the latter though as we compile it and have all information, it is _possible_.

I however would have loved to keep this patch rather simple ... (uh, yes ...) and do some things as followups.

Security

Well, I did not want to open the can of worms for the $GLOBALS discussion, so #1 from "A secure theme system" was not an option for something _simple_.

Ideally we want template authors to only write:

node.body or node.field_my_nice_field and not having to deal with the other things at all. I can think both of a decorator (performance problem) or a getter called token, which works similar to how tokens work.

node.token.body (would call node->token('body')), but that is a separate discussion.

I can for sure leave auto escape out. That is one git revert away, but Dries wanted to see it in so ...

Big Picture

So we move "Performance" and "Security" out of Benefits to "Potential Benefits"?

However I agree: Yes, benefits could be stated more realistically based on the current situation.

Other issues

* NULL and FALSE fixed
*

Hm, this seems to embed Twig very deeply into Drupal... what about other theme engines? Maybe in practice it doesn't matter much.

Either Drupal commits to Twig or not. But if there is hesitation, I am happy to do:

  if ($arg instanceof Attribute || $arg instanceof AttributeBase) }
    return new Twig_Markup((string)$arg);
  }

Thats three lines of code added to twig_render, but I did not do it for performance reasons ...

* @todo removed from node.twig
* datetime.twig:

That code is a little strange now (and yes I agree that here the variables mark safe leads to a strange situation, but I think this is an edge case as escaping within templates should not be that often:

<time class="{{ attributes.class }}" {{ attributes }}>{{ html ? text|raw : text|escape }}</time>

is a universal version that works always with all autoescape and non-autoescape enabled.

I see that this is a little problem right now.

Actionable Items?

Besides the cleanup, which I did now (and is attached, also re-rolled) and the @todo to provide a cleaned up page.twig, is there anything actionable I can currently do to get this in?

Or is it just sit and wait?

It helps to actually add the patch ;-)

This one test really does not like the addition of more classes, but this cleanup would apply both to node.tpl.php and node.twig, so leaving as straight conversion from node.tpl.php for now.

testbot go!