We are recommending not to call drupal_render() in preprocess, now formally documented here: http://drupal.org/node/1920746

Per #1898432-20: node.module - Convert PHPTemplate templates to Twig, the same should be true of drupal_render_children().

drupal_render_children should be implicit when something is printed using twig, so that we don't have to do it in preprocess.

Files: 
CommentFileSizeAuthor
#71 views_ef_fieldset6.jpg65.97 KBPol
#70 interdiff.txt710 bytesrteijeiro
#70 drupal_render_should-1920886-70.patch1.36 KBrteijeiro
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,254 pass(es). View
#68 drupal_render_should-1920886-68.patch1.33 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,380 pass(es), 0 fail(s), and 1 exception(s). View
#64 drupal_render_should-1920886-64.patch1.41 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,390 pass(es), 1 fail(s), and 1 exception(s). View
#58 1920886-58.patch1.47 KBCottser
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1920886-58.patch. Unable to apply patch. See the log in the details link for more information. View
#58 interdiff.txt507 bytesCottser
#54 drupal-fix_double_rendering_of_element_children-1920886-54.patch1.42 KBjenlampton
FAILED: [[SimpleTest]]: [MySQL] 56,303 pass(es), 1,633 fail(s), and 8 exception(s). View
#48 drupal-fix_double_rendering_of_element_children-1920886-48.patch2.28 KBjenlampton
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site. View
#33 1920886-33.patch4.34 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 55,812 pass(es). View
#33 interdiff.txt4.29 KBCottser
#31 1920886-31.patch4 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 55,816 pass(es). View
#31 interdiff.txt2.59 KBCottser
#26 1920886-26.patch3.14 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 55,612 pass(es). View
#26 interdiff.txt2.14 KBCottser
#21 1920886-20.patch1 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 55,932 pass(es). View
#18 core_0001-Issue-1920886-by-Cottser-Twig-should-check-to-see-if-v2.patch1.38 KBFabianx
FAILED: [[SimpleTest]]: [MySQL] 55,438 pass(es), 156 fail(s), and 4,359 exception(s). View
#16 core_0001-Issue-1920886-by-Cottser-Twig-should-check-to-see-if.patch1.35 KBFabianx
FAILED: [[SimpleTest]]: [MySQL] 55,394 pass(es), 156 fail(s), and 4,357 exception(s). View
#10 1920886-10.patch576 bytesCottser
FAILED: [[SimpleTest]]: [MySQL] 54,917 pass(es), 524 fail(s), and 75 exception(s). View

Comments

Cottser’s picture

Title: Twig preprocess: don't call drupal_render_children() in preprocess » Don't call drupal_render_children() in preprocess functions
Issue tags: +Twig

Tagging and re-titling.

Fabianx’s picture


$variables['children'] = drupal_render_children($form);

This is problematic, because $form had a #theme, which would lead to an infinite loop.

What we can do is have a function like:

drupal_lazy_render_children()

which just does.


function drupal_lazy_render_children(&$element) {
  $output = array();

  foreach (element_children($element) as $key) {
    $output[$key] = &$element[$key];
  }

return $output;

}

That way render will still correctly render the children, but won't run into the infinite loop.

Another possibility is to introduce type hints for twig, like:

'#render => TRUE
#render_children => TRUE
'

as part of the render array().

That can be used for optimization of the code internally.

jenlampton’s picture

I'm running into this same problem in color.module conversion to Twig.

The problem is if we leave $variables['children'] = drupal_render_children($form) in the preprocess (temporarily) then the entire form gets rendered twice, since drupal_render_children($form) at this point contains the whole form (since nothing else has been rendered yet).

As a temporary workaround for this core patch, I'm going to make a copy of $form and call it $children, and remove the corresponding element from $children every time the element in $form is preprocessed into it's own variable. Then $variables['children'] = drupal_render_children($children) returns the expected output.

Cottser’s picture

An example from #1898052: color.module - Convert theme_ functions to Twig that I'm working on:

// Remove elements from the 'children' render array.
unset($children['palette'], $children['scheme']);
$variables['children'] = drupal_render_children($children);
jenlampton’s picture

Priority: Normal » Major

