Fix the engine [meta]

Adding Twig tag

For clarity, is #1781640: toolbar_pre_render array error a twig_engine issue or a front-end issue?

For clarity, is #1781640: toolbar_pre_render array error a twig_engine issue or a front-end issue?

Is this is_array_alike function just being suggested as an interim solution or has it been put forward as a patch for core?

updating component

2jenlampton
from #1779146: Refactoring theme_comment_block for TWIG

*edit* Please please please DO NOT REMOVE THEME FUNCTIONS. It will break everything other than the Stark theme.

How can we handle such issues?
roadmap needed...

#11 +1

No. The only theme that is running Twig at the moment is Stark. We haven't switched Bartik, or more importantly Seven, to use twig. If we remove a function like theme_form_element from core we'll have no admin forms anywhere in Drupal.

We need to be able to compare the output from PHPTemplate to the output from Twig to make sure that what we are doing is working. Removing theme functions will come at the very end of the process, after we get all core themes (and core itself) running Twig instead of PHPTemplate. But until we can confirm that all our templates actually render the correct content replacing working theme functions with broken template files is a very bad idea :)

@podarok I will post a roadmap on the sandbox node.

#13 looks like very important documentation issue!
Inserted into conversion how-to

This is a different approach to chx' 8.x branch to fix the engine. It uses the idea presented in #1759178: Strict warning: Only variables should be passed by reference in __TwigTemplate_58ad372b1c0487154aa1ce2022eb6eaf->doDisplay() and does not need to change all core.

This patch is against the frontend branch.

In general the idea is very easy:

Each "complex" array item is changed to include a "self-reference" like

  $foo = array( "bar" => array(0,1,3) );
  $foo['#writable_ref'] = &$foo; // Yes, that works!

With that idea the only thing functions called by twig that need references need to do is to replace the value with the real variable like:

 foreach ($arguments as $key => $val) {
   if (isset($val['#writable_ref']) && is_array($val['#writable_ref'])) {
     $arguments[$key] = &$val['#writable_ref'];
   }
  }

To not need to create a helper function for each of the functions twig wants to call I used a magic helper class instead, which is using a variation of the above code to call for example the render function with the argument as a reference to the real array.

What was changed:

* core/lib/Drupal/Core/Template/TemplateData.php was changed to call twig_render_template to reduce code duplication
* twig_render_template was altered to call twig_add_writable_self_reference like the idea by aquariumtap - I left the comment for now
* All $twig->addFunction / $twig->addFilter have been changed to indirectly call the functions via a helper class by prefixing it with Drupal\Core\Template\TwigReferenceFunctions::
** The same is true for the added hide( token
* Adding the Drupal\Core\Template\TwigReferenceFunctions class to "magically" provide the wrapper for the functions to call them by reference.

What does that get me?

* Apply the patch to frontend branch
* Add the following "PATCH"

diff --git a/core/themes/stark/templates/node/node.twig b/core/themes/stark/templates/node/node.twig
index f6dd3d5..1cc533e 100644
--- a/core/themes/stark/templates/node/node.twig
+++ b/core/themes/stark/templates/node/node.twig
@@ -105,10 +105,8 @@
 
   <div class="content" {{- content_attributes }}>
     {# We hide the comments and links now so that we can render them later. #}
-    {# @TODO uncomment this when http://drupal.org/node/1753676 is resolved
     {%  hide(content.comments) %}
     {%  hide(content.links) %}
-    #}
 
     {# @todo: remove this render call #}
     {{ render(content) }}

And see how the node display is totally correct and hide and render work _just_ like if it was passing references in the first place.

In comparison to chx approach this does _not_ need to fix all of core to switch from render arrays to render objects.

Next steps?

Need confirmation from someone from core if the self-reference would be okay for core with the #writable_ref attribute. If not this can be still a possibility to work on twig templates while core is being patched to change from render arrays to render objects.

Enjoy!

Best,

Fabian

#15 nice!

After some concerns by chx regarding references in FAPI arrays, this version does reference the "original" version and changes a copy.

With that the self references are only visible while the data is processed by twig, but any function called via our twig redirection function gets the original debuggable version without the references.

Interdiff and Diff attached.

#17 the logic itself looks reasonable for me

@todo - split filters and functions here

Setting status to "CNR", forgot to do that before.

I have done some functional testing on fabianx's patch from #17. This looks like some stellar progress, render() and hide() are now working in node.twig! Also has resolved the errors that were reported at #1759178: Strict warning: Only variables should be passed by reference in __TwigTemplate_58ad372b1c0487154aa1ce2022eb6eaf->doDisplay().

nice

After discussion with jenlampton and chx and some more testing by me, which uncovered some problems, I changed from writable references to TwigReference wrapper objects that are wrapping the array items on demand like a proxy.

A new patch is attached and it works perfect as the writable_references before, but has even less overhead. (I quickly checked with XHProf: Performance is no problem). Also an interdiff to the array solution is attached as well.

This patch is just for review by the community.

I continue working in the twig_engine branch and commit there directly. I have some nice ideas in regards to error reporting, etc. and some experimental features.

Please do not yet merge that one into front-end, yet.

I'd like to successively merge community reviewed commits and use a "rebase" workflow.

Thanks,

Fabian

+++ b/core/lib/Drupal/Core/Template/TwigReference.phpundefined
@@ -0,0 +1,44 @@
+
+class TwigReference extends ArrayObject { ¶

trailing whitespace

+++ b/core/lib/Drupal/Core/Template/TwigReference.phpundefined
@@ -0,0 +1,44 @@
+  }
+ ¶

trailing whitespace

+++ b/core/lib/Drupal/Core/Template/TwigReference.phpundefined
@@ -0,0 +1,44 @@
+  /**
+   * (PHP 5 &gt;= 5.0.0)<br/>
+

> ??????

Fixed problems in #23.

Other changes in this patch:

• Issue #1780730 by Fabianx: Change myrender to twig_render to use own namespace.
• Issue #1780730 by Fabianx: Make {{ content }} work by implementing render() in twig_render.
• Issue #1780730 by Fabianx: Use getter/setter functions for accessing TwigReference.

This means {{ content}}, {{ content.links }} etc. all work now.

However there are two broken templates currently:

filter-tips.twig and username.twig have errors currently.

You will get something like:

User error: "0" is an invalid render array key in element_children() (line 6251 of core/includes/common.inc).
User error: "lang" is an invalid render array key in element_children() (line 6251 of core/includes/common.inc).

These are template bugs and not engine bugs.

To be able to debug this easier you can checkout the feature branch: "feature--add-nice-debug-messages", which extends the compiler to add runtime debug information, which we need because render _can_ fail.

These changes can also be checked out directly from twig_engine branch in GIT.

That said: Are we "allowed" to clutter the twig_* namespace at all? Obviously we need it for some parts to implement hooks, etc.

However that can be easily reworked in a follow-up :-).

Still @todo here:

• * Remove render from functions and map it to twig_render instead to give use more control
• * Restrict the subset of functions available from twig (is drupal_render really necessary?)
• * Think about if we really want to implement a hide( tag instead of just a normal hide function.

Best,

Fabian

#24 looks good for me
do we need follow-up issues for template bugs here?

>> Merged into front-end!

Yes, lets open up several follow up issues now that this is merged.

For the username.twig:

http://pastebin.com/dAZFZecG

For the filter-tips.twig:

Remove {{ tips }} from first part of template.

All TODOs done here.

Testbot: You can close this now ;-).