Handing theme() function an empty string or NULL as its second parameter triggers a fatal error

Here's the call stack - contrived example, if in user.module you call theme('username', '');

Fatal error: Unsupported operand types in /home/ben/workspace/d7x/includes/theme.inc on line 817
Call Stack: 0.0000 96632 
1. {main}() /home/ben/workspace/d7x/index.php:0 0.0253 3342480 
2. menu_execute_active_handler() /home/ben/workspace/d7x/index.php:22 0.0306 3776128 
3. drupal_deliver_page() /home/ben/workspace/d7x/includes/menu.inc:495 0.0307 3777896 
4. drupal_deliver_html_page() /home/ben/workspace/d7x/includes/common.inc:2395 0.0309 3778952 
5. drupal_render_page() /home/ben/workspace/d7x/includes/common.inc:2500 0.0315 3888152 
6. block_page_build() /home/ben/workspace/d7x/includes/common.inc:5262 0.0420 4165992 
7. block_get_blocks_by_region() /home/ben/workspace/d7x/modules/block/block.module:268 0.0420 4166552 
8. block_list() /home/ben/workspace/d7x/modules/block/block.module:315 0.0420 4166808 
9. _block_render_blocks() /home/ben/workspace/d7x/modules/block/block.module:648 0.0420 4168408
10. module_invoke() /home/ben/workspace/d7x/modules/block/block.module:818 0.0420 4169816 
11. call_user_func_array() /home/ben/workspace/d7x/includes/module.inc:745 0.0421 4170592 
12. user_block_view() /home/ben/workspace/d7x/includes/module.inc:0 0.0423 4173288 
13. theme() /home/ben/workspace/d7x/modules/user/user.module:1347 0.0423 4175024 
14. theme_user_list() /home/ben/workspace/d7x/includes/theme.inc:881 0.0423 4176232 
15. theme() /home/ben/workspace/d7x/modules/user/user.module:1445 

The attached patch sidesteps the unsupported operand fatal error.

***

Related, perhaps. My understanding of #600974: theme() fails for theme functions taking one argument in renderable arrays is that a string is never, ever expected as a parameter of theme(), let alone an empty string. However: A string handed to theme() as a sole render element is turned into an array, $variables, with one value in it– the value of the string keyed to the name given to 'render_element' in our hook_theme() implementation.

This appears to be a magical and unintentional byproduct of this code:

  // If a renderable array is passed as $variables, then set $variables to
  // the arguments expected by the theme function.
  if (isset($variables['#theme']) || isset($variables['#theme_wrappers'])) {
    $element = $variables;
    $variables = array();
    if (isset($info['variables'])) {
      foreach (array_keys($info['variables']) as $name) {
        if (isset($element["#$name"])) {
          $variables[$name] = $element["#$name"];
        }
      }
    }
    else {
      $variables[$info['render element']] = $element;
    }
  }

Because isset($variables['#theme']) evaluates to true for any string.

Maybe we should stop all non-arrays, strings or empty strings or NULL alike, with watchdogged warning before this?

***

To recap, the current patch protects against fatal errors for empty string and NULL parameters passed to theme().

If (or before) this approach is accepted, we should decide whether we are going to:

1) Document that handing a string in turns it into the value for a single-element array, keyed with the name of the render element.
2a) Make it impossible (obviating this patch), or
2b) Tell people not to do this.
3) Pretend it's not there and go on with our merry lives.

OK, maybe if i didn't ask at 4am at IRC i'd get a better response for reviews, but furamag has reviewed, this prevents a fatal error from the theme layer, marking RTBC.

OK, that's fine, can we catch an exception here and not take down a whole site if something manages to give this a NULL value? Or have theme return nothing in that case?

Really, given all the contexts that theme() is used in, having it cause "Fatal error: Unsupported operand types" is not acceptable, even if giving it a NULL value is never supposed to happen.

typehinting is excellent, but it still returns a site-ending error:

Error
The website encountered an unexpected error. Please try again later.
Error message
Recoverable fatal error: Argument 2 passed to theme() must be an array, string given, called in /home/ben/workspace/dh/modules/user/user.module on line 1445 and defined in theme() (line 738 of /home/ben/workspace/dh/includes/theme.inc).

In general i absolutely agree that it is better to cause a fatal error than to silently fail, but theme() is a function that is way too common and handed data in too many ways. If it is given nothing it should return nothing; that's not a silent fail, that's just sensible. That makes more sense than its current behavior, which is to secretly convert strings, but choke on NULLs and empty strings. And it makes more sense to do no error or a softer error for non-arrays than it does to make the third-most common function in Drupal one of the most draconian in its response to unexpected input.

This patch has theme() immediately return nothing if given nothing.

Eh, crosspost. Still think that a function called thousands of times in unknowable situations should not destroy a site should it get NULL instead of an array.

Rudely bumping this to major to get one last review before Drupal 7 is released: What is handed to theme() can change at runtime, unlike most ways of blowing up a site. For the price of an isset() we can have theme('nothing_of_importance', $oops) simply go unprinted instead of causing a WSOD.

I really really hope i'm wrong and never have a chance to point a sobbing siteowner to this issue and say "Hey, i tried."

^{benjamin, agaric}