drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead

$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.

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.

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.

#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 ...

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)

The example exists here:

node-edit-form.html.twig

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

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

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

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 )

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! :-)

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

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

Correct patch attached! :-)

This will need tests.

Besides that: RTBC :-).

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

AAAAND fixes one big WTF! of Drupal 7.

=> Full win, RTBC

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.

I am very happy with #33.

Even nicer, has nice docs, tests still pass.

=> RTBC

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!

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?

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

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....

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.

Clariying title to make task more clear.

Changing title to the best title in this issue.

Implemented @Fabianx's suggestions from #45

#48: drupal-fix_double_rendering_of_element_children-1920886-48.patch queued for re-testing.

#48: drupal-fix_double_rendering_of_element_children-1920886-48.patch queued for re-testing.

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.

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

Its DrupalCon ...

Adding tags

Reroll

Lets see what the testbot says

Trying to fix the broken test.

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.

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 ... )

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.

Adding tag for #73

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

Correcting historical tag.