The docs say that drupal_render() will pass a renderable array to theme() if #theme is set and in this case it is the responsibility of the theme function called to render children as required.

Currently drupal_render() does this:

  // Call the element's #theme function if it is set. Then any children of the
  // element have to be rendered there. If the internal #render_children
  // property is set, do not call the #theme function to prevent infinite
  // recursion.
  if (isset($elements['#theme']) && !isset($elements['#render_children'])) {
    $elements['#children'] = theme($elements['#theme'], $elements);
  }
  // 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]);
    }
  }

The problem is that if theme() returns an empty string for some reason, drupal_render() then ignores the theme function's decision to not render the children elements and tries to go ahead and recursively render the children anyway.

Files: 
CommentFileSizeAuthor
#44 2012812-44.patch11.01 KBthedavidmeister
PASSED: [[SimpleTest]]: [MySQL] 56,616 pass(es).
[ View ]
#44 interdiff-43-44.patch7.01 KBthedavidmeister
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-43-44.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#43 2012812-43.patch10.38 KBthedavidmeister
PASSED: [[SimpleTest]]: [MySQL] 56,654 pass(es).
[ View ]
#41 2012812-41.patch9.89 KBthedavidmeister
FAILED: [[SimpleTest]]: [MySQL] 57,781 pass(es), 20 fail(s), and 0 exception(s).
[ View ]
#37 2012812-37.patch9.13 KBthedavidmeister
FAILED: [[SimpleTest]]: [MySQL] 57,864 pass(es), 19 fail(s), and 0 exception(s).
[ View ]
#33 Double_content_in_overlay.png40.85 KBalexpott
#28 2012812-28.patch8.14 KBpwieck
PASSED: [[SimpleTest]]: [MySQL] 56,858 pass(es).
[ View ]
#23 2012812-23.patch8.05 KBthedavidmeister
PASSED: [[SimpleTest]]: [MySQL] 55,187 pass(es).
[ View ]
#22 2012812-22.patch8.49 KBthedavidmeister
PASSED: [[SimpleTest]]: [MySQL] 55,059 pass(es).
[ View ]
#21 2012812-tests.patch3.77 KBthedavidmeister
FAILED: [[SimpleTest]]: [MySQL] 55,508 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#20 2012812-20.patch4.96 KBthedavidmeister
PASSED: [[SimpleTest]]: [MySQL] 55,873 pass(es).
[ View ]
#18 2012812-18.patch1.94 KBthedavidmeister
FAILED: [[SimpleTest]]: [MySQL] 55,467 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#17 2012812-17.patch2.29 KBthedavidmeister
FAILED: [[SimpleTest]]: [MySQL] 55,465 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#16 2012812-16.patch3.08 KBthedavidmeister
FAILED: [[SimpleTest]]: [MySQL] 55,811 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#14 2012812-14.patch2.99 KBthedavidmeister
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#1 2012812-1.patch856 bytesthedavidmeister
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Comments

thedavidmeister’s picture

Issue tags:+theme system cleanup
StatusFileSize
new856 bytes
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
thedavidmeister’s picture

Status:Active» Needs review

Status:Needs review» Needs work
Issue tags:-theme system cleanup

The last submitted patch, 2012812-1.patch, failed testing.

thedavidmeister’s picture

Status:Needs work» Needs review

#1: 2012812-1.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 2012812-1.patch, failed testing.

thedavidmeister’s picture

Status:Needs work» Needs review

#1: 2012812-1.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+theme system cleanup

The last submitted patch, 2012812-1.patch, failed testing.

thedavidmeister’s picture

hmmmmmmmmmmmmmmmmmmmmmm

thedavidmeister’s picture

Wow, yep, the installer doesn't appear which implies that it declares #theme but then relies on drupal_render() to be called recursively make children appear. Fail :/

Going to look into this further.

thedavidmeister’s picture

  // If no #theme has been set, automatically apply theme suggestions.
  // theme_form() itself is in #theme_wrappers and not #theme. Therefore, the
  // #theme function only has to care for rendering the inner form elements,
  // not the form itself.
  if (!isset($form['#theme'])) {
    $form['#theme'] = array($form_id);
    if (isset($form_state['build_info']['base_form_id'])) {
      $form['#theme'][] = $form_state['build_info']['base_form_id'];
    }
  }

In drupal_prepare_form() leads to #theme => array('install_select_language_form'). This theme function doesn't exist and theme('install_select_language_form') returns an empty string.

