Problem/Motivation

TwigReference Removal Problem/Motivation

Show/Hide Removal Motivation

The show and hide twig functions map to the equivalent Drupal PHP functions. The hide function prevents the specified child element of a renderable array from being printed along with the rest of the children by internally setting #printed => TRUE . The show function sets #printed => FALSE to allow printing or re-printing of the specified element.

While this sounds useful and has some practical use-cases, its overhead is quite large. The show and hide functions require TwigReference to wrap all arrays sent to Twig. Because of TwigNodeVisitor, TwigReference inadvertently breaks the twig raw filter. The broken raw filter prevents us from turning autoescape on by default #1825952: Turn on twig autoescape by default.

{% hide(content.links) %}
{{ content }}
{% show(content.links) %}
<nav>{{ content.links }}</nav>

Why both in one issue:

These are not easy to separate because TwigRefence's main reason for being is to support the show/hide feature of renderable arrays. So committing the removal of it would likely break show/hide functionality. And as you can see by the test fail in #67 , committing the without filter would fail the tests without TwigReference removal as well.

Proposed resolution

The show function is not used anywhere in core and is not needed. If we need to print a renderable array, we can just {{ print }} it twice.

@webchick gave me a hand here. This is all of the calls to show in contrib:

./atom_nzgovt/views_plugin_row_node_atom.inc:        show($node->content['links']);
./dcco/includes/common.inc:    show($element);
./oembed/oembed.module:      return show($element);
./rotoslider/theme/theme.inc:      show($element[$key]);

The hide function can be useful to exclude children from a renderable array.

The solution here is to remove show() and replace the hide function with a filter. This is an improvement because a filter always applies to an array with children. A filter is also more explicit and in context to what is happening.

This issue only removes show() and hide() from the context of Twig templates, the functions are still available in the PHP layer and preprocess functions (which are the only usage of show() above).

Example:

Before

{% hide(content.links) %}
{% hide(content.actions) %}
<article>{{ content }}<article>
...
<nav>{{ content.links }}</nav>
<nav>{{ content.actions }}</nav>

After

<article>{{ content|without('links', 'actions') }}<article>
...
<nav>{{ content.links }}</nav>
<nav>{{ content.actions }}</nav>

Remaining tasks

  1. Get feedback of use-cases that aren't solvable without show/hide in the template files.
  2. Remove 'show' and 'hide' twig functions.
  3. Replace 'hide' twig function calls in templates with the equivalent 'without' filter call.
  4. Remove TwigReference/TwigTemplate, their tests, and their reference passing from TwigNodeVisitor.
  5. Write tests for the 'without' filter.

API changes

Remove 'show' and 'hide' twig functions from inside templates.
The hide() function is replaced with a filter called 'without' which can be applied to a renderable array.

