Overview

If you input any numeric value into field in the contextual settings panel, when it tries to update the component display, it will cause a property validation failure because it interprets the value as an integer instead of a string:

Drupal\Core\Render\Component\Exception\InvalidComponentException: [cta1] Integer value found, but a string or an object is required in Drupal\Core\Theme\Component\ComponentValidator->validateProps() (line 203 of core/lib/Drupal/Core/Theme/Component/ComponentValidator.php).

Any changes to the form will be lost.

Screen recording

Proposed resolution

User interface changes

CommentFileSizeAuthor
#15 select.gif181.18 KBhooroomoo
numeric-field-input.gif388.78 KBtraviscarden
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:

Comments

traviscarden created an issue. See original summary.

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

bnjmnm’s picture

There was a utility function that would send everything through Number() that didn't result in NaN - so things like spaces or strings that included only numerals would get changed to numbers.

The fix is the same for this and #3473704: Space press in empty component props field inserts `0` so I just added a test that covers what the other issue reported.

bnjmnm’s picture

Status: Active » Needs review
traviscarden’s picture

Status: Needs review » Needs work

Thanks, @bnjmnm. That fixes the bug in question in my local testing. Moving to "Needs work" though, since the tests are failing.

shyam_bhatt’s picture

Assigned: Unassigned » shyam_bhatt
shyam_bhatt’s picture

Assigned: shyam_bhatt » Unassigned
bnjmnm’s picture

Yikes, the random test failures in components-slots.cy.js are back with a vengance despite the changes in this issue having no impact on it whatsoever. I'll likely try a few things in this MR since it seems like the starts aligned to make the random fails happen pretty reliably here

bnjmnm’s picture

Status: Needs work » Needs review

Lets keep the components-slots.cy.js fixes in there if we can even if it is technically out of scope - it'll make life easier for everyone.

kristen pol’s picture

traviscarden’s picture

I assume this is related: #3472176: String props that are integer values aren't treated as strings

Yes. That issue has been marked as a duplicate of this one.

I've manually tested again and confirmed that, of course, it still works. A quick review of the JS code (not my area of expertise) should wrap this up.

wim leers’s picture

Assigned: Unassigned » hooroomoo
Status: Needs review » Reviewed & tested by the community

Needs @hooroomoo's (or @effulgentsia's) approval on the MR before this can be merged, but LGTM!

(Just one nit/suggestion, not commit-blocking.)

hooroomoo’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new181.18 KB

Found a regression where it breaks the select element. Could be worth adding a test for that too.

hooroomoo’s picture

Assigned: hooroomoo » Unassigned
bnjmnm’s picture

It looks like this specifically breaks select elements with type: integer and wasn't caught e2e because the only select elements with tests right now are string based. This issue makes sense as the cast-to-number is currently specific to numeric inputs. This logic will need to be expanded to check data type, and we should add an integer select to the sdc_test_all_props component.

wim leers’s picture

Version: » 0.x-dev
Issue tags: +Novice, +Barcelona2024, +Needs tests

we should add an integer select to the sdc_test_all_props component.

At least this part is Novice and is a great contribution for DrupalCon Barcelona!

Dear Barcelona code sprint participant: see /tests/modules/sdc_test_all_props/components/all-props/all-props.component.yml. See

    test_string_enum:
      title: 'String - Enum'
      type: string
      enum:
        - foo
        - bar
      examples:
        - foo

What @bnjmnm identified is that we should have a similar SDC prop for integers: type: integer and enum: ….

wim leers’s picture

Related: over at #3467870: Support `{type: array, …}` prop shapes, I've had to add:

diff --git a/themes/engines/semi_coupled/semi_coupled.engine b/themes/engines/semi_coupled/semi_coupled.engine
index 7890e549..cfe2abb6 100644
--- a/themes/engines/semi_coupled/semi_coupled.engine
+++ b/themes/engines/semi_coupled/semi_coupled.engine
@@ -276,7 +276,7 @@ function _semi_coupled_collect_props(array &$props, array &$slots, array $prop_t
  * @return array|string|object|null
  *   Essentially $variables[~$index_in] or $variables->{~$index_in}.
  */
-function _semi_coupled_get_variable(array|object $variables, string $index_in_js_format): array|string|object|bool|null {
+function _semi_coupled_get_variable(array|object $variables, string $index_in_js_format): array|string|object|bool|null|int {
   // The lookup information we have is camelCased, but the values are likely
   // in snake case, so we create a snake case formatted index.
   $index_in_php_format = _semi_coupled_snake_case($index_in_js_format);

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

jessebaker’s picture

In an attempt to push this issue along, I went ahead and added the test described by @bnjmnm in #17. Unfortunately this appears to have highlighted a further problem. Or perhaps this is actually the symptom of the problem @bnjmnm is referring to in that comment.

When props that are NOT the new Integer Enum (select) field are updated, the value of the Integer select field is NOT sent to the back end.

IOW the initial value of the select does not appear to be correctly updated in the redux store when loading the form for the first time.

bnjmnm’s picture

Status: Needs work » Closed (duplicate)

This is addressed much more easily with the schemas fully available which is happening in #3474732: Premature prop validation can break the UI, so I addressed the reported issue there + included tests to verify it is fixed. Get that one reviewed/codeowner signed and the symptoms reported here are fixed in a less hacky way than what can currently be done off 0.x