Problem/Motivation

On a custom composite element created from a module you can add radios elements.


class MyCompositeElementWebform extends WebformCompositeBase {

 ...

  public static function getCompositeElements(array $element) {
    $elements = [];
    $elements['Color'] = [
      '#type' => 'radios',
      '#title' => t('Color'),
      '#options' => ['blue' => 'Blue', 'red' => 'Red'],
    ];

    return $elements;
  }

 ...
}

You overwrite the option from the YAML source, but if a site builder want to edit or add those options, it's not possible from the UI:

screenshot

Proposed resolution

Provide an UI. A safe start could be to only allow overwriting the text of the existing option, like with the title of the element (so not adding extra option neither changing the key)?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gagarine created an issue. See original summary.

jrockowitz’s picture

Version: 6.2.x-dev » 6.1.x-dev
Status: Active » Needs review

We can also address this via 6.1.x

jrockowitz’s picture

diegors’s picture

Assigned: Unassigned » diegors

I will review this.

Status: Needs review » Needs work

The last submitted patch, 3: 3295420-2.patch, failed testing. View results

diegors’s picture

Assigned: diegors » Unassigned

I tried to reproduce the bug, but as I don't know much about the module I wasn't able to create a custom composite element to test.

gagarine’s picture

@diegors you need to create a custom module. Their is an example on https://git.drupalcode.org/project/webform/-/tree/6.2.x/modules/webform_... . Sadly I don't have the time to test this right now.

jrockowitz’s picture

Status: Needs work » Needs review
FileSize
5.84 KB

You can the webform_example_composite.module for testing

Status: Needs review » Needs work

The last submitted patch, 8: 3295420-8.patch, failed testing. View results

lucasbaralm’s picture

Assigned: Unassigned » lucasbaralm

I will review this.

lucasbaralm’s picture

Assigned: lucasbaralm » Unassigned

Good Morning!
I reviewed the patch #8.

Steps taken:
1.Modified webform_example_composite.module to add the radio button elements.
2. Activate the Webform UI plugin to allow modification of the webform from the UI.
4. Applied the patch that creates the UI for modifying radio buttons.
5. Tested the UI, verifying that it saves the modified radio values and that it saves the new submissions entries with the new values correctly.

The created UI is working perfectly, but the two errors in the tests should be fixed before moving to RTBC as the branch passes tests without the patch.

lucasbaralm’s picture

Assigned: Unassigned » lucasbaralm

I will work on this.

lucasbaralm’s picture

Assigned: lucasbaralm » Unassigned
Status: Needs work » Needs review
FileSize
6.31 KB
335 bytes

I fixed the issue that was causing a test to fail with the error "Warning: Array to string conversion" by changing a conversion from (string) $value to print_r($value, true), please review.

gquisini’s picture

Assigned: Unassigned » gquisini

I'll be reviewing this one.

gquisini’s picture

Assigned: gquisini » Unassigned
Status: Needs review » Reviewed & tested by the community
FileSize
18.64 KB
35.91 KB

So,
following steps #11 and using patch #13, everything worked successfully. I was able to overlay the radio options.
Webform

I also ran the functional tests on the /composite folder. I just had to install the address module to make everything work.

jrockowitz’s picture

Status: Reviewed & tested by the community » Needs work

The patch has some code styling issues that need to be addressed

+ elseif (print_r($value,true) === print_r($option_value,true)) {

+ elseif (print_r($value, TRUE) === print_r($option_value, TRUE)) {

I am also skeptical of using print_r() to compare values.

The original (string) $value === (string) $option_value is meant to allow Markup and strings to be accurately and identically compared.

We might be able to use (and note in comments) a less strict equality $value == $option_value

@see https://www.php.net/manual/en/language.operators.comparison.php

gquisini’s picture

Assigned: Unassigned » gquisini

I'll work on @jrockowitz suggestions.

gquisini’s picture

Assigned: gquisini » Unassigned
Status: Needs work » Needs review
FileSize
5.31 KB
719 bytes

So...

WebformOptionsHelper::hasOption():
I tested equal and identical operator in a sandbox environment and did some debugging as well.
If we use the identical operator as it is in code, where it does the casting type and then compares the values or use the equal operator without the casting, it doesn't have much difference, so I kept the original changes.

WebformTokenManager::validateElement():
Same as above, I tested !is_string() and !mb_strlen() in a sandbox environment and did some debugging.
The !is_string() only returns true if the variable is not of type string and the !mb_strlen() returns true if the length is zero. Since the variable is being taken from the element value or the element's default value. The type will always be string, so we just have to check if it is empty. So, again, I kept the original changes.

Status: Needs review » Needs work

The last submitted patch, 18: 3295420-18.patch, failed testing. View results

lucasbaralm’s picture

The only problem is the test output, it fails and set a warning to "Warning: Array to string conversion" without the changes that i did. Maybe we should try another workaround?

jrockowitz’s picture

Status: Needs work » Postponed (maintainer needs more info)

I am inclined not to implement this feature and let it be handled via custom code or dedicated contribution.

I suspect this change will cause regressions.

Marking postponed to give people a chance to weigh in.

jrockowitz’s picture

Version: 6.1.x-dev » 6.2.x-dev