If this is the intended behaviour then this in drupal_prepare_form():

Therefore, the
// #theme function only has to care for rendering the inner form elements,
// not the form itself.

and this inside drupal_render():

// Call the element's #theme function if it is set. Then any children of the
// element have to be rendered there.

and this above drupal_render():

* #theme is the theme function called first. If it is set and the element has
* any children, it is the responsibility of the theme function to render
* these children. For elements that are not allowed to have any children,
* e.g. buttons or textfields, the theme function can be used to render the
* element itself. If #theme is not present and the element has children, each
* child is itself rendered by a call to drupal_render(), and the results are
* concatenated.

are really misleading statements in the documentation.

thedavidmeister’s picture

I'm not really sure what to do here.

There's multiple ways to get theme() to return an empty string that don't justify drupal_render() having a "second go" at rendering children itself.

The documentation is also inaccurate.

I can also see why it would be beneficial to want to "fallback" to the drupal_render() behaviour, like in this exact case where drupal_get_form() merges in default theme suggestions into the form renderable array, which returns an empty string when none of those suggestions match, despite the fact that you would clearly want your form to render it's children even without matching suggestions.

I can't tell just by looking at drupal_render() and the documentation what the intended behaviour is here, so I don't know if the code or the docs are wrong. If the intended behaviour is to allow "fallbacks" then I believe we should be more careful about when we "fallback" to avoid undermining the work done by theme().

c4rl’s picture

The check on $elements['#children'] === '' is just bad design because it doesn't talk to the theme registry and instead assumes execution resulting in an empty string means there are sub-elements (true in come cases, but not all).

I can't tell just by looking at drupal_render() and the documentation what the intended behaviour is here, so I don't know if the code or the docs are wrong.

The fact that our code and docs are un-grokkable likely indicates they are *both* wrong. drupal_render() implementation was conceived via Form API, and so there may be some conventions there we can refactor. I need to tinker with this a bit more, but it seems to me we need to establish some base principles that define a render array in D8.

thedavidmeister’s picture

Status:Needs work» Needs review
StatusFileSize
new2.99 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

so... I don't love the idea of having theme() returning mixed data types but this does seem like the simplest resolution of this bug with the smallest change to the API. It wouldn't be so bad if theme() became "private" to drupal_render().

I'm totally open to better ideas.

Status:Needs review» Needs work

The last submitted patch, 2012812-14.patch, failed testing.

thedavidmeister’s picture

Status:Needs work» Needs review
StatusFileSize
new3.08 KB
FAILED: [[SimpleTest]]: [MySQL] 55,811 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
thedavidmeister’s picture

StatusFileSize
new2.29 KB
FAILED: [[SimpleTest]]: [MySQL] 55,465 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

this one might actually pass tests...

thedavidmeister’s picture

StatusFileSize
new1.94 KB
FAILED: [[SimpleTest]]: [MySQL] 55,467 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

simpler, might work.

Apologies for all the noise here!

Status:Needs review» Needs work

The last submitted patch, 2012812-18.patch, failed testing.

thedavidmeister’s picture

Title:if theme() in drupal_render() returns an empty string, drupal_render() will inappropriately try to render children recursively» drupal_render() can't distinguish between empty strings from theme() from an implemented hook and no hook being matched
Status:Needs work» Needs review
StatusFileSize
new4.96 KB
PASSED: [[SimpleTest]]: [MySQL] 55,873 pass(es).
[ View ]

update for failing test, also moving the related issue into this one and closing that as a duplicate.

thedavidmeister’s picture

StatusFileSize
new3.77 KB
FAILED: [[SimpleTest]]: [MySQL] 55,508 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Here's some tests demonstrating this issue. HEAD fails two of the four new tests in this patch on my local.

thedavidmeister’s picture

StatusFileSize
new8.49 KB
PASSED: [[SimpleTest]]: [MySQL] 55,059 pass(es).
[ View ]

This combines #20 which the testbots like (yay!) with #21 which introduces new tests for this issue.

thedavidmeister’s picture

StatusFileSize
new8.05 KB
PASSED: [[SimpleTest]]: [MySQL] 55,187 pass(es).
[ View ]

In IRC @Fabianx and @chx both said they'd prefer FALSE to NULL.

thedavidmeister’s picture

The inability to use #markup and #theme suggestions together is currently blocking #1898474: pager.inc - Convert theme_ functions to Twig.

Fabianx’s picture