CommentFileSizeAuthor
#95 interdiff-89-94.txt956 bytesmartin107
#95 2114563-94-remove-twigreference-without-filter.patch38.48 KBmartin107
#94 interdiff.txt956 bytesstar-szr
#94 2114563-94.patch38.48 KBstar-szr
#89 interdiff.txt971 bytesjoelpittet
#89 2114563-89-remove-twigreference-without-filter.patch38.19 KBjoelpittet
#86 interdiff.txt3.31 KBjoelpittet
#86 2114563-86-remove-twigreference-without-filter.patch38.21 KBjoelpittet
#84 2114563-84-remove-twigreference-without-filter.patch38.27 KBjoelpittet
#84 interdiff.txt6.62 KBjoelpittet
#80 interdiff.txt6.91 KBjoelpittet
#80 2114563-80-remove-twigreference-without-filter.patch40.65 KBjoelpittet
#67 2114563-67-remove-twigreference-without-filter-with-tests-pass.patch37.99 KBjoelpittet
#67 2114563-67-without-filter-with-tests-fail.patch16.46 KBjoelpittet
#67 interdiff-tests.txt4.61 KBjoelpittet
#65 interdiff.txt7.89 KBjoelpittet
#65 interdiff-array.txt1.19 KBjoelpittet
#65 2114563-kill-show-hide-65.patch35.21 KBjoelpittet
#65 2114563-kill-show-hide-65-referenced.patch13.67 KBjoelpittet
#59 2114563-attributes-without-59.txt3.8 KBjoelpittet
#58 interdiff.txt3.86 KBjoelpittet
#58 2114563-kill-show-hide-58.patch13.02 KBjoelpittet
#49 interdiff.txt8.09 KBjoelpittet
#49 2114563-kill-show-hide-48.patch9.31 KBjoelpittet
#48 interdiff.txt1.33 KBjoelpittet
#48 2114563-kill-show-hide-48.patch9.31 KBjoelpittet
#37 interdiff.txt1.33 KBjoelpittet
#37 2114563-37-kill-show-hide.patch9.64 KBjoelpittet
#34 interdiff.txt1.01 KBjoelpittet
#34 2114563-34-kill-show-hide.patch9.67 KBjoelpittet
#24 interdiff.txt2.76 KBjoelpittet
#24 2114563-24-kill-show-hide.patch9.64 KBjoelpittet
#20 2114563-20-kill-show-hide.patch8.54 KBjoelpittet
#20 interdiff.txt661 bytesjoelpittet
#17 2114563-17-kill-show-hide.patch8.54 KBjoelpittet
#14 interdiff.txt8.82 KBjoelpittet
#14 2114563-14-kill-show-hide.patch12.79 KBjoelpittet
#7 2114563-kill-show-hide-in-twig.patch8.78 KBjoelpittet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

star-szr’s picture

Thanks for posting this @joelpittet, I really like this idea.

Just discussed this with @Mark Carver on the Twig call.

I think we can solve this really cleanly with set tags. http://twig.sensiolabs.org/doc/tags/set.html

So new code for example 1 without unset, (untested):

{% set links %}{{ content.links }}{% endset %}
{{ content }}
<nav>{{ links }}</nav>
markhalliwell’s picture

I definitely agree with removing hide/show. The above code in #1 I think makes a lot of sense. +1

star-szr’s picture

Adding related issues. Removing show()/hide() would actually vastly improve template inspection since we could remove TwigReference objects.

star-szr’s picture

I think this qualifies as cleanup and an API change.

joelpittet’s picture

Yay delicious murder! :)

Should we just remove it from twig (easier) or remove them from preprocess/prepare layer as well?

joelpittet’s picture

Quick note: show() is not used in core, only hide(). So still think unset() would be nice to have.

joelpittet’s picture

Just a test to see what we're dealing with.

joelpittet’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 7: 2114563-kill-show-hide-in-twig.patch, failed testing.

star-szr’s picture

@joelpittet - Yeah, we could just use a set tag and never use the resulting variable but that's wasted rendering time I think.

Maybe we create a custom Twig filter/function to just remove items from an array by key along the lines of your unset(content.links) example.

tim.plunkett’s picture

Issue summary: View changes
joelpittet’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 7: 2114563-kill-show-hide-in-twig.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
12.79 KB
8.82 KB

If I can get away with it, I'd prefer not to introduce unset like twig was meant to be... so if we can do it in preprocess... all the better. This is more sane errors too. but the way nodes get variables in content is so weird.

tim.plunkett’s picture