This temporary workaround is also causing some problems with hide(). If we preprocess everything into separate variables then we can't use hide in the template files. Maybe this is okay? (death to hide!) but we had originally thought we needed to keep it (death to preprocess!).

Maybe time to re-evaluate, and solve this one. Bumping up priority.

Fabianx’s picture

#5 This just means that functionality does not work that did not work before, so that is not a big deal.

We can remove drupal_render_children() once we have sorted out the infinite loop problem.

Because in general:

{{ form['palette'] }}
{% hide(form['scheme']) %}
{{ form }}
{{ form['scheme'] }}

should work fine.

From a performance perspective:

drupal_render_children vs. drupal_render call -> no measurable difference, because isset() is so freaking fast ...

jenlampton’s picture

Title: Don't call drupal_render_children() in preprocess functions » Twig should check to see if it's rendering a 'render element' and if so call drupal_render_children instead() of drupal_render()

We should be able to tell when Twig should render the renderable we give it, and when it should render only the children within. It only needs to render the children within when the 'render element' is specified in hook_theme - can we use that as our toggle?

Updating title to reflect the TODO action of this issue (please change again if necessary)

Cottser’s picture

Title: Twig should check to see if it's rendering a 'render element' and if so call drupal_render_children instead() of drupal_render() » Twig should check to see if it's rendering a 'render element' and if so call drupal_render_children() instead of drupal_render()

Title is better I think, just fixing one thing. Thanks @jenlampton :)

Fabianx’s picture

Assigned: Unassigned » Fabianx

The example exists here:

node-edit-form.html.twig

in http://drupal.org/node/1898432#comment-7162006

Cottser’s picture

Status: Active » Needs review
FileSize
576 bytes
FAILED: [[SimpleTest]]: [MySQL] 54,917 pass(es), 524 fail(s), and 75 exception(s). View

Here's one idea based on a discussion @Fabianx and I had yesterday. With this patch in place, node-edit-form.html.twig works fine, no preprocess needed and hide() can be used in the template.

{#
/**
 * @file
 * Two column template for the node add/edit form.
 *
 * Available variables:
 * - form: Complete form. Use {{ form }} to print the entire form, or print a
 *   subset such as {{ form.actions }}. Use {% hide(form.actions) %} to
 *   temporarily suppress the printing of a given element.
 *   - form.advanced: The advanced options for the node form.
 *   - form.actions: The action buttons for save and delete.
 *
 * @ingroup themable
 */
#}
{% hide(form.advanced) %}
{% hide(form.actions) %}
<div class="layout-node-form clearfix">
  <div class="layout-region layout-region-node-main">
    {{ form }}
  </div>
  <div class="layout-region layout-region-node-secondary">
    {{ form.advanced }}
  </div>
  <div class="layout-region layout-region-node-footer">
    {{ form.actions }}
  </div>
</div>

Status: Needs review » Needs work

The last submitted patch, 1920886-10.patch, failed testing.

shanethehat’s picture

When this is resolved, seven.theme will need a cleanup as mentioned in #72 of #1898432: node.module - Convert PHPTemplate templates to Twig

jenlampton’s picture

Status: Needs work » Needs review
Issue tags: -Twig

#10: 1920886-10.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Twig

The last submitted patch, 1920886-10.patch, failed testing.

Fabianx’s picture

After long discussions, I think Twig should check via isset for this key manually and call drupal_render children instead.

The necessary key can be set when $form[$render_element] = $element is set in theme.inc.

( will add correct line later )

Fabianx’s picture

Assigned: Fabianx » Unassigned
Status: Needs work » Needs review
FileSize
1.35 KB
FAILED: [[SimpleTest]]: [MySQL] 55,394 pass(es), 156 fail(s), and 4,357 exception(s). View

And here is the patch for that:

* Gives a hint to rendering engines that this is coming from a self-render to prevent recursion.
* Changes twig engine to take this new hint into account.

a) Patch - DONE

Stil todo:

b) Test Coverage
c) Documenation - especially for valid render array keys

Have fun, Twig Patch Squad! :-)

webchick’s picture

Wow! Well that's far less problematic than I was picturing. :) Great work!

Fabianx’s picture