Status:Needs review» Reviewed & tested by the community

It looks good to me and provides a needed next step to be able to combine #type and #theme (suggestions).

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Needs a reroll...

curl https://drupal.org/files/2012812-23.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  8241  100  8241    0     0   5860      0  0:00:01  0:00:01 --:--:--  6925
error: patch failed: core/modules/system/tests/modules/common_test/common_test.module:250
error: core/modules/system/tests/modules/common_test/common_test.module: patch does not apply
pwieck’s picture

Assigned:Unassigned» pwieck

Working on reroll

pwieck’s picture

StatusFileSize
new8.14 KB
PASSED: [[SimpleTest]]: [MySQL] 56,858 pass(es).
[ View ]

reroll to current head with only minor conflict fix in common_test.module

pwieck’s picture

Status:Needs work» Needs review

status change

pwieck’s picture

Assigned:pwieck» Unassigned

#28 green for the win.

thedavidmeister’s picture

Status:Needs review» Reviewed & tested by the community

setting back to RTBC as per #25.

alexpott’s picture

Priority:Normal» Critical
Status:Reviewed & tested by the community» Active
Issue tags:-Needs reroll

Nice tests!

Committed 431dc2e and pushed to 8.x. Thanks!

Think we need a change notice here as the behaviour of drupal_render() has slightly changed.

alexpott’s picture

Priority:Critical» Normal
Status:Active» Needs work
StatusFileSize
new40.85 KB

Okay so this broke overlay... all content gets duplicated... see image for admin/content/node ...

Double_content_in_overlay.png

So reverting... committed aca6007 and pushed to 8.x.

thedavidmeister’s picture

That's super weird. I will look into this more. Hopefully if I figure out what happened to overlay I can see if it's something that's likely to effect other parts of core.

scor’s picture

Issue tags:+Needs change record

update tags (normalize to "Needs change notification")

thedavidmeister’s picture

I figured out the issue.

  $theme_is_implemented = FALSE;
   if (isset($elements['#theme']) && !isset($elements['#render_children'])) {

If #theme and #render_children are both set then $theme_is_implemented will be assumed as FALSE, which leads to double rendering of children.

We should start by assuming that $theme_is_implemented = isset($elements['#theme']) and then re-assess after theme() has been called.

Patch coming soon.

thedavidmeister’s picture

StatusFileSize
new9.13 KB
FAILED: [[SimpleTest]]: [MySQL] 57,864 pass(es), 19 fail(s), and 0 exception(s).
[ View ]

patch attached that fixes the overlay doubling bug and includes an extra test to ensure that when #theme is set and #render_children is TRUE we dont... render the children. Maybe that variable should be called #children_rendered or something?

thedavidmeister’s picture

or maybe #skip_theme

thedavidmeister’s picture

Status:Needs work» Needs review
thedavidmeister’s picture

Status:Needs review» Needs work

this needs work. I don't think what I did is quite right.

thedavidmeister’s picture

Status:Needs work» Needs review
StatusFileSize
new9.89 KB
FAILED: [[SimpleTest]]: [MySQL] 57,781 pass(es), 20 fail(s), and 0 exception(s).
[ View ]

actually, it is fine I think.

I was concerned that maybe I should wrap:

    foreach ($children as $key) {
      $elements['#children'] .= drupal_render($elements[$key]);
    }

In an additional check for #children being an empty string. Essentially:

array(
  '#children' => 'foo',
  'child' => array('#markup' => 'bar'),
);

Should this render 'foo', 'bar' or 'foobar'? In the end, I felt that 'foobar' is simplest, seems to fit the current docs best and would be less surprising so I added a test for it :)

Status:Needs review» Needs work

The last submitted patch, 2012812-41.patch, failed testing.

thedavidmeister’s picture

Status:Needs work» Needs review
StatusFileSize
new10.38 KB
PASSED: [[SimpleTest]]: [MySQL] 56,654 pass(es).
[ View ]

It's exactly the thinking in #42 that led to double overlay - glad I wrote a test for it :)

thedavidmeister’s picture

StatusFileSize
new7.01 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-43-44.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new11.01 KB
PASSED: [[SimpleTest]]: [MySQL] 56,616 pass(es).
[ View ]

consolidated and extended the test coverage.

Status:Needs review» Needs work

The last submitted patch, interdiff-43-44.patch, failed testing.

Cottser’s picture

Status:Needs work» Needs review
steveoliver’s picture

