Radios or Checkboxes inside Composite not saved

I am very hesitant to start supporting this feature because of the risk of regressions. But, at the same time, I understand why having support for multi-select and checkboxes within a composite is important.

Instead of pipe-delimited values, my initial thought is that we need to serialize the values using JSON, YAML, or PHP. I am partial to using YAML, but I open to suggestions.

Also, this improvement is going to require a decent amount of test coverage.

Thank you for creating the first patch.

The patch in comment #2 applies without problems and work good, it remains to assess the feasibility of the implementation.

The attached patch uses serialization and tweaks \Drupal\webform\Plugin\WebformElement\WebformCompositeBase::formatComposite to support multiple values.

This is a major improvement that is changing how some data is stored, My thoughts are we should implement it using the least amount of code with a lot of test coverage.

Also, I would like to implement this feature in 8.x-5.x and then merge into 6.x

I'll write some tests for it.

@paulocs Please hold off writing the automated tests. We need to create more test webform examples.

For example, we need to manually test adding checkboxes to a composite in code,

Okay. I already wrote some test to the webform created in #13. Nothing big

For example, we need to manually test adding checkboxes to a composite in code

Do you mean for example via hook?

Maybe add checkboxes to the webform_example_composite.module

Please review...
I added a checkboxes field via hook to the composite element.

Please let me moving this ticket forward and I will ping you as it needs review.

So I added checkboxes to example composite and fixed a few related bugs.

I think for an MVP we should just support checkboxes and multi-select. Support for other multiple value elements (manually table selects) can be added later (or never)

Fixing minor typo. This patch needs to be reviewed by someone else.

The patch in the comment #25 save the checkboxes correctly, not affecting others functionalities.

@marcusvsouza Thank you for reviewing the patch. This is a major change and probably requires more review and consideration.

I tried to fix the display of the values at a higher level. Can you create an example webform that can be used to reproduce what you are seeing via email.

Changing the priority to Major since the feature request is going to require a lot of review and testing.

This is working very well for us. Composite element with a custom checkboxes element.

I created a second issue to handle re-rolls for 6.x: #3240605: Radios or Checkboxes inside Composite not saved (6.x-dev clone)

I suggest you consider using JSON format for serialization, as DB engines are capable of handling JSON; doing this should get submissions better prepared to perform query/manipulation directly with DB primitives/plug-ins, which should be way more performant than grep'ping + PHP-land unserialization. For reference, see:

• MySQL support: 12.18.3 Functions That Search JSON Values
• PostgreSQL support: 9.15. JSON Functions and Operators
• SQLite support: The JSON1 Extension

@jedihe That was exactly the type of suggestion I was looking for via #4. Thanks.

JSON it is.

Thanks, @michiellucas and @jrockowitz for great work!

Couple of things I'd like to suggest:

1. Hardcoding webform element types which are supported in composite element in Plugin\WebformElement\WebformCompositeBase::isSupportedElementType() and in Element\WebformElementComposite::validateWebformElementComposite() methods make it really hard to make it work with custom webform element types which directly extend the allowed hardcoded type classes, e.g., Checkboxes or Radios.

In a project I'm working on I have several custom webform element types which extend said classes (e.g., class DecoupledCheckboxes extend Checkboxes). They are not going to work with current implementation, even if they are technically completely compatible.

2. The logic behind checking if a particular element type is supported by a composite element type is scattered across many related classes like Plugin\WebformElement\WebformCompositeBase and Element\WebformElementComposite. It becomes very hard to override methods like Plugin\WebformElement\WebformCompositeBase::isSupportedElementType() because it requires at least two class overrides which is cumbersome and hardly maintainable.

Given that Plugin\WebformElement\WebformCompositeBase::isSupportedElementType() and Element\WebformElementComposite::validateWebformElementComposite() methods are designed to work within the webform_custom_composite webform element plugin (or the extended classes of this class), I think it's best if is-this-type-allowed checks are performed by the composite webform element plugin itself. This will allow for easier customizations in the extended classes of the custom webform types, and will also keep the logic contained in one place.

3. Currently Plugin\WebformElement\WebformCompositeBase::isSupportedElementType() method returns FALSE for element plugins which are instance of WebformElementEntityReferenceInterface.

After doing some testing, I can report that term reference option webform element works fine within the composite element and multiple reference term IDs are nicely saved as multiple values in the database.

It's fine to disable term reference elements by default, if maintainers have concerns that this may cause regressions. But, going back to first two points I raised, currently it's a huge headache to override the logic in isSupportedElementType() method in cases where there's no such concern.

Also, I really hope that patches in this issue will end up in 6.x branch as well.

Yes, I am very concerned about unexpected issues with composite sub-elements which is why WebformCompositeBase::isSupportedElementType acts as a whitelist. In a separate ticket, we could rework the code to allow a webform element to determine if it can or can't be used as a composite sub-element.

Re-rolling 3216923-36.patch for 6.x branch.

I will probably not commit this feature until 6.2.x

Patch at #41 does not currently work with 6.1.1, 6.1.2 or current 6.x-dev. It does work with 6.1.0

I updated the patch to work with Webform 6.2.x. I am very hesitant to merge this because I think it may create more issues that it is worth.

Please don't drop this :)

We are successfully using it in production (with 6.1.2). Our composite elements have textfield, textarea, and checkboxes. What sort of issues to you foresee happening by merging this into a release? Anything specific that I could test for?

Very minor re-rolll for 6.1.2 (currently recommended version).

Did a minor re-roll for 6.1.3 in case anyone needs it for the latest 6.1.x version :)

Tested @JonMcL's patch file on Drupal 9 and 6.1.2. Seems to be working fine with fields saving.

Ok, I am shooting in the dark here. I added #36 patch trying to fix the error below. It happens only when I click on one of the composite checkboxes and submit the webform. Just wanted to see if anyone had run into this. Trying to figure what is causing it.

I am using Drupal 9.3.12 on php 8.1.5 and Maria db 10.6.7-MariaDB-2ubuntu1.

Thanks!

TypeError: Illegal offset type in Drupal\Core\Render\Element\Checkboxes::valueCallback() (line 136 of /var/www/sdgs.localhost/web/core/lib/Drupal/Core/Render/Element/Checkboxes.php)
#0 [internal function]: Drupal\Core\Render\Element\Checkboxes::valueCallback()
#1 /var/www/sdgs.localhost/web/core/lib/Drupal/Core/Form/FormBuilder.php(1258): call_user_func_array()
#2 /var/www/sdgs.localhost/web/core/lib/Drupal/Core/Form/FormBuilder.php(1000): Drupal\Core\Form\FormBuilder->handleInputElement()
#3 /var/www/sdgs.localhost/web/core/lib/Drupal/Core/Form/FormBuilder.php(1070): Drupal\Core\Form\FormBuilder->doBuildForm()
#4 /var/www/sdgs.localhost/web/core/lib/Drupal/Core/Form/FormBuilder.php(1070): Drupal\Core\Form\FormBuilder->doBuildForm()
#5 /var/www/sdgs.localhost/web/core/lib/Drupal/Core/Form/FormBuilder.php(1070): Drupal\Core\Form\FormBuilder->doBuildForm()
#6 /var/www/sdgs.localhost/web/core/lib/Drupal/Core/Form/FormBuilder.php(1070): Drupal\Core\Form\FormBuilder->doBuildForm()
#7 /var/www/sdgs.localhost/web/core/lib/Drupal/Core/Form/FormBuilder.php(1070): Drupal\Core\Form\FormBuilder->doBuildForm()
#8 /var/www/sdgs.localhost/web/core/lib/Drupal/Core/Form/FormBuilder.php(1070): Drupal\Core\Form\FormBuilder->doBuildForm()
#9 /var/www/sdgs.localhost/web/core/lib/Drupal/Core/Form/FormBuilder.php(1070): Drupal\Core\Form\FormBuilder->doBuildForm()
#10 /var/www/sdgs.localhost/web/core/lib/Drupal/Core/Form/FormBuilder.php(1070): Drupal\Core\Form\FormBuilder->doBuildForm()
#11 /var/www/sdgs.localhost/web/core/lib/Drupal/Core/Form/FormBuilder.php(574): Drupal\Core\Form\FormBuilder->doBuildForm()
#12 /var/www/sdgs.localhost/web/core/lib/Drupal/Core/Form/FormBuilder.php(320): Drupal\Core\Form\FormBuilder->processForm()
#13 /var/www/sdgs.localhost/web/core/lib/Drupal/Core/Entity/EntityFormBuilder.php(48): Drupal\Core\Form\FormBuilder->buildForm()
#14 /var/www/sdgs.localhost/web/modules/contrib/webform/src/Entity/Webform.php(1243): Drupal\Core\Entity\EntityFormBuilder->getForm()
#15 /var/www/sdgs.localhost/web/modules/contrib/webform/src/Controller/WebformEntityController.php(76): Drupal\webform\Entity\Webform->getSubmissionForm()
#16 [internal function]: Drupal\webform\Controller\WebformEntityController->addForm()
#17 /var/www/sdgs.localhost/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(123): call_user_func_array()
#18 /var/www/sdgs.localhost/web/core/lib/Drupal/Core/Render/Renderer.php(564): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
#19 /var/www/sdgs.localhost/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(124): Drupal\Core\Render\Renderer->executeInRenderContext()
#20 /var/www/sdgs.localhost/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(97): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext()
#21 /var/www/sdgs.localhost/vendor/symfony/http-kernel/HttpKernel.php(158): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
#22 /var/www/sdgs.localhost/vendor/symfony/http-kernel/HttpKernel.php(80): Symfony\Component\HttpKernel\HttpKernel->handleRaw()
#23 /var/www/sdgs.localhost/web/modules/contrib/redirect_after_login/src/RedirectMiddleware.php(46): Symfony\Component\HttpKernel\HttpKernel->handle()
#24 /var/www/sdgs.localhost/web/core/lib/Drupal/Core/StackMiddleware/Session.php(58): Drupal\redirect_after_login\RedirectMiddleware->handle()
#25 /var/www/sdgs.localhost/web/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(48): Drupal\Core\StackMiddleware\Session->handle()
#26 /var/www/sdgs.localhost/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(106): Drupal\Core\StackMiddleware\KernelPreHandle->handle()
#27 /var/www/sdgs.localhost/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(85): Drupal\page_cache\StackMiddleware\PageCache->pass()
#28 /var/www/sdgs.localhost/vendor/asm89/stack-cors/src/Asm89/Stack/Cors.php(49): Drupal\page_cache\StackMiddleware\PageCache->handle()
#29 /var/www/sdgs.localhost/web/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(48): Asm89\Stack\Cors->handle()
#30 /var/www/sdgs.localhost/web/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(51): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle()
#31 /var/www/sdgs.localhost/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle()
#32 /var/www/sdgs.localhost/web/core/lib/Drupal/Core/DrupalKernel.php(708): Stack\StackedHttpKernel->handle()
#33 /var/www/sdgs.localhost/web/index.php(19): Drupal\Core\DrupalKernel->handle()
#34 {main}
.

I am also facing the TypeError: Illegal offset type in Drupal\Core\Render\Element\Checkboxes::valueCallback() error that @elneto reports, with version 6.1.3 of the module and patch #48. The details of the environment are: Drupal 9.3.12 and php 8.1.5. This was previously working (tested with a previous version of php - not sure which 8.1.x- and with Drupal 9.3.7 - I haven't used it with newer Drupal core versions until today), however I an not sure what broke it (Drupal core update or possibly the PHP update).

#48 working for me, apart from when I add the field in a view (webform_views installed). Array is outputted rather then the actual submitted data.

@danthorne which Drupal core version are you using?

I am currently using Drupal 9.4.4, Webform 6.1.3 and the error still appears with patch #48.

For anyone interested for a quick workaround (unfortunately I didn't have the time to search for an appropriate solution), the problem occurs since in line 136 of core/lib/Drupal/Core/Render/Element/Checkboxes.php:

if (!empty($element[$key]['#disabled'])) {

$key is actually an array.

Replacing (using a local patch) that line with the one below seems to solve the problem (at least for my use case):

if (!is_array($key) && !empty($element[$key]['#disabled'])) {

Currently using Drupal 9.4.3 and Webform 6.1.3

I'm on D9.5 and php 8.1

Tried the patch #48 with webforms 6.1.4 and tried the patch from the MR (https://git.drupalcode.org/project/webform/-/merge_requests/123.patch) with version 6.2.0-beta5 (latest release) and the dev version, but all the 3 gave me the same error as #50 on form submission:

TypeError: Illegal offset type in Drupal\Core\Render\Element\Checkboxes::valueCallback() (line 136 of core/lib/Drupal/Core/Render/Element/Checkboxes.php).

So I'm moving this to needs work.

For me the parth #48 works fine right after applying it, using Drupal 10.0.8, Webforms 6.2.0-beta5 and PHP 8.1. This feature was very much needed and as it's just saving the data as JSON string, I don't think adding this to the actual module would be much of an issue. However if that is not desired, maybe this could be moved to a separate module while it's not considered stable?

And for @bbombachini, I had no issue using checkboxes and submitting form, no errors there.

Interesting @phonkala, I'm still testing this on D9.5.3, webform 6.2.0-beta5, PHP 8.1, and the patch from #48. And I still have the same error:

TypeError: Illegal offset type in Drupal\Core\Render\Element\Checkboxes::valueCallback() (line 136 of core/lib/Drupal/Core/Render/Element/Checkboxes.php).

So I created a simple webform for testing, can you try using this form and tell me what it gives you for $element[$key] on line 36 of core/lib/Drupal/Core/Render/Element/Checkboxes.php?

I have the same issue as outlined on comment #53 where $key is an array.

uuid: 04b7199e-cd72-46ba-954d-d4e196f58f43
langcode: en
status: open
dependencies: {  }
weight: 0
open: null
close: null
uid: 85
template: false
archive: false
id: test
title: Test
description: ''
category: ''
elements: |-
  custom_composite_test:
    '#type': webform_custom_composite
    '#title': 'Custom composite test'
    '#element':
      select_options:
        '#type': webform_checkboxes_other
        '#options':
          test1: 'Test 1'
          test2: 'Test 2'
          test3: 'Test 3'
          test4: 'Test 4'
        '#title': 'Select options'
css: ''
javascript: ''
settings:
  ajax: false
  ajax_scroll_top: form
  ajax_progress_type: ''
  ajax_effect: ''
  ajax_speed: null
  page: true
  page_submit_path: ''
  page_confirm_path: ''
  page_theme_name: ''
  form_title: both
  form_submit_once: false
  form_open_message: ''
  form_close_message: ''
  form_exception_message: ''
  form_previous_submissions: true
  form_confidential: false
  form_confidential_message: ''
  form_disable_remote_addr: false
  form_convert_anonymous: false
  form_prepopulate: false
  form_prepopulate_source_entity: false
  form_prepopulate_source_entity_required: false
  form_prepopulate_source_entity_type: ''
  form_unsaved: false
  form_disable_back: false
  form_submit_back: false
  form_disable_autocomplete: false
  form_novalidate: false
  form_disable_inline_errors: false
  form_required: false
  form_autofocus: false
  form_details_toggle: false
  form_reset: false
  form_access_denied: default
  form_access_denied_title: ''
  form_access_denied_message: ''
  form_access_denied_attributes: {  }
  form_file_limit: ''
  form_attributes: {  }
  form_method: ''
  form_action: ''
  share: false
  share_node: false
  share_theme_name: ''
  share_title: true
  share_page_body_attributes: {  }
  submission_label: ''
  submission_exception_message: ''
  submission_locked_message: ''
  submission_log: false
  submission_excluded_elements: {  }
  submission_exclude_empty: false
  submission_exclude_empty_checkbox: false
  submission_views: {  }
  submission_views_replace: {  }
  submission_user_columns: {  }
  submission_user_duplicate: false
  submission_access_denied: default
  submission_access_denied_title: ''
  submission_access_denied_message: ''
  submission_access_denied_attributes: {  }
  previous_submission_message: ''
  previous_submissions_message: ''
  autofill: false
  autofill_message: ''
  autofill_excluded_elements: {  }
  wizard_progress_bar: true
  wizard_progress_pages: false
  wizard_progress_percentage: false
  wizard_progress_link: false
  wizard_progress_states: false
  wizard_start_label: ''
  wizard_preview_link: false
  wizard_confirmation: true
  wizard_confirmation_label: ''
  wizard_auto_forward: true
  wizard_auto_forward_hide_next_button: false
  wizard_keyboard: true
  wizard_track: ''
  wizard_prev_button_label: ''
  wizard_next_button_label: ''
  wizard_toggle: false
  wizard_toggle_show_label: ''
  wizard_toggle_hide_label: ''
  wizard_page_type: container
  wizard_page_title_tag: h2
  preview: 0
  preview_label: ''
  preview_title: ''
  preview_message: ''
  preview_attributes: {  }
  preview_excluded_elements: {  }
  preview_exclude_empty: true
  preview_exclude_empty_checkbox: false
  draft: none
  draft_multiple: false
  draft_auto_save: false
  draft_saved_message: ''
  draft_loaded_message: ''
  draft_pending_single_message: ''
  draft_pending_multiple_message: ''
  confirmation_type: page
  confirmation_url: ''
  confirmation_title: ''
  confirmation_message: ''
  confirmation_attributes: {  }
  confirmation_back: true
  confirmation_back_label: ''
  confirmation_back_attributes: {  }
  confirmation_exclude_query: false
  confirmation_exclude_token: false
  confirmation_update: false
  limit_total: null
  limit_total_interval: null
  limit_total_message: ''
  limit_total_unique: false
  limit_user: null
  limit_user_interval: null
  limit_user_message: ''
  limit_user_unique: false
  entity_limit_total: null
  entity_limit_total_interval: null
  entity_limit_user: null
  entity_limit_user_interval: null
  purge: none
  purge_days: null
  results_disabled: false
  results_disabled_ignore: false
  results_customize: false
  token_view: false
  token_update: false
  token_delete: false
  serial_disabled: false
access:
  create:
    roles:
      - anonymous
      - authenticated
    users: {  }
    permissions: {  }
  view_any:
    roles: {  }
    users: {  }
    permissions: {  }
  update_any:
    roles: {  }
    users: {  }
    permissions: {  }
  delete_any:
    roles: {  }
    users: {  }
    permissions: {  }
  purge_any:
    roles: {  }
    users: {  }
    permissions: {  }
  view_own:
    roles: {  }
    users: {  }
    permissions: {  }
  update_own:
    roles: {  }
    users: {  }
    permissions: {  }
  delete_own:
    roles: {  }
    users: {  }
    permissions: {  }
  administer:
    roles: {  }
    users: {  }
    permissions: {  }
  test:
    roles: {  }
    users: {  }
    permissions: {  }
  configuration:
    roles: {  }
    users: {  }
    permissions: {  }
handlers: {  }
variants: {  }

Hi I just rebased.

@bbombachini I was getting no output of that yaml (well, there was submit button) so checked into it a bit and seems the element types are named differently than mine. Modified the source "elements" section a bit, with this I got composite with checkboxes:

elements:
  custom_composite_test:
    '#type': custom_composite
    '#title': 'Custom composite test'
    '#element':
      select_options:
        '#type': checkboxes
        '#options':
          test1: 'Test 1'
          test2: 'Test 2'
          test3: 'Test 3'
          test4: 'Test 4'
        '#title': 'Select options'

And submitting gives no error.

Basically I just checked the element types from my other forms and replaced the ones you had with them. And also removed the "|-" part from the "elements" row.

@bbombachini Ah now I see it, using "webform_checkboxes_other" as the element type sure does give the array key error! Weirdly, basic checkboxes work nicely and for example even "webform_select_other" works just fine.

EDIT: Tested submitting the form with "radios or other", "checkboxes or other" and "select with other" input types and only the "checkboxes with other" has the issue. Even "Select with other" with multiple enabled works great and does save submissions correctly.

The patch in #48 was no longer applying to 6.2.x (Drupal 10 compatible) so I fixed the merge conflict in the fork

Fixed "webform_checkboxes_other" issue and the failing tests.

Rolled patch with the latest updates from merge request

Please rebase the merge request and get tests to pass. Which merge request is preferred?

I note that in addition to the two merge requests, there is also branch 3216923-composite-multiple.

How would one go about rebasing this for 6.3? besides the rebase, what else is needed to get this ready for beta?

You could either cherry-pick all the needed commits or use git rebase --onto. Then force-push the branch and update the merge request to target 6.3.x.

I don't want to implement this feature because it would require too much work to support it.

For example, the webform views module would most likely need to be updated and tokens will probably need to fixed.