Element::properties() produces notices if given an array with integer keys

Hi,

I am applying a patch for this issue.

Kindly review the patch once.

Thanks.

Uploading new patch.

Uploading new patch.

Uploading new patch.

Nice catch, this should definitely be fixed.

Thanks for getting this started @Pooja Ganjage! I'm not sure exactly what is going wrong, but somehow you're patches cannot be applied, not sure if there's a way to assist with that.

I think similar to what was done in #3086374: Make Drupal 8 & 9 compatible with PHP 7.4 we should add an is_int() check to the code.

Also, this will need a test. Should be easy to simply add another line to \Drupal\Tests\Core\Render\ElementTest::testChild().

@Poonja Ganjage: When I look at the latest 9.1.x-branch I see this code in core/lib/Drupal/Core/Render/Element.php:

  public static function child($key) {
    return !isset($key[0]) || $key[0] != '#';
  }

The code in your patch is looking for this line to replace:

- return (!isset($key === '' || (0 != strpos($key, '#')));

which isn't there, which makes the patch not applicable.

Not sure on what branch you created your patch, but it should be against the latest 9.1.x-branch.

Hope that helps! :)

Sorry previous applied patch was zero bytes applying updated patch

patch #11 worked for me.

Following @tstoeckler suggestion in [3172166-8] Checking for is_int first. This makes sense for all possible keys passed anyway.

For the tests it is to be expected that an array passed into ::properties() could have mixed keys, so a test added there, and ::property() can just have the one key passed (from anyone doing something like properties() for example).

So attached patch updates the return to check it's not an integer first; and tests both methods. I've run tests against 9.4.x-dev. So I'll leave it at that first; but I think this will apply to 10.0.x-dev too.

Thanks! Looks good to me.

Nice find and nice test coverage!

+++ b/core/lib/Drupal/Core/Render/Element.php
@@ -24,7 +24,7 @@ class Element {
-    return $key[0] == '#';
+    return !is_int($key) && $key[0] == '#';

How about is_string($key) && $key[0] == '#' - it has less logic - the lack of the ! - and is more inline with the $key documentation. Array keys have to a string or an integer. Even floats don't work.

> Array keys have to a string or an integer. Even floats don't work.

Just read those casting rules. Sure there's some potential fun there - but for this it's straightforward.

Thanks for the review @alexpott and the quick turnaround @ekes. Marking RTBC assuming the bot passes.

Committed and pushed 60a42060171 to 10.0.x and ed0195c2bf2 to 9.4.x and 089376cf5ed to 9.3.x. Thanks!

I believe this issue has introduced a regression on PHP 7.3:
https://www.drupal.org/pift-ci-job/2305316

Here's a patch to test the revert.

That fixes PHP 7.3 tests, so I've reverted it from all three branches. So, we'll need to come up with a way to make the tests pass on PHP 7.3 in Drupal 9.3 and 9.4 while still fixing the existing notice on PHP 7.4. Be sure to run tests against environments for all three PHP versions before commit. (That's always a good idea anyway for any issue where we're fixing something PHP-version-specific.) Thanks!

...I guess I probably didn't need to revert it on 10.0.x since it requires PHP 8, but maybe there's a better way to update the test anyway. Up to whoever picks this back up. Thanks!

Added a 7.3 test to #3172166-19: Element::properties() produces notices if given an array with integer keys but assuming that fails it will be because of $this->assertNotContains(0, $properties); which must be some feature of assertNotContains with a (int) 0 and PHP 7.3. The code being tested runs the same from 5.4 onward https://3v4l.org/fn61K

As the $this->assertCount(2, $properties); was also added, and the code checks for properties then containing the two expected keys, checking that the other two are not there is extraneous.

So assuming above fails, this one should pass. I'll run it against 7.3. 7.4 and 8.

+++ b/core/tests/Drupal/Tests/Core/Render/ElementTest.php
@@ -29,13 +30,14 @@ public function testProperties() {
+    $this->assertCount(2, $properties);
     $this->assertContains('#property1', $properties);
     $this->assertContains('#property2', $properties);
-    $this->assertNotContains('property3', $properties);

Let's replace all this with

    $this->assertSame(['#property1', '#property2'], $properties);

It's simpler and works on all PHPs.

I also think we should file a follow up to do:

   public static function child($key) {
-    return !isset($key[0]) || $key[0] != '#';
+    return !static::property($key);
   }

The Element::child() must be the inverse of Element::property() - so let's make this true in code.

Also in Element::children() we have if (is_int($key) || $key === '' || $key[0] !== '#') { I guess we're inlining this here for performance - which makes sense (up to a point - this stuff is called thousands of times so often the measuring makes it seem way way slower than it actually is). But regardless - we can still make this more efficient by doing if (!(is_string($key) && $key[0] !== '#')) {. We could include changing this logic in the same followup - the follow could be called something like "Align the property / child determination in Element class". Note the performance arguments for Element::child() don't really hold based on core usage.

#33 looks great.

Tests now passing on both for PHP 7.3 through 8. Follow-up posted #3262736: Align the property / child determination in Element class.

Awesome, thanks! Back to RTBC then

Committed and pushed e14d16c5894 to 10.0.x and 1538662f475 to 9.4.x and e71fcde4d2c to 9.3.x. Thanks!