Overview

Reproducible in 0.x HEAD right now:

$ ddev test tests/src/Kernel/Config/JavaScriptComponentValidationTest.php 
PHPUnit 10.5.45 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.19
Configuration: /var/www/html/web/core/phpunit.xml.dist

DDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDDFD          56 / 56 (100%)

Time: 01:47.778, Memory: 6.00 MB

There was 1 failure:

1) Drupal\Tests\experience_builder\Kernel\Config\JavaScriptComponentValidationTest::testInvalidSlotIdentifiedByConfigSchema
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
 Array &0 [
+    '' => '[slots.test-slot] Array value found, but an object is required',
     'slots.test-slot' => ''title' is a required key.',
 ]

/var/www/html/web/core/tests/Drupal/KernelTests/Core/Config/ConfigEntityValidationTestBase.php:413
/var/www/html/web/modules/contrib/experience_builder/tests/src/Kernel/Config/BetterConfigEntityValidationTestBase.php:21
/var/www/html/web/modules/contrib/experience_builder/tests/src/Kernel/Config/JavaScriptComponentValidationTest.php:792
/var/www/html/web/modules/contrib/experience_builder/tests/src/Kernel/Config/JavaScriptComponentValidationTest.php:748

Proposed resolution

User interface changes

Issue fork experience_builder-3523978

Command icon 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:

About issue forks

Comments

longwave created an issue. See original summary.

isholgueras’s picture

isholgueras commented

I can't reproduce. It runs well for me in 0.x:

╭─    ~/apps/contrib/drupal11/web/modules/contrib/experience_builder    0.x ·························· ✔  17:23:44  ─╮
╰─ g pull --prune --rebase                                                                                                  ─╯
Already up to date.

╭─    ~/apps/contrib/drupal11/web/modules/contrib/experience_builder    0.x ·························· ✔  17:23:49  ─╮
╰─ ddev phpunit tests/src/Kernel/Config/JavaScriptComponentValidationTest.php                                               ─╯
PHPUnit 10.5.46 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.19
Configuration: /var/www/html/web/core/phpunit.xml.dist

................................D.D.....................          56 / 56 (100%)

Time: 01:35.943, Memory: 6.00 MB

2 tests triggered 1 PHP deprecation:

1) /var/www/html/web/core/modules/options/src/Plugin/Field/FieldType/ListItemBase.php:514
Implicit conversion from float 3.14 to int loses precision

Triggered by:

* Drupal\Tests\experience_builder\Kernel\Config\JavaScriptComponentValidationTest::testInvalidEnumsAndExamples#Invalid string
  /var/www/html/web/modules/contrib/experience_builder/tests/src/Kernel/Config/JavaScriptComponentValidationTest.php:145

* Drupal\Tests\experience_builder\Kernel\Config\JavaScriptComponentValidationTest::testValidEnumsAndExamples#3
  /var/www/html/web/modules/contrib/experience_builder/tests/src/Kernel/Config/JavaScriptComponentValidationTest.php:121

OK, but there were issues!
Tests: 56, Assertions: 164, Deprecations: 1.

What I see is that I have this specific issue reproducible in 2 different unrelated builds that are not updated with 0.x at this time:

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: JavaScriptComponentValidationTest::testInvalidSlotIdentifiedByConfigSchema fails » [11.1.7 regression] JavaScriptComponentValidationTest::testInvalidSlotIdentifiedByConfigSchema fails

99% likely due to https://www.drupal.org/project/drupal/releases/11.1.7 having been released, and CI just having started using it by default.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Related issues: +#3517317: SDC slots not being validated against json config schema

Reverting #3517317: SDC slots not being validated against json config schema fixes the test.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

To me though the strange thing is:

    $this->entity->set('slots', ['test-slot' => []]);

causes the error [slots.test-slot] Array value found, but an object is required but

    $this->entity->set('slots', ['test-slot' => ['title' => 'test']]);

does not - @isholgueras suggests this may be due to json_encode() where an empty array is not converted to an object, but an associative array is.

If that is the case then just expecting the error on >= 11.1.7 seems wrong - instead we should fix this at the point of encoding if we can?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

instead we should fix this at the point of encoding if we can?

+1

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Did some debugging but we aren't really in control of that, and it's going to be hard to change.

Core's ComponentValidator does this:

    $definition_object = Validator::arrayToObjectRecursive($definition);
    $this->validator->reset();
    $this->validator->validate(
      $definition_object,
      (object) ['$ref' => 'file://' . dirname(__DIR__, 5) . '/assets/schemas/v1/metadata-full.schema.json']
    );

Validator::arrayToObjectRecursive() is from justinrainbow/json-schema. I think we can probably argue that it is doing the right thing:

> \JsonSchema\Validator::arrayToObjectRecursive(['slots' => []]);
= {#8010
    +"slots": [],
  }

> \JsonSchema\Validator::arrayToObjectRecursive(['slots' => ['test' => 'slot']]);
= {#7977
    +"slots": {#8011
      +"test": "slot",
    },
  }

It can't know at this point whether an empty array should stay as an empty array or be cast to an empty object - there are likely just as many cases where an empty array is correct here.

Also, because ::arrayToObjectRecursive() works by encoding and decoding JSON, even if we force an empty slots to an object first, that is lost and it is cast back to an empty array.

This is wrong, followup coming.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

We might want to raise this upstream and solve it in core in ComponentValidator, but we can fix this for us in ::toSdcDefinition():

    if ($this->slots) {
      foreach ($this->slots as $slot_name => $slot) {
        // Force empty slots to be an object; ComponentValidator casts non-
        // empty arrays to objects, but empty arrays trigger a false positive
        // validation error: "Array value found, but an object is required".
        if ($slot === []) {
          $slot = new \stdClass();
        }
        $definition['slots'][$slot_name] = $slot;
      }
    }

longwave opened merge request !1022

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Needs review
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs followup

As soon as there's a core follow-up + our code pointing to it with a // @todo Remove in <core issue URL>, this is IMHO ready to go 👍

Epic digging, @longwave! 👏

tedbow made their first commit to this issue’s fork.

  • tedbow committed ccbae46e on 0.x authored by longwave 
    Issue #3523978 by longwave, wim leers, isholgueras: [11.1.7 regression]...
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Assigned: Unassigned » longwave
Status: Reviewed & tested by the community » Needs work

I merged this as is because there some issues I think I would like to merge and I don't feel qualified to make the core follow-up. I would rather not merged the other issue with failing phpunit tests.

so setting back to Needs Work and assigning to @longwave to create another MR for the core follow-up link

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Good call, @tedbow :) Thanks! 🙏

longwave opened merge request !1026

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Assigned: longwave » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs followup

Opened #3524163: [regression] Empty slot definitions give incorrect validation message and linked it from a comment in MR!1026.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs review » Reviewed & tested by the community

Looks good

  • tedbow committed a09c12f9 on 0.x authored by longwave 
    Issue #3523978 by longwave, tedbow, wim leers, isholgueras: [11.1.7...
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Reviewed & tested by the community » Fixed
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#3524163: [regression] Empty slot definitions give incorrect validation message
penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Related issues: +#3519891: Constrain slot names allowed by XB in Components (and in its component tree)

Status: Fixed » Closed (fixed)

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