FileSize
1.38 KB
FAILED: [[SimpleTest]]: [MySQL] 55,438 pass(es), 156 fail(s), and 4,359 exception(s). View

Well, if you apply a working patch, then it is easier :-) ...

Correct patch attached! :-)

Cottser’s picture

Assigned: Unassigned » Cottser

We discussed adding this logic in drupal_render() itself, initial tests seem to show that will work as well.

I'll look at writing some tests for this. Thanks @Fabianx!

Status: Needs review » Needs work
Cottser’s picture

Status: Needs work » Needs review
FileSize
1 KB
PASSED: [[SimpleTest]]: [MySQL] 55,932 pass(es). View

I'll just leave this here… :)

Cottser’s picture

Cottser’s picture

(Sending for a re-test, ran some of the tests locally and they passed)

It looks like we'd be documenting this in http://api.drupal.org/api/drupal/developer%21topics%21forms_api_referenc... as an "internal property". I have no idea how one goes about changing that documentation :)

Fabianx’s picture

Issue tags: +Needs tests

This will need tests.

Besides that: RTBC :-).

Cottser’s picture

Status: Needs review » Needs work

Hoping to get something up this weekend :)

Cottser’s picture

Title: Twig should check to see if it's rendering a 'render element' and if so call drupal_render_children() instead of drupal_render() » drupal_render() should check to see if it's rendering a 'render element' and if so call drupal_render_children()
Assigned: Cottser » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2.14 KB
3.14 KB
PASSED: [[SimpleTest]]: [MySQL] 55,612 pass(es). View

Here's a test for this. Forgoing a test-only patch because the pre-patch behaviour is an infinite loop. I tested locally and running the test results in an infinite loop so it is testing the behaviour.

I also created a postponed issue in the documentation project to document this new internal property: #1993198: Document new internal FAPI property: #render_children

Re-titling to indicate that this is a change for the theme system in general, not just Twig.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Quick fix

* Easy patch.
* Has nice test coverage.
* Well documented.
* isset() is very very fast

AAAAND fixes one big WTF! of Drupal 7.

=> Full win, RTBC

catch’s picture

There's a bunch of calls to this in preprocess in core: http://api.drupal.org/api/drupal/core%21includes%21common.inc/function/d...

Is this something that'll get handled in the twig conversions or do we need explicit follow-ups to go through those?

Patch looks good to me and a nice step along the way to removing preprocess bloat.

Cottser’s picture

@catch - we can definitely remove the calls from the .tpl.php preprocess functions right away (and in some cases remove preprocess functions entirely!). As for the theme_ functions I think it would make more sense to do that as follow-up.

Cottser’s picture

Status: Reviewed & tested by the community » Needs review

I didn't think of this before but I think we can avoid the function call to drupal_render_children() altogether.

Cottser’s picture

FileSize
2.59 KB
4 KB
PASSED: [[SimpleTest]]: [MySQL] 55,816 pass(es). View

Here's another take on this, just has drupal_render() check for the #render_element key before any calls to theme(). Added test coverage for #theme_wrappers as well.

Cottser’s picture

Of course the inline docs would need to be updated for #31. But other than that?

Cottser’s picture

FileSize
4.29 KB
4.34 KB
PASSED: [[SimpleTest]]: [MySQL] 55,812 pass(es). View

Here's a pass at the docs in line with #31.

Cottser’s picture

Title: drupal_render() should check to see if it's rendering a 'render element' and if so call drupal_render_children() » drupal_render() should only render the child elements when rendering a 'render element'
Fabianx’s picture

Title: drupal_render() should only render the child elements when rendering a 'render element' » drupal_render() should only render the child elements when rendering a 'render element' for the first time
Status: Needs review » Reviewed & tested by the community

I am very happy with #33.

Even nicer, has nice docs, tests still pass.

=> RTBC

Dries’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs followup

Committed to 8.x.

This adds a little more complexity to drupal_render(), but that can be offset by removing drupal_render_children(), so let's open a follow up to remove that function.

Thanks!

moshe weitzman’s picture

We add drupal_render_children() in #355236: Refactor drupal_render theming - docs and everyone was happy at the time with its cleanliness. Now it looks like we are deprecating that. Can anyone explain why a that function was an improvement before and a hindrance now?