+++ b/core/modules/block/block.module
@@ -407,6 +407,20 @@ function block_list($region) {
+ * Prepares variables for block list templates.
+ *
+ * Default template: block-list.html.twig.
+ *
+ * @param array $variables
+ *   An associative array containing:
+ *   - form: A render element representing the form.
+ */
+function template_preprocess_block_list(&$variables) {
+  $variables['place_blocks'] = $variables['form']['place_blocks'];
+  unset($variables['form']['place_blocks']);
+}
+
+/**

+++ b/core/modules/block/templates/block-list.html.twig
@@ -9,16 +9,16 @@
-{% hide(form.place_blocks) %}
...
-    {{ form.place_blocks }}
+    {{ place_blocks }}

This seems like a pretty unfortunate DX (TX?) loss. Are we sure its worth it?

EDIT: See http://drupalcode.org/project/drupal.git/commitdiff/1c54559 for when we added this template. It removed the left/right logic from the form.

Status: Needs review » Needs work

The last submitted patch, 14: 2114563-14-kill-show-hide.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
8.54 KB

So in an effort to try and make more happy, I left hide as a filter to only 'exclude' children from printing. This seems to be a bit cleaner and may still allow the removal or replace of TwigReference.

Status: Needs review » Needs work

The last submitted patch, 17: 2114563-17-kill-show-hide.patch, failed testing.

joelpittet’s picture

#17 totally kills show from twig, turns hide into a filter and it doesn't call hide() php function so it doesn't get passed as a reference. The exception/fails are probably easy to deal with. And *crosses fingers* it should help us kill TwigReference which is the ultimate goal here. Still gives people who use hide() the feature still in a slightly cleaner way. Also core doesn't use show() at all at the moment in twig files, so I doubt it will be missed. Still consider changing the name of hide to something more intuitive like, exclude() or something. Let me know your thoughts.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
661 bytes
8.54 KB

That was easy to fix;)

Caveat, this fix doesn't currently go two deep... though it could if deemed necessary.

star-szr’s picture

Nice green patch. @joelpittet two levels within the render array?

tim.plunkett’s picture

This should likely get some profiling numbers.

  1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -53,14 +52,11 @@ public function getNodeVisitors() {
    -  public function getName()
    -  {
    +  public function getName() {
    

    If you're going to start fixing other things, this needs a docblock. Or just remove this hunk from the patch...

  2. +++ b/core/modules/node/templates/node-edit-form.html.twig
    @@ -15,11 +15,9 @@
    -{% hide(form.advanced) %}
    -{% hide(form.actions) %}
    ...
    -    {{ form }}
    +    {{ form|hide('advanced', 'actions') }}
    

    I personally think this is a TX improvement!

  3. +++ b/core/themes/engines/twig/twig.engine
    @@ -137,10 +137,15 @@ function twig_render_var($arg) {
    +    $args = func_get_args();
    ...
    +      foreach($args as $arg) {
    +        if (isset($element[$arg])) $element[$arg]['#printed'] = TRUE;
    

    Missing space after foreach, and {} around if. Also the func_get_args part should be documented like @param ... in the docblock, see FormBuilderInterface::getForm() for an example.

  4. +++ b/core/themes/engines/twig/twig.engine
    @@ -137,10 +137,15 @@ function twig_render_var($arg) {
    +      array_shift($args);
    

    When you don't need the return value of array_shift, use unset($args[0]); (This is reliable because func_get_args returns a numerically indexed array

star-szr’s picture

Thanks @tim.plunkett! I agree on #1, I think we should just remove the hunk. I also agree the syntax is an improvement :D

joelpittet’s picture

Thank you @tim.plunkett and @Cottser

@Cottser yeah one level of children. If we need to hide deeper children we can accomplish that in other ways... though not sure if it's needed?

I tried to clean up docs and remove twig_show()

The last submitted patch, 24: 2114563-24-kill-show-hide.patch, failed testing.

markhalliwell’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests
star-szr’s picture

Example of possibly related functionality from another CMS using Twig:

http://buildwithcraft.com/docs/templating/filters#without

/**
 * Returns an array without certain values.
 *
 * @param array $arr
 * @param mixed $exclude
 * @return array
 */
public function withoutFilter($arr, $exclude)
{
  $filteredArray = array();

  if (!is_array($exclude))
  {
    $exclude = array($exclude);
  }

  foreach ($arr as $key => $value)
  {
    if (!in_array($value, $exclude))
    {
      $filteredArray[$key] = $value;
    }
  }

  return $filteredArray;
}
joelpittet’s picture

The last submitted patch, 24: 2114563-24-kill-show-hide.patch, failed testing.

The last submitted patch, 24: 2114563-24-kill-show-hide.patch, failed testing.

joelpittet’s picture

The last submitted patch, 24: 2114563-24-kill-show-hide.patch, failed testing.

The last submitted patch, 24: 2114563-24-kill-show-hide.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
9.67 KB
1.01 KB

Added the getName back in because it's part of the Twig_ExtensionInterface interface.

star-szr’s picture

Issue tags: +needs profiling

Nice! I think we should remove the unrelated code style/docblock cleanup from this patch.

tstoeckler’s picture

  1. +++ b/core/themes/engines/twig/twig.engine
    @@ -131,27 +131,27 @@ function twig_render_var($arg) {
       if ($element instanceof TwigReference) {
    ...
    +function twig_hide($element) {
    

    I know this was already this way before, but why not type-hint $element directly?

  2. +++ b/core/themes/engines/twig/twig.engine
    @@ -131,27 +131,27 @@ function twig_render_var($arg) {
    +    if (count($args) > 1) {
    

    Why do we support the case of not supplying any args?

joelpittet’s picture

Good points @tstoeckler, thanks for the review! I'm just used to catching edge cases in my code. Here's without.

tstoeckler’s picture

Awesome, thanks!

One minor nitpick: AFAIK TwigReference is in the global namespace, so it should be prefixed with a backslash.

star-szr’s picture

TwigReference is in \Drupal\Core\Template, it's use'd in twig.engine, so I think the function parameters are fine. The docblock should probably use the full namespace though.

joelpittet’s picture

Title: Kill show()/hide() in the theme layer. » Kill show() in Twig. Change hide() to a Twig filter.
Issue summary: View changes
Issue tags: +DX (Developer Experience)
tstoeckler’s picture

Oops sorry. Thanks for clearing that up!

joelpittet’s picture

joelpittet’s picture

@todo: Split out the cleanup into a follow.

joelpittet’s picture

Mind if I bump this up to major? We really need to get rid of TwigReference and possibly TwigNodeVisitor because they are blocking. #1825952: Turn on twig autoescape by default

chx’s picture

Priority: Normal » Major
Status: Needs review » Needs work

This is great and everyone working on Twig agrees but if I see this correctly, we lost all comments regarding the usage of hide? Like this:

- *   {{ content.field_example }}. Use hide(content.field_example) to temporarily
- *   suppress the printing of a given element.
+ *   {{ content.field_example }}.
chx’s picture

Also, renaming to without is a great idea.

yched’s picture

"without" ++

joelpittet’s picture

Status: Needs work » Needs review
FileSize
9.31 KB
1.33 KB

Thanks @chx and @yched. I am +1 for 'without' as well. So here is that plus getting those docs back in there and removing the class cleanup to be added to a followup.

joelpittet’s picture

Wrong interdiff, sorry forgot to commit.

yched’s picture

Nitpick: "temporarily suppress the printing of a given child element" is a bit vague.
"Will it come back if I wait long enough ?" ;-)

Maybe: "exclude a given child element from the output" ?

The last submitted patch, 49: 2114563-kill-show-hide-48.patch, failed testing.

joelpittet’s picture

The last submitted patch, 49: 2114563-kill-show-hide-48.patch, failed testing.

joelpittet’s picture

star-szr’s picture

If we can make the 'without' filter work for classes/attribute objects I can see that being a big win for being able to easily strip out unwanted classes, that can be follow-up though.

star-szr’s picture

Status: Needs review » Needs work

Also, NW because this needs tests.

joelpittet’s picture

Issue summary: View changes
joelpittet’s picture

Status: Needs work » Needs review
FileSize
13.02 KB
3.86 KB

This is just a rough of where I think the tests should go... maybe you can tell me if I'm on the right track...? I don't write many tests.

joelpittet’s picture

Title: Kill show() in Twig. Change hide() to a Twig filter. » Remove show()/hide() in Twig. Replace hide() function with |without() filter.
Assigned: Unassigned » joelpittet
FileSize
3.8 KB

Assigning. Less murderous title. Attached is an experiment in allowing attributes and attributes.class to be used with this filter.

I can do this array_flip trick for OffsetGet or I can set the AttributeArray keys to the values on OffsetSet. This allows for easy removal of classes, although individual classes don't have printed states.

joelpittet’s picture

I'm going to need to revert my type hinting because I'm trying to "take out" TwigReference because it's evil:

  • Makes the twig |raw filter not work.
  • Makes things slow
  • Prevents twig {{ debug(_context|keys) }} from working.

I'll put the Attributes to use |without as a follow up as I'm sure the attributes can be done, and maybe even individual classes as well if I play my cards right!

And remove type hinting because I'll need to accept Attributes and why not any array/object that has a printed property

dawehner’s picture

+++ b/core/modules/system/tests/modules/twig_filter_test/lib/Drupal/twig_filter_test/TwigFilterTestController.php
@@ -0,0 +1,31 @@
+class TwigFilterTestController {

Can't we just put that code directly in the test and safe the full http request around it in the test? the TwigNamespaceTest does the same

joelpittet’s picture

@dawehner that's a great suggestion, I'll give that a try if I can do it:)

star-szr’s picture

Issue tags: +sprint

Adding focus tag.

star-szr’s picture

Status: Needs review » Needs work

Just fixing the status since this still needs tests at least.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
13.67 KB
35.21 KB
1.19 KB
7.89 KB

I moved the filter tests into the twig_theme_test to lighten the load a bit and made theme nicer to look at.

Also, I've put in the removal of TwigReference and TwigTemplate and it's kin. The reason is because in doing the tests if you print a render array more than once on a page, it would duplicate it's children each time.

Try the referenced patch. Note the actual tests aren't written yet... sorry.
drush en twig_theme_test -y
then go to:
/twig-theme-test/filter

Sorry tired when I wrote this... should move TwigReference/TwigTemplate killings to a new critical patch... will do tomorrow, with some test to prove it's critical.

joelpittet’s picture

Title: Remove show()/hide() in Twig. Replace hide() function with |without() filter. » Remove TwigReference with the help of 'without' filter replacing 'show'/'hide' functions.
Issue summary: View changes

Starting cleanup the issue summary.

joelpittet’s picture

Now with tests. Hopefully my verbose patch names help.

  • The one I expect to fail is the without filter + tests only.
  • The one I expect to pass is the same but lacking TwigReference and all it's buddies.
joelpittet’s picture

Issue summary: View changes

The last submitted patch, 67: 2114563-67-without-filter-with-tests-fail.patch, failed testing.

The last submitted patch, 67: 2114563-67-without-filter-with-tests-fail.patch, failed testing.

joelpittet’s picture

Priority: Major » Critical
Issue tags: -Needs tests +Needs issue summary update

Need some help with the issue summary clean-up.

Due to:

  • Duplicated printing bug that TwigReference currently has in core
  • Blocker for auto-escape because it breaks twig's raw filter
  • Bug preventing twig debug variable inspection
  • A performance hindering by tracking and passing references around

I'm bumping this to Critical.

These are not easy to separate because TwigRefence's main reason for being is to support the show/hide feature of renderable arrays. So committing the removal of it would likely break show/hide functionality. And as you can see by the test fail above, commiting the without filter would fail the tests without TwigReference removal.

Also a side benefit is we can use the without on attributes to allow explicit out of order rendering of attributes:
Currently class="{{ attributes.class }}"{{ attributes }} will magically not print the class attribute twice due to Attribute's printed properties and {{ attributes }} class="{{ attributes.class }}" would print two class attributes with the second being empty which is not likely expected.
Though with this without filter we can likely improve DX with explicit {{ attributes|without('class') }} class="{{ attributes.class }}" to let the template do exactly as we ask it.

joelpittet’s picture

Issue summary: View changes
joelpittet’s picture

Issue summary: View changes
joelpittet’s picture

Assigned: joelpittet » Unassigned

Unassigning, I gave a stab at the issue summary. Please have another look through the code and issue summary for any minor detail I may have missed.

RainbowArray’s picture

  1. +++ b/core/themes/bartik/templates/comment.html.twig
    @@ -7,8 +7,11 @@
    + *   printing of a given child element:
    

    This probably needs to be changed to "Use following code to exclude the printing of a given child element:"

  2. +++ b/core/themes/engines/twig/twig.engine
    @@ -131,27 +127,24 @@ function twig_render_var($arg) {
    + * Removes child elements from a renderable array.
    

    Maybe change to:

    "Prevents child elements in a renderable array from being printed."

Above are just a couple comments that could be made clearer.

As much as I can understand what's going on, it seems to make sense. Seems easy enough to use for themers and site builders. I'll take a look at the issue summary.

RainbowArray’s picture

Issue summary: View changes

Tried to clean up a bit of the language in the issue summary.

RainbowArray’s picture

For twig_without docblock, maybe something like this:


Creates a copy of the renderable array without the child elements specified, so that the copy can be printed without these elements. The original renderable array is still available and can be used to print child elements on their own later on in the template.

star-szr’s picture

Issue summary: View changes

Thanks @mdrummond! Adding one line to the issue summary and making a small tweak but it's looking pretty good to me. Nice work @joelpittet and @mdrummond. Going to see about reviewing the patch but may not get to posting a review right away.

star-szr’s picture

Status: Needs review » Needs work
Issue tags: -Needs issue summary update

On top of @mdrummond's suggestions, here's my review:

  1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -22,10 +22,8 @@ public function getFunctions() {
    +      'render_var' => new \Twig_Function_Function('twig_render_var'),
    

    Twig_Function_Function is deprecated, use Twig_SimpleFunction instead. Probably might as well convert 'url' as well since that's the only other one we're keeping here:

    return array(
      new \Twig_SimpleFunction('url', 'url'),
      new \Twig_SimpleFunction('twig_render_var', 'twig_render_var'), 
    );
    

    …should do it.

  2. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -40,6 +38,7 @@ public function getFilters() {
    +      'without' => new \Twig_Filter_Function('twig_without'),
    

    Similarly here, this is deprecated and Twig_SimpleFilter is the new way to do it. Not sure we should convert all these, that could be done in a separate non-critical issue.

  3. +++ b/core/modules/comment/templates/comment.html.twig
    @@ -7,8 +7,11 @@
    + *   {{ content.field_example }}. Use following code to temporarily suppress the
    + *   printing of a given child element:
    
    +++ b/core/themes/bartik/templates/comment.html.twig
    @@ -7,8 +7,11 @@
    + *   {{ content.field_example }}. Use following code to temporarily suppress the
    + *   printing of a given child element:
    

    "Use the following code" would read a bit better here IMO. (Two instances)

  4. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/TwigFilterTest.php
    @@ -0,0 +1,85 @@
    +  /**
    +   * Test Twig "trans" tags.
    +   */
    +  public function testTwigTransTags() {
    

    Method name and docblock need updating here.

  5. +++ b/core/themes/engines/twig/twig.engine
    @@ -96,9 +95,6 @@ function twig_render_template($template_file, $variables) {
     function twig_render_var($arg) {
    -  if ($arg instanceof TwigReference) {
    -    $arg = &$arg->getReference();
    -  }
     
       // Check for numeric zero.
    

    Remove extra blank line at the start of this function please :)

  6. +++ b/core/themes/engines/twig/twig.engine
    @@ -131,27 +127,24 @@ function twig_render_var($arg) {
    + * @param string[] $args,...
    + *   The string keys of $element to prevent printing.
    

    I don't think the string[] thing matches up with https://drupal.org/node/1354#param. I have a feeling we don't document this as a @param at all when we're grabbing the extra arguments, I found a couple examples in views.module like the following but not sure that is part of core standards.

     * @param ...
     *   Any additional parameters will be passed as arguments.
    
joelpittet’s picture

Status: Needs work » Needs review
FileSize
40.65 KB
6.91 KB

This is only doc cleanup, I think the @param string[] $args, ... should be in the coding standards though I did add a space it does describe a lot better what is going in.

Thank you very much @Cottser and @mdrummond for the reviews!

We can grab the interdiff here and post it as a follow-up if that makes the patch easier to read?

joelpittet’s picture

Issue tags: -needs profiling

Here's a profiling. It's not a huge improvement, but still one.

Scenario

  • Bartik
  • Turn on all permissions for everybody jQuery(":checkbox").prop('checked', true);
  • Created an article with 3 tags and 3 comments.
  • Added a custom block and some other random blocks around the page.
=== 8.x..8.x compared (53180b3960796..53180bc98020f):

ct  : 118,855|118,855|0|0.0%
wt  : 709,995|713,596|3,601|0.5%
cpu : 701,886|705,217|3,331|0.5%
mu  : 46,167,536|46,167,536|0|0.0%
pmu : 46,290,024|46,290,024|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=53180b3960796&...

=== 8.x..2114563-twig-show-hide-without compared (53180b3960796..53180c595dcf8):

ct  : 118,855|118,318|-537|-0.5%
wt  : 709,995|708,622|-1,373|-0.2%
cpu : 701,886|700,474|-1,412|-0.2%
mu  : 46,167,536|45,757,832|-409,704|-0.9%
pmu : 46,290,024|45,907,688|-382,336|-0.8%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=53180b3960796&...

star-szr’s picture

Status: Needs review » Needs work

I really think the unrelated docs/coding standards cleanups should be moved to a separate, non-critical issue to expedite this patch. I can volunteer to do that, just let me know what you'd prefer @joelpittet!

star-szr’s picture

From what I can see the changes look great by the way :)

joelpittet’s picture

Status: Needs work » Needs review
FileSize
6.62 KB
38.27 KB

Added a docs clean-up follow-up non-critical;)
#2212309: Drupal\Core\Template and twig.engine docs, coding standards, and unused code cleanup

These are just more important bits from #80 applied to #67

star-szr’s picture

Status: Needs review » Needs work

Thanks @joelpittet, that makes it much easier! Some more minor points, I can't find any more faults, this is very close IMO.

  1. +++ b/core/lib/Drupal/Core/Template/TwigNodeVisitor.php
    @@ -61,27 +34,17 @@ function enterNode(\Twig_NodeInterface $node, \Twig_Environment $env) {
        * @see twig_render
        */
       function leaveNode(\Twig_NodeInterface $node, \Twig_Environment $env) {
    +     // We use this to inject a call to render_var -> twig_render_var()
    +     // before anything is printed.
    

    Since this added comment is duplicating the docblock, let's either remove this change or make the change you had before changing the docblock to {@inheritdoc}. No sense doing this in between change :)

  2. +++ b/core/lib/Drupal/Core/Template/TwigNodeVisitor.php
    @@ -61,27 +34,17 @@ function enterNode(\Twig_NodeInterface $node, \Twig_Environment $env) {
    +      $line = $node->getLine();
           return new $class(
    -        new \Twig_Node_Expression_Function('render_var', new \Twig_Node(array($node->getNode('expr'))), $node->getLine()),
    +        new \Twig_Node_Expression_Function('render_var', new \Twig_Node(array($node->getNode('expr'))), $line),
             $node->getLine()
           );
    

    If we're going to define the $line variable, let's use it! Replace the last $node->getLine() with $line.

  3. +++ b/core/modules/comment/templates/comment.html.twig
    @@ -93,8 +96,7 @@
         {# We hide the links now so that we can render them later. #}
    -    {% hide(content.links) %}
    -    {{ content }}
    +    {{ content|without('links') }}
    
    +++ b/core/modules/node/templates/node.html.twig
    @@ -94,8 +94,7 @@
         {# We hide links now so that we can render them later. #}
    -    {% hide(content.links) %}
    -    {{ content }}
    +    {{ content|without('links') }}
    
    +++ b/core/themes/bartik/templates/comment.html.twig
    @@ -107,8 +110,7 @@
           {# We hide the links now so that we can render them later. #}
    -      {% hide(content.links) %}
    -      {{ content }}
    +      {{ content|without('links') }}
    
    +++ b/core/themes/bartik/templates/node.html.twig
    @@ -92,8 +90,7 @@
         {# We hide links now so that we can render them later. #}
    -    {% hide(content.links) %}
    -    {{ content }}
    +    {{ content|without('links') }}
    

    Minor point but the "We hide links..." comments here might not make as much sense when we're using |without. Maybe something as simple as "Render the content without the links."? Or maybe it's a lot more self-explanatory now and we don't need to comment on it at all? :)

    (4 instances)

  4. +++ b/core/themes/bartik/templates/node.html.twig
    @@ -17,9 +17,7 @@
    - *   or print a subset such as {{ content.field_example }}. Use
    - *   {% hide(content.field_example) %} to temporarily suppress the printing
    - *   of a given element.
    + *   or print a subset such as {{ content.field_example }}.
    

    I think this is the only case where we're losing valuable docs, this should be consistent with the node.html.twig in node module IMO where we retain the docs on how to hide/exclude sub-elements.

joelpittet’s picture

Did mostly the drop/remove options from #85;) and thanks for the $line variable catch!

star-szr’s picture

Status: Needs work » Needs review

Testbot go!

star-szr’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -21,25 +21,24 @@ public function getFunctions() {
+      'url' => new \Twig_SimpleFunction('url', 'url'),
+      // This function will receive a renderable array, if an array is detected.
+      'render_var' => new \Twig_SimpleFunction('render_var', 'twig_render_var'),

I don't think this array needs keys, just new \Twig_SimpleFunction(…),.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
38.19 KB
971 bytes

Thanks, good catch wouldn't do anything but totally unnecessary with that syntax.

star-szr’s picture

Status: Needs review » Reviewed & tested by the community

I think that's the winner. To sum up:

  • Improves DX and theming experience - the without filter reads like English to me!
    {{ this|without('that') }}
  • Unblocks autoescape
  • Makes variable inspection and debugging within Twig templates approximately 700% better
  • Removes a ton of Drupal-specific Twig code
  • Slight performance boost and smaller memory footprint due to less references being passed around
star-szr’s picture

Woke up and realized we'd need a draft change record: https://drupal.org/node/2212845

Fabianx’s picture

What a genius idea!

+1 to **RTBC** from me.

I reviewed the code and that works very well.

My only nitpick is:

diff --git a/core/lib/Drupal/Core/Template/TwigNodeExpressionNameReference.php b/core/lib/Drupal/Core/Template/TwigNodeExpressionNameReference.php
@@ -21,7 +21,7 @@ class TwigNodeExpressionNameReference extends \Twig_Node_Expression_Name {

This file is now orphaned. Poor TwigNodeExpressionNameReference is no longer needed :).

Not sure that this is a merge blocker though.

Thanks!

catch’s picture

Status: Reviewed & tested by the community » Needs work

Let's remove that while we're here.

star-szr’s picture

martin107’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
38.48 KB
956 bytes

Removed TwigNodeExpressionNameReference.

The last submitted patch, 94: 2114563-94.patch, failed testing.

star-szr’s picture

Status: Needs review » Reviewed & tested by the community

Patches are identical so I've cancelled the testing of mine, #95 is good to go pending testbot's approval :)

martin107’s picture

Opps now that I have been caught playing snap with Cottser ... I will have to stand in the corner.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

joelpittet’s picture

Change record published @ https://drupal.org/node/2212845

Thanks everybody!

star-szr’s picture

+++ /dev/null
@@ -1,89 +0,0 @@
-    if (isset($context['_references'][$item])) {
-      return $context['_references'][$item];
-    }
...
-    // Save that this is a reference
-    $context['_references'][$item] = $ref;

This means we can now remove a line from twig_render_template() as well, small Novice follow-up:

#2218849: Remove unused line from twig_render_template()

forestmars’s picture

Issue summary: View changes

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.