Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I am sure I filed this before but for my life can't find it. In the spirit of #1067750: Let Field API fail in a tale-telling way on invalid $entity let's add better error handling to element_children().
Comment | File | Size | Author |
---|---|---|---|
#39 | drupal-1283892-39.patch | 3.89 KB | tim.plunkett |
#38 | drupal-1283892-38.patch | 3.79 KB | tim.plunkett |
#37 | drupal-1283892-25.patch | 5.05 KB | tim.plunkett |
#25 | drupal-1283892-25.patch | 5.05 KB | tim.plunkett |
#23 | drupal-1283892-23.patch | 3.05 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettHere 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, butdebug($this->drupalGetContent());
showed nothing.Comment #2
tim.plunkettPer xjm's suggestion, just resorting to using in_array and assertTrue.
Attaching both test, and test with fix.
Comment #3
bfroehle CreditAttribution: bfroehle commentedA 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:
(or similar).
Comment #4
tim.plunkett#3 is very true. Also, it should likely be an error, not a status.
Comment #5
fakingfantastic CreditAttribution: fakingfantastic commented+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.
Comment #6
tim.plunkettThis patch already exposed an invalid render api usage in toolbar: #1285098: Toolbar Drawer uses invalid Render API key
Comment #7
bfroehle CreditAttribution: bfroehle commentedHmm, 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!
Comment #8
tim.plunkettI guess I could add similar assertFalse()'s for
$element['#key'] = 'string';
and$element['key'] = array();
?Is that overkill?
Comment #9
bfroehle CreditAttribution: bfroehle commentedI think just a nice comment would suffice, or test something like
Or whatever. Sorry for chiming in here to only add some extra confusion. The original patch was probably good enough.
Comment #10
tim.plunkettExpanded the comment a bit. I think this is ready to go.
Comment #12
tim.plunkett#10: drupal-1283892-10.patch queued for re-testing.
Comment #14
tim.plunkettUgh sorry for the noise, I'd left a debug_backtrace in there.
Comment #15
tim.plunkettTo clarify, the current error for a FAPI-based invalid key is
which doesn't actually tell you what broke, and continues to try to use the key.
This patch prevents that error, and displays
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.
Comment #16
BTMash CreditAttribution: BTMash commentedThe tests pass and the code makes sense while doing a readthrough. Marking it at RTBC.
Comment #17
catchI 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?
Comment #18
chx CreditAttribution: chx commentedSo, you are in the do or do not, there is no try camp? Sure! Let's throw exceptions and add array typing to everything that expects an element. Leave no argument behind.
Comment #19
chx CreditAttribution: chx commentedA quick review shows for example
render()
accepting strings so I would say only the two most important ones need an array marker: drupal_render and form_builder. If you add that then my patch becomes totally unnecessary.Comment #20
chx CreditAttribution: chx commentedHrm, that would leave out the $key. OK. Let's try then trigger_error with an E_USER_ERROR level and dont add arrays.
Comment #21
chx CreditAttribution: chx commentedThe test will fail btw.
Comment #23
tim.plunkettSwitching the render over to a menu callback was the only way I could catch the error.
Comment #25
tim.plunkettThe 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.
Comment #26
webchickBased on the diff, there's no way we could possibly backport this to D7. :\ Looks like contributed modules would almost certainly require updating.
Comment #27
tim.plunkettLeaving 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.
Comment #28
bfroehle CreditAttribution: bfroehle commentedThis is only half true... a correctly functioning contributed module shouldn't need updating. Only the broken ones would.
Comment #29
webchickYes, 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.
Comment #30
catchWith 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.
Comment #31
bfroehle CreditAttribution: bfroehle commentedOoh, E_STRICT... brilliant!
Comment #32
tim.plunkettE_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
Comment #33
chx CreditAttribution: chx commentedWe could add a recursive walker to devel module to check for erroneous keys. It'd be blatantly trivial. Very very nice work here with that array_intersect_key . I do not get the aggregator problem because drupal_render starts with
Comment #34
tim.plunkettWHOA crazy interesting bug. At least to me.
Simplification of theme_aggregator_categorize_items:
First print_r:
Second print_r:
$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.Comment #35
tim.plunkettThe only solution I can think of is silently ignoring
'key' => NULL
. So changing the else to elseif (isset()), or the like.Comment #36
chx CreditAttribution: chx commentedHrm. OK. We can add that elseif isset. While form_builder and _form_validate doesnt have the kind of passed-in NULL protection drupal_render has, those arent called from the outside so this artefact wont affect them. Congrats on finding a huge PHP WTF.
Comment #37
tim.plunkettTurns 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.
Comment #38
tim.plunkettARGH. Wrong patch.
Comment #39
tim.plunkettAdded a comment.
Also, see http://www.phpwtf.org/more-array-references for a discussion of why this is crazy.
Comment #40
tim.plunkettTo note, this cannot be backported to D7 unless #1285098: Toolbar Drawer uses invalid Render API key is committed first.
Comment #41
chx CreditAttribution: chx commentedThis is now ready.
Comment #42
catchWould 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.
Comment #43
catchComment #44
tim.plunkettUsing 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.
Comment #45
catchOK 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!
Comment #47
rfayHmm.. 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
Comment #48
tim.plunkettAs 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.
Comment #49
tim.plunkettIf 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.
Comment #50
chx CreditAttribution: chx commentedThat's a completely illegal render element. Just because you rape the system by writing a theme function that renders it , doesn't mean it's valid :) edit: legal render elements render without any theme functions. theme functions are not supposed to change structure.
Comment #51
tim.plunkettI posted [#1311610].
My first change notice, so I'm not sure that I did it properly.
Comment #52
tim.plunkettWith the change record created, now its time to figure out how to fix this in Examples...
Comment #55
Anonymous (not verified) CreditAttribution: Anonymous commentedWhat is the best way to override this in a custom case for form rendering? I have some elements in my $form array that are from $form_state that a custom module uses (Inline Entity Form). The reason they are there is to pre-populate the references on a cloned form...
The error is triggering because the form elements aren't "renderable", but it doesn't seem to actually matter.
Comment #56
q11q11 CreditAttribution: q11q11 as a volunteer commented@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:
At least this helped me getting over "fatal: user_id is an invalid render array key".