from @balintbrews
Sending values for enum or examples of JS component props as integers/numbers using saves them as strings. The effect of this is that the component input form we generate can’t set an example/default value for integer/number props with enum. I didn’t test if this is an issue with the HTTP API of the config entity implementation itself.
This can actually be seen in \Drupal\Tests\experience_builder\Functional\XbConfigEntityHttpApiTest::testJavaScriptComponent
The prop is sent as
'integer' => [
'type' => 'integer',
'title' => 'Integer',
'examples' => [23, 10, 2024],
],
but returned as
integer' => [
'title' => 'Integer',
'type' => 'integer',
'examples' => ['23', '10', '2024'],
],
My guess would be that because the schema is define as
examples:
type: sequence
label: 'Examples'
sequence:
type: string
enum:
requiredKey: false
type: sequence
sequence:
type: string
where both are string the config system will case them as strings
Step debugging it seems this happens at save, $xb_config_entity->save();.
return these as the correct type
Show commandsStart 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
tedbowComment #3
tedbowComment #5
tedbowgot it started. add comment about needed test coverage. It seems like `\Drupal\experience_builder\Entity\JavaScriptComponent::normalizeForClientSide` is the right place but maybe we want to refactor that into another method.
Do we need to validate that only integers and numbers are sent in the request if that matches the type?
Comment #6
wim leersGreat catch!
Another missed defect in #3499931: HTTP API for code component config entities (actually even before that, in #3499927: Config entity for storing code components, but there it was less obvious than the literally incorrect example in the test that @tedbow quoted in the issue summary 👍).
Looking at your analysis so far, I wonder if this is actually a pure config schema problem 🤔
Comment #7
wim leersYep, confirmed: this is a pure config schema problem. It's quite interesting, really: we need to describe in config schema how to specify a valid JSON Schema, for the subset of it that XB supports. 🤓🤯😄
Tests + fixes incoming.
Comment #8
wim leersThe stricter validation this introduces even uncovered multiple test coverage bugs that @f.mazeikis and I missed in #3507929: Allow adding "image" props in the code component editor yesterday! 😱🥳
P.S.: This is what I was hoping/expecting to see on #3499927: Config entity for storing code components, but didn't ask for/block it on because it'd have delayed all of #3499919: [Meta] Plan for in-browser code components.
Comment #9
wim leersCI agrees! (
component-operations.cy.jsfails on CI, but passes locally. It's been a known fairly frequent random failure for a while.)I'd like @tedbow to do final sign-off given he started this MR and I took it in a very different direction. You don't need to review every detail — I am mostly curious whether you have concerns about the overall approach.
Also, I bet that @tedbow's #3509026: JavaScript code components slots' examples and description should be optional (which just landed) conflicts with this. About to merge in…
Comment #10
balintbrewsI also tested this, and the problem it caused is gone. I'll leave it up to you if this is ready to be merged.
Comment #11
tedbowa couple small things
Comment #12
wim leersAll addressed 👍 Some nice catches, @tedbow — thanks!
@balintbrews Thanks for explicitly testing this indeed fixes the consequences in the UI! 🙏👍
Comment #14
wim leers