Contrib.social

'"@key" is an invalid render array key' error in \Drupal\Core\Render\Element is misleading

Open on Drupal.org →
Created on 25 September 2019, over 5 years ago
Updated 30 March 2024, 9 months ago

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

  1. Move the elseif (isset($value)) { trigger_error(...); } to the outer if statement
  2. Optional: add an else { trigger_error(...('Expected an array but got a @type', ['@type' => gettype($value)]), E_USER_ERROR); } to the inner if statement

Remaining tasks

  2. Review
  3. RTBC
  4. Determine if we need a release notes snippet, and if so, write one.
  5. 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.

📌 Task
Status

Fixed

Version

10.3 ✨

Component
Render  →

Last updated about 8 hours ago

Created by

🇨🇦Canada mparker17 UTC-4

Live updates comments and jobs are added and updated live.
  • DrupalWTF

    Worse Than Failure. Approximates the unpleasant remark made by Drupal developers when they first encounter a particular (mis)feature.

  • Novice

    It would make a good project for someone who is new to the Drupal contribution process. It's preferred over Newbie.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment 10 months ago →
    🇬🇧United Kingdom joachim

    I 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

  • First commit to issue fork.
  • Merge request !6937Issue #3083690 by gxleano: Update trigger error message → (Open) created by gxleano
  • Status changed to Needs review 10 months ago
  • Comment 10 months ago →
    🇪🇸Spain gxleano Cáceres
  • Status changed to Needs work 10 months ago
  • Comment 10 months ago →
    🇬🇧United Kingdom joachim

    Thanks for the MR. The @key needs to be preserved so the developer knows where the problem is.

  • Pipeline finished with Failed
    10 months ago
    #112681
  • Pipeline finished with Failed
    10 months ago
    Total: 553s
    #112707
  • Pipeline finished with Success
    10 months ago
    Total: 483s
    #112764
  • Status changed to Needs review 10 months ago
  • Comment 10 months ago →
    🇪🇸Spain gxleano Cáceres
  • Comment 10 months ago →
    🇪🇸Spain gxleano Cáceres

    Thanks @joachim to put an eye on it.

    I've put back the @key and also updated the tests that go with it.

  • Status changed to RTBC 10 months ago
  • Comment 10 months ago →
    🇺🇸United States smustgrave

    Ran the test-only feature and got 

    1) Drupal\Tests\Core\Render\ElementTest::testInvalidChildren
Failed asserting that exception message '"foo" is an invalid render array key' contains '"foo" is an invalid render array key. Value should be an array but got a string'.
/builds/issue/drupal-3083690/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/builds/issue/drupal-3083690/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/builds/issue/drupal-3083690/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
/builds/issue/drupal-3083690/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/builds/issue/drupal-3083690/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
/builds/issue/drupal-3083690/vendor/phpunit/phpunit/src/TextUI/Command.php:144
/builds/issue/drupal-3083690/vendor/phpunit/phpunit/src/TextUI/Command.php:97
FAILURES!
Tests: 38, Assertions: 51, Failures: 1.

    So change has coverage. Think this is a nice little improvement

  • Status changed to Fixed 10 months ago
  • Comment 10 months ago →
    🇬🇧United Kingdom alexpott 🇪🇺🌍

    Committed 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( to trigger_error(sprintf( - we should not using FormattableMarkup here.

  • Comment 10 months ago →
    System Message
    • alexpott → committed 85d08d1c on 10.3.x 
      Issue #3083690 by gxleano, mparker17, jungle, joachim, Kris77: '"@key"...
  • Comment 10 months ago →
    System Message
    • alexpott → committed ee4eaba6 on 11.x 
      Issue #3083690 by gxleano, mparker17, jungle, joachim, Kris77: '"@key"...
  • Comment 10 months ago →
    🇬🇧United Kingdom joachim

    Yup, 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) → .

  • Comment 9 months ago →
    System Message

    Automatically closed - issue fixed for 2 weeks with no activity.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024