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.Problem/Motivation
In \Drupal\Core\Render\Element::children(), while looping through the elements in the render array, there's an if/elseif statement which reads as follows (I've put line numbers at the start which reflect the state of the file at commit a19a3806e2, the HEAD of 8.8.x at time-of-writing)
81: if ($key === '' || $key[0] !== '#') {
82: if (is_array($value)) {
// ...
93: }
94: // Only trigger an error if the value is not null.
95: // @see https://www.drupal.org/node/1283892
96: elseif (isset($value)) {
97: trigger_error(new FormattableMarkup('"@key" is an invalid render array key', ['@key' => $key]), E_USER_ERROR);
98: }
99: }
The error message triggered on line 97 is misleading: it is being triggered because $value is set (line 96) but is not an array (line 82)... and in fact, $key is perfectly valid because of the check on line 81.
I was running into this error on a client site inherited from another vendor when a Drupal theme twig file included a Patternlab twig file (which involved some sort of custom patternlab/drupal integration), because at the time this piece of code ran, my array looked like...
[
// ...
'sidebar_resource_download_text' => new TranslatableMarkup('Download this resource'),
// ...
]
... so the check on line 81 passed ($key was not empty and did not start with a number-sign), the check on line 82 failed (a TranslatableMarkup object is not an array), and the elseif on line 96 passed ($value was set), so an error was triggered. That is to say, $key was valid, but $value was not. The message, 'User error: "sidebar_resource_download_text" is an invalid render array key'... was really confusing and sent me down the wrong debugging path for a while until I stepped through the loop in a debugger, and found the key was fine; it was the value that was wrong.
Speculating, it looks like that trigger_error was supposed to go with the outer if statement; but, probably due to a merge conflict, got attached to the inner if statement.
Proposed resolution
- Move the
elseif (isset($value)) { trigger_error(...); }to the outerifstatement - Optional: add an
else { trigger_error(...('Expected an array but got a @type', ['@type' => gettype($value)]), E_USER_ERROR); }to the innerifstatement
Remaining tasks
Write a patch- Review
- RTBC
- Determine if we need a release notes snippet, and if so, write one.
- Commit
User interface changes
No UI changes.
API changes
No API changes.
Data model changes
No data model changes.
Release notes snippet
To be done.
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | 3083690-12.patch | 735 bytes | jungle |
| #2 | 3083690-2--invalid-key-error-is-misleading.patch | 1.46 KB | mparker17 |
Issue fork drupal-3083690
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
mparker17Okay, here's a patch that both moves the check, and adds the optional "expected an array but got @type" error.
Comment #3
mparker17This patch Needs Review.
Comment #7
Kris77 CreditAttribution: Kris77 commented@mparker17 after apply your patch(with drupal 8.9.8) the site slow down and receive this error:
User error: "#theme" is an invalid render array key in Drupal\Core\Render\Element::children() (line 103 of core/lib/Drupal/Core/Render/Element.php).
User error: "#message_list" is an invalid render array key in Drupal\Core\Render\Element::children() (line 103 of core/lib/Drupal/Core/Render/Element.php).
User error: "#status_headings" is an invalid render array key in Drupal\Core\Render\Element::children() (line 103 of core/lib/Drupal/Core/Render/Element.php).
User error: "#cache" is an invalid render array key in Drupal\Core\Render\Element::children() (line 103 of core/lib/Drupal/Core/Render/Element.php).
User error: "#lazy_builder_built" is an invalid render array key in Drupal\Core\Render\Element::children() (line 103 of core/lib/Drupal/Core/Render/Element.php).
User error: "#attached" is an invalid render array key in Drupal\Core\Render\Element::children() (line 103 of core/lib/Drupal/Core/Render/Element.php).
User error: "#cache" is an invalid render array key in Drupal\Core\Render\Element::children() (line 103 of core/lib/Drupal/Core/Render/Element.php).
User error: "#attached" is an invalid render array key in Drupal\Core\Render\Element::children() (line 103 of core/lib/Drupal/Core/Render/Element.php).
Comment #10
klonosI tried to backport this to Backdrop, and confirm @Kris77's findings. I got a series (dozens) of errors of this type:
User error: [#some_element_property] is an invalid render array key. in element_children() (line 7390 of /app/docroot/core/includes/common.inc).Comment #11
klonosComment #12
jungleMaybe it's fine just to ignore invalid keys.
Comment #16
joachim CreditAttribution: joachim at Factorial GmbH commentedI don't think patch #12 is right. We shouldn't be ignoring invalid keys -- if there is an invalid key, then the developer has made a mistake in the output of their render array, and they need to be told so they can fix it.
The first part of patch #2 looks nearly right. The second part doesn't seem relevant to me -- I don't know when that condition would fire, but it's out of scope for this issue.
One other tweak -- the isset() should be kept, otherwise we'll be adding new errors for NULL values. (And perhaps we *should* do that -- but again, that is out of scope).
Tagging as novice:
- update patch #2
- remove the 2nd chunk
- preserve the isset() condition
- make an MR
Comment #19
gxleano CreditAttribution: gxleano at Factorial GmbH commentedComment #20
joachim CreditAttribution: joachim at Factorial GmbH commentedThanks for the MR. The @key needs to be preserved so the developer knows where the problem is.
Comment #21
gxleano CreditAttribution: gxleano at Factorial GmbH commentedComment #22
gxleano CreditAttribution: gxleano at Factorial GmbH commentedThanks @joachim to put an eye on it.
I've put back the
@keyand also updated the tests that go with it.Comment #23
smustgrave CreditAttribution: smustgrave at Mobomo commentedRan the test-only feature and got
So change has coverage. Think this is a nice little improvement
Comment #24
alexpottCommitted and pushed ee4eaba6ff to 11.x and 85d08d1cfb to 10.3.x. Thanks!
Can we get a follow-up to convert the 3 core instances of
trigger_error(new FormattableMarkup(totrigger_error(sprintf(- we should not using FormattableMarkup here.Comment #27
joachim CreditAttribution: joachim as a volunteer commentedYup, I spotted that but it was out of scope to change that here.
There is #2552163: Do not use FormattableMarkup in exceptions, trigger_error, and debug (the second pass).