Cottser’s picture

Looks like this did not get pushed yet?

Regarding #37, I definitely can't speak to the history of the linked issue, way before my time.

But @chx in #355236: Refactor drupal_render theming - docs:

Uhm why there is a d_r and why there is a d_r_c? When i want to use which?

I think that's a pretty good reason. drupal_render() before this commit still had the functionality of drupal_render_children() contained within it. I don't think we necessarily have to deprecate drupal_render_children(), but I'm not sure we need it for any reason.

Snippet from drupal_render():

  // If #theme was not set and the element has children, render them now.
  // This is the same process as drupal_render_children() but is inlined
  // for speed.
  if ($elements['#children'] === '') {
    foreach ($children as $key) {
      $elements['#children'] .= drupal_render($elements[$key]);
    }
  }
Cottser’s picture

Status: Fixed » Reviewed & tested by the community

Back to RTBC per #35 to try and get this pushed or re-committed.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 3b1f85a and pushed to 8.x. Thanks!

alexpott’s picture

Re #37 this patch did not deprecate drupal_render_children() it just made it safe to call multiple time... it's deprecation or removal and be discussed in a followup if we want to pursue that....

Cottser’s picture

Cottser’s picture

Issue tags: -Needs followup

Tags.

Status: Fixed » Closed (fixed)

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

Fabianx’s picture

Status: Closed (fixed) » Needs work
Issue tags: -Quick fix

Unfortunately I have to re-open this,

The committed patch is wrong, as you can set '#prefix', '#suffix' or '#pre_render', '#post_render' and easily get double output. (like a double wrapper or such). (It did fix the recursion though)

The correct patch would be like:

   if (empty($elements) || (isset($elements['#access']) && !$elements['#access'])) {
     return '';
   }
+
+  // If the special '#render_children' key is set, call drupal_render_children()
+  // instead of drupal_render() to prevent recursion.
+  if (isset($elements['#render_children'])) {
+    return drupal_render_children($elements);
+  }

And then just copying the code from below and inlining it for speed:

   if (empty($elements) || (isset($elements['#access']) && !$elements['#access'])) {
     return '';
   }
+
+  // If the special '#render_children' key is set, call drupal_render_children()
+  // instead of drupal_render() to prevent recursion.
+  if (isset($elements['#render_children'])) {
+   $children_keys = element_children($elements);
+   $output = '';
+   foreach ($children_keys as $key) {
+      $output .= drupal_render($element[$key]);
+    }
+    return $output;
+  }

and removing all further isset(#render_children) from below.

It is crucial that it returns that early.

Fabianx’s picture

Title: drupal_render() should only render the child elements when rendering a 'render element' for the first time » drupal_render() should render _only_ the child elements when rendering a 'render element' for the second time

Clariying title to make task more clear.

Fabianx’s picture

Title: drupal_render() should render _only_ the child elements when rendering a 'render element' for the second time » drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead

Changing title to the best title in this issue.

jenlampton’s picture

Status: Needs work » Needs review
FileSize
2.28 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site. View

Implemented @Fabianx's suggestions from #45

Status: Needs review » Needs work
Issue tags: -Twig

The last submitted patch, drupal-fix_double_rendering_of_element_children-1920886-48.patch, failed testing.

jenlampton’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-fix_double_rendering_of_element_children-1920886-48.patch, failed testing.

Fabianx’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Twig

The last submitted patch, drupal-fix_double_rendering_of_element_children-1920886-48.patch, failed testing.

jenlampton’s picture

Status: Needs work » Needs review
FileSize
1.42 KB
FAILED: [[SimpleTest]]: [MySQL] 56,303 pass(es), 1,633 fail(s), and 8 exception(s). View

A few problems. Returning early is not an option when we are dealing with the $page array. I added an exception to prevent it from rendering & returning for the page, and I seem to have gotten my site back up :) Also, I was overdoing the removal of isset('#render_children'). Fixed now.

Status: Needs review » Needs work

The last submitted patch, drupal-fix_double_rendering_of_element_children-1920886-54.patch, failed testing.

thedavidmeister’s picture

ooh, this is interesting. Best of luck with this :)

Fabianx’s picture