Yes, renaming #render_children to #skip_theme would at least help this mess. Other than that, #44 looks good to me.

thedavidmeister’s picture

I think actually renaming that variable should be a different issue as it could easily cause this bugfix to be blocked on bikeshedding.

steveoliver’s picture

Status:Needs review» Reviewed & tested by the community

No need to change this internal property name, really. Let's get this in.

alexpott’s picture

Status:Reviewed & tested by the community» Active

Nice sleuthing... I've manual tested overlay and we don't have the doubling up anymore...

Committed 0329b22 and pushed to 8.x. Thanks!

jenlampton’s picture

Status:Active» Fixed

Think you meant to change this to fixed?

jhodgdon’s picture

Title:drupal_render() can't distinguish between empty strings from theme() from an implemented hook and no hook being matched» Change notice for: drupal_render() can't distinguish between empty strings from theme() and no hook being matched
Status:Fixed» Active

No, it should be active for the change notice.

tim.plunkett’s picture

Category:bug» task
Priority:Normal» Critical

Which are all critical tasks.

tim.plunkett’s picture

Title:Change notice for: drupal_render() can't distinguish between empty strings from theme() and no hook being matched» Change notice: drupal_render() can't distinguish between empty strings from theme() and no hook being matched
ekl1773’s picture

Assigned:Unassigned» ekl1773
Status:Active» Needs review

Summary:

Problem: If theme() returns an empty string for any reason, then drupal_render() ignores the theme function's decision not to render the children elements recursively renders the children anyway.
Solution: After clearing up some documentation confusion, the problem was solved by clarifying when to render children.

Before:

 
  // Call the element's #theme function if it is set. Then any children of the
  // element have to be rendered there. If the internal #render_children
  // property is set, do not call the #theme function to prevent infinite
  // recursion.
  if (isset($elements['#theme']) && !isset($elements['#render_children'])) {
    $elements['#children'] = theme($elements['#theme'], $elements);
  }
  // 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]);
    }
  }

After:

  // Call the element's #theme function if it is set. Then any children of the
  // element have to be rendered there. If the internal #render_children
  // property is set, do not call the #theme function to prevent infinite
  // recursion.
  if ($theme_is_implemented && !isset($elements['#render_children'])) {
    $elements['#children'] = theme($elements['#theme'], $elements);

    // If theme() returns FALSE this means that the hook in #theme was not found
    // in the registry and so we need to update our flag accordingly. This is
    // common for theme suggestions.
    $theme_is_implemented = ($elements['#children'] !== FALSE);
  }

  // If #theme is not implemented or #render_children is set and the element has
  // an empty #children attribute, render the children now. This is the same
  // process as drupal_render_children() but is inlined for speed.
  if ((!$theme_is_implemented || isset($elements['#render_children'])) && empty($elements['#children'])) {
    foreach ($children as $key) {
      $elements['#children'] .= drupal_render($elements[$key]);
    }
  }

Two commits

#33 was pushed to 8.x and broke the overlay- #36 found the fix: "If #theme and #render_children are both set then $theme_is_implemented will be assumed as FALSE, which leads to double rendering of children."
#50 patch was successfully committed to 8.x

This change affects: module developers. The behaviour of drupal_render() has slightly changed.

Cottser’s picture

Status:Needs review» Needs work

@ekl1773 - thanks for working on this! That looks a bit more like an issue summary to me. I think the change notice should be geared towards people affected by the change. Things like what comment the patch was committed in are IMO not relevant in that context.

In this case, it might be useful to use example code in the before/after. You could look at the tests in this patch to get an idea. You could show calling a theme function that is not implemented and add comments to indicate the behaviour before and after. Something like this (I'm not sure if this is accurate or not, and an example with children might be more instructive):

Before:

<?php
// $output is an empty string.
$output = theme('notimplemented');
?>

After:

<?php
// $output is FALSE.
$output = theme('notimplemented');
?>
thedavidmeister’s picture

theme(array('notimplemented'));

@Cottser, what you posted, without the array results in watchdog complaining about hooks not found.

thedavidmeister’s picture

Ok, so most of this issue was just figuring out how to fix a bug without breaking anything. Everything to do with the overlay being broken temporarily is now irrelevant - I simply didn't understand enough about the API at the time of filing that patch.

The summary of what has happened here:

Previously drupal_render() would render the children of a render element recursively and #markup if and only if #theme returned an empty string for any reason.

It is important that drupal_render() only attempts to handle child elements and #markup if theme() (called when #theme is set) never built markup via. a theme function or template.

Since it is entirely possible that a theme() function/template could render an empty string (this counts as theme() building markup), and this does not mean that the theme function/template was never called at all, there needed to be a way to differentiate between the lack of any theme implementation for an element and the implementation existing and being invoked but still returning an empty result.

The change has two parts:

- Now theme() returns FALSE when the theme hook/suggestion passed to theme() is not found in the registry and returns the return value of the theme function/template otherwise.
- drupal_render() will explicitly check for a FALSE return from theme() before rendering children or #markup, instead of assuming that an empty value automatically leads to invoking the "alternative" rendering process.

Incidentally (I don't know that this should be in the change notice), there's a very closely related bug still outstanding at #2061835: theme() doesn't enforce that what it returns is a string or FALSE.

Who does this effect? potentially nobody actually as the change is just designed to improve the internal logic of drupal_render(), but "themers" probably, the only way to be "stung" by this change is if you were abusing the API by making drupal_render() treat a render element's children differently by creating a theme() function that returns FALSE or an empty string instead of rendering children within the theme function.

Before:

<?php
// Output is 'foo'
$empty = array(
 
'#theme' => 'theme_returns_empty',
 
'#markup' => 'foo',
);

// Output is 'foo'
$empty = array(
 
'#theme' => 'theme_returns_empty',
 
'child' => array('#markup' => 'foo'),
);

// Output is 'foo'
$not_implemented = array(
 
'#theme' => array('theme_not_implemented'),
 
'#markup' => 'foo',
);
?>

After:

<?php
// Output is '' (an empty string)
$empty = array(
 
'#theme' => 'theme_returns_empty',
 
'#markup' => 'foo',
);

// Output is '' (an empty string)
$empty = array(
 
'#theme' => 'theme_returns_empty',
 
'child' => array('#markup' => 'foo'),
);

// Output is 'foo'
$not_implemented = array(
 
'#theme' => array('theme_not_implemented'),
 
'#markup' => 'foo',
);
?>
ekl1773’s picture

Thank you @thedavidmeister, for all the information, and thanks @Cottser for the pointers. You're right, I was writing an issue summary instead of a change notice! I'll wrap all this together and ping you.

jessebeach’s picture

<?php
// Output is '' (an empty string)
$empty = array(
  '#theme' => 'theme_returns_empty',
  '#markup' => 'foo',
);

// Output is 'foo'
$not_implemented = array(
  '#theme' => array('theme_not_implemented'),
  '#markup' => 'foo',
);

Cottser, I find this behavior confusing. Can you elaborate a bit more on why the return value would vary here because of value type provided in the #theme property?

Cottser’s picture

@jessebeach - I think this would do the same thing but I would definitely defer to @thedavidmeister on this topic :)

<?php
// Output is '' (an empty string)
$empty = array(
 
'#theme' => array('theme_returns_empty'),
 
'#markup' => 'foo',
);

// Output is 'foo'
$not_implemented = array(
 
'#theme' => array('theme_not_implemented'),
 
'#markup' => 'foo',
);
?>
thedavidmeister’s picture

#61 is correct and probably clearer than my examples :)

The assumption in the examples is that theme_returns_empty() is a function that exists and returns an empty string, while 'theme_not_implemented' is not an implemented theme hook.

The data type does change how #theme behaves subtly, but is irrelevant to this change notice.

jessebeach’s picture

<?php
// theme() intentionally returns an empty string. This should not be considered a falsey value. Output is '' (an empty string).
// Do not back off to the #markup key value.
$empty = array(
 
'#theme' => array('theme_returns_empty'),
 
'#markup' => 'foo',
);

// The theme function is not implemented, so theme() returns false. Output is 'foo'
// Back off to the #markup key value.
$not_implemented = array(
 
'#theme' => array('theme_not_implemented'),
 
'#markup' => 'foo',
);
?>

Ok, thanks for the further clarification. I tried to capture that extra input in the comments of the code example from #61.

ekl1773’s picture

Title:Change notice: drupal_render() can't distinguish between empty strings from theme() and no hook being matched» drupal_render() can't distinguish between empty strings from theme() and no hook being matched
Assigned:ekl1773» Unassigned
Priority:Critical» Normal
Status:Needs work» Fixed
Issue tags:-Needs change record

Created change record (thanks to @thedavidmeister, @jessebeach and @Cottser for helping to clarify).

https://drupal.org/node/2068529

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