drupal_render() can't distinguish between empty strings from theme() and no hook being matched

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

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

hmmmmmmmmmmmmmmmmmmmmmm

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.

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

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

Related #2015311: If #theme suggestions are set as an array for theme() but none are implemented #markup is treated incorrectly by drupal_render()

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.

this one might actually pass tests...

simpler, might work.

Apologies for all the noise here!

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

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

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

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

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

setting back to RTBC as per #25.

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.

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.

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?

or maybe #skip_theme

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

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

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

consolidated and extended the test coverage.

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

theme(array('notimplemented'));

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

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:

// 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:

// 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',
);

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