Assigned: Unassigned » Fabianx

Its DrupalCon ...

Cottser’s picture

Status: Needs work » Needs review
FileSize
507 bytes
1.47 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1920886-58.patch. Unable to apply patch. See the log in the details link for more information. View

This might work a bit better - the main change is $element -> $elements.

Status: Needs review » Needs work
Issue tags: -Twig

The last submitted patch, 1920886-58.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

edit

markcarver’s picture

Adding tags

Cottser’s picture

+++ b/core/includes/common.inc
@@ -3822,6 +3822,20 @@ function drupal_render(&$elements) {
+  // Call drupal_render_children() instead of drupal_render() to prevent
+  // recursion if the special '#render_children' key is set (in all instancecs
+  // except for the $page array).
+  if (isset($elements['#render_children']) && $elements['#theme'] != 'page') {

This comment doesn't make sense, we're actually doing the equivalent of drupal_render_children() inline. Thanks @Mark Carver on the Twig call.

Status: Needs work » Needs review

steveoliver queued 58: 1920886-58.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 58: 1920886-58.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
1.41 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,390 pass(es), 1 fail(s), and 1 exception(s). View

Reroll

Status: Needs review » Needs work

The last submitted patch, 64: drupal_render_should-1920886-64.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 64: drupal_render_should-1920886-64.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
1.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,380 pass(es), 0 fail(s), and 1 exception(s). View

Lets see what the testbot says

Status: Needs review » Needs work

The last submitted patch, 68: drupal_render_should-1920886-68.patch, failed testing.

rteijeiro’s picture

Assigned: Fabianx » Unassigned
Status: Needs work » Needs review
FileSize
1.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,254 pass(es). View
710 bytes

Trying to fix the broken test.

Pol’s picture

FileSize
65.97 KB

The patch works successfully.

Without it I have thinks like the screenshot uploaded to this comment for the module: https://www.drupal.org/project/views_ef_fieldset

Can't wait to see this in.

Fabianx’s picture

Priority: Major » Critical
  1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
    @@ -143,6 +143,17 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
    +    if (isset($elements['#render_children']) && empty($elements['#children']) && isset($elements['#theme']) && $elements['#theme'] != 'page') {
    

    That is unfortunately not the right logic.

    Edit: After some sleep:

    That is kinda the right logic as only #theme sets the #render_children property in core as only that needs to prevent recursion.

    Theoretically this could also occur via #theme_wrappers though, so what about:

      $theme_property = isset($elements['#theme']) ? $elements['#theme'] : FALSE;
       if (isset($elements['#render_children']) && empty($elements['#children']) && $theme_property != 'page') {
           // ...
       }
    
  2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
    @@ -267,9 +278,7 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
    +    // element have to be rendered there.
         if ($theme_is_implemented && !isset($elements['#render_children'])) {
    

    This should also remove the other cases of #render_children - unless #page needs it - which should be checked.

Bumping to critical as this completely breaks #post_render for e.g. views form_views_exposed_form_alter as found by @Pol.

There is no work-around possible, the #post_render is called twice and you cannot check that as you don't have a reference to the render array, so you cannot even know if it was rendered already.

( Edit: Code can obviously check for '#render_children' presence themselves ... )

Fabianx’s picture

Priority: Critical » Major
Status: Needs review » Needs work
Issue tags: +DrupalCon LA

Demoting back to Major, code can just check #render_children itself.

Really not optimal, but a work-around is possible.

Pushing to core committers for attention though if it should be release blocking ...

CNW, per #72.

joelpittet’s picture

Issue tags: +rc target triage

Adding tag for #73

xjm’s picture

Issue tags: -rc target triage

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

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • alexpott committed 3b1f85a on 8.3.x
    Issue #1920886 by Cottser, Fabianx: Drupal_render() should only render...

  • alexpott committed 3b1f85a on 8.3.x
    Issue #1920886 by Cottser, Fabianx: Drupal_render() should only render...
Fabianx’s picture

Status: Needs work » Closed (duplicate)
Related issues: +#2346893: Duplicate AJAX wrapper around a file field

Closing this as duplicate of #2346893: Duplicate AJAX wrapper around a file field, which has tests, etc.