Let Render API fail in a tale-telling way on invalid $element

Here is an attempt at a test. Except that the drupal_set_message doesn't seem to be a part of the page. debug(drupal_get_messages()) showed the message just fine, but debug($this->drupalGetContent()); showed nothing.

Per xjm's suggestion, just resorting to using in_array and assertTrue.
Attaching both test, and test with fix.

+++ b/modules/simpletest/tests/common.testundefined
@@ -1708,6 +1708,20 @@ class DrupalRenderTestCase extends DrupalWebTestCase {
+    // Define an element with an invalid key.
+    $key = 'not valid';
+    $element = array(
+      $key => 'fail',

A first reading of this would lead the user to believe that one cannot use the key 'not valid' in a render array. However this is not the case. The real issue is that the value of the key is a string (and not an array).

Since the tests are a valuable resource for figuring out how to write Drupal code properly, I suggest we change the comments to reflect the real cause of the error:

$value = 'should be an array, not a string';
$element = array(
  'child' => $value,
);

(or similar).

#3 is very true. Also, it should likely be an error, not a status.

+1'ing, if anyone is looking for a way to reproduce. Open up node_view_multiple() in node.module, take out the "#" in ['#weight'] and view /node on your site. You should get a bunch of ugly errors. This patch turns those errors into tasty messages.

This patch already exposed an invalid render api usage in toolbar: #1285098: Toolbar Drawer uses invalid Render API key

Hmm, on re-reading I understand now the original intent about invalid keys --- '#weight' is a valid key, but 'weight' is not.

Essentially we'd like the test to show that keys which start with a # are generically valid, but things which don't are (usually) treated as a child element which must be a render array. Hmf, confusing!

I guess I could add similar assertFalse()'s for $element['#key'] = 'string'; and $element['key'] = array();?
Is that overkill?

I think just a nice comment would suffice, or test something like

$element = array(
  '#key' => 'value', // okay
  'weight' => 10, // wrong, missing #
  'child1' => array(), // okay
  'child2' => 'fail', // wrong, not a renderable array
);

Or whatever. Sorry for chiming in here to only add some extra confusion. The original patch was probably good enough.

Expanded the comment a bit. I think this is ready to go.

#10: drupal-1283892-10.patch queued for re-testing.

Ugh sorry for the noise, I'd left a debug_backtrace in there.

To clarify, the current error for a FAPI-based invalid key is

Warning: Invalid argument supplied for foreach() in element_children() (line 6141 of /var/www/d7/includes/common.inc).
Warning: Cannot use a scalar value as an array in drupal_render() (line 5629 of /var/www/d7/includes/common.inc).

which doesn't actually tell you what broke, and continues to try to use the key.

This patch prevents that error, and displays

$key is an invalid render array key.

where $key is the offending key.

In certain cases of standalone Render API usage (non-FAPI), there is currently no error, the key is silently skipped. After the patch, the key is still skipped, but the error is shown.

Tagging for possible backport.

The tests pass and the code makes sense while doing a readthrough. Marking it at RTBC.

I don't agree with using drupal_set_message() here, that means nothing in watchdog and it will always show to end users regardless of error reporting settings.

Why not trigger_error() or similar?

Switching the render over to a menu callback was the only way I could catch the error.

The last patch exposed 3 new bugs. One in theme_aggregator_categorize_items(), one in theme_system_date_time_settings(), and one menu_overview_form_submit(). The last has it's own issue #1291528: menu_overview_form_submit() breaks element_children(), the first two do not yet. All three fixes are included to see if the tests pass.

Based on the diff, there's no way we could possibly backport this to D7. :\ Looks like contributed modules would almost certainly require updating.

Leaving off the backport tag, but for D7 I can reroll without the trigger_error in element_children, and without the test, and we'll be in good shape.

Looks like contributed modules would almost certainly require updating.

This is only half true... a correctly functioning contributed module shouldn't need updating. Only the broken ones would.

Yes, but poor end users are victim to Drupal spewing out errors suddenly on a minor point release of modules that were 'working' (for some values of working) fine before then. We keep doing that (like in 7.8 when suddenly Media module exploded in flames) and people will stop using Drupal 7...

However, I'm fully supportive of some means via, say, Devel module or similar, to provide this kind of extra checking in D7.

With media it was an exception which gets spewed everywhere, this is just a php warning which should not. One option would be to make it E_STRICT for D7 then it's likely to be ignored for anyone who doesn't care about it.

Ooh, E_STRICT... brilliant!

E_STRICT doesn't work with trigger_error. Only E_USER_ERROR, E_USER_WARNING, and E_USER_NOTICE do.
http://php.net/manual/en/function.trigger-error.php

WHOA crazy interesting bug. At least to me.

Simplification of theme_aggregator_categorize_items:

$output = '';
$element = array();
$element['something'] = array('#markup' => 'Valid');
$output .= drupal_render($element['missing']);
print_r($element);
$output .= drupal_render_children($element);
print_r($output);

First print_r:

Array
(
    [something] => Array
        (
            [#markup] => Valid
        )

    [missing] => 
)

Second print_r:

Valid

$element['missing'] doesn't exist at first. But because drupal_render works by reference, and returns NULL, all of a sudden it IS there. And then drupal_render_children fails for the same reason.

The only solution I can think of is silently ignoring 'key' => NULL. So changing the else to elseif (isset()), or the like.

Turns out two of the earlier "fixes" are covered by the added isset(), so this is now just the change to element_children, the test, and the array_intersect_key fix.

ARGH. Wrong patch.

Added a comment.

Also, see http://www.phpwtf.org/more-array-references for a discussion of why this is crazy.

To note, this cannot be backported to D7 unless #1285098: Toolbar Drawer uses invalid Render API key is committed first.

Would it be possible to do the test with set_error_handler() and errorException - seems like we could catch the error and pass/fail based on that, would make this nearly a unit test then rather than having to make the http request to read the error message off it.

See #1247666: Replace @function calls with try/catch ErrorException blocks for more.

If the answer is "no", or "after that patch is in" then I can live with it as is though.

Using ErrorException seems like it will be a big improvement in the future, but nothing like drupal_exception_error_handler exists yet. Posting to the other issue to revisit this if we move in that direction.

OK that's fair enough, adding the new API just for the test is a bit much, and we've already got that issue open.

Committed to 8.x, thanks!

Hmm.. This is a pretty major API change against render arrays, and broke both the Render Example and the Theming Example for D8. Is there an API change notice on this one?

User error: "0" is an invalid render array key in element_children() (line 6166 of includes/common.inc).

The related issue is #1304502: [critical] Theming Example and Render Example are broken

As I understood it, it wasn't technically an API change, but a clarification/enforcement.

The relevant breaking code in the Examples module is here: http://drupalcode.org/project/examples.git/blob/refs/heads/8.x-1.x:/rend...

That example doesn't follow the strict rules of render arrays, though it seems like a reasonable usage.

If the following code should be allowed, then this has to be rolled back and rethought.
Otherwise, this does need an API change notice, and Examples will need to change.

$element = array(
  t('theme for an element') => array(
    'child' => array(
      t('This is some text that should be put together'),
      t('This is some more text that we need'),
    ),
    '#separator' => ' | ',  // Made up for this theme function.
    '#theme' => 'render_example_aggregate',
  ),
);

function theme_render_example_aggregate($variables) {
  $output = '';
  foreach (element_children($variables['element']['child']) as $item) {
    $output .= $variables['element']['child'][$item] . $variables['element']['#separator'];
  }
  return $output;
}

I posted [#1311610].
My first change notice, so I'm not sure that I did it properly.

With the change record created, now its time to figure out how to fix this in Examples...

@vilepickle

I have faced your custom case (and maybe this might be useful to someone else too).
Quick way to override - is to convert all custom array values to array of arrays containing "#markup" => $value,
Something like this:

// FROM:
$data = [
  'user_id'   => $user->id(),
  'image_url' => $image_url,
];
// TO:
$data = [
  'user_id'   => ['#markup' => $user->id()],
  'image_url' => ['#markup' => $image_url],
];

At least this helped me getting over "fatal: user_id is an invalid render array key".