Selected yet disabled individual options from checkboxes element don't persist through save

This is major at least and a bug and should hopefully be fixed for 8.4. IMO. Data could be potentially lost (which may make it critical?) if the disabled form fields have been submitted with checked boxes values will be lost due to this bug.

Thank you very much @Maouna for finding this issue.

This has tests but the solution may not be right yet. Thanks for writing the tests to show this is reproducible:) I'm adding those steps to the issue summary.

Maybe someone at the sprint can confirm the steps added are correct?

So I tested this, I can't reproduce this issue.

Steps i went through:

• Added checkbox fields to a node
• Downloaded "disable field" module
• created a node
• disabled the field via this module
• edit and save node

also, I tried disabling it via the form-display, but same results.

In your test script you disable a individual checkbox inside a list of checkboxes, Is there a way we can easily reproduce this case via the UI?

Thanks for the steps I replaced my earlier steps with your updated steps to reproduce in the summary.

I was unable to reproduce the bug with the steps provided, but with a little more guidance i was able to find the problem. I've updated the steps to reproduce so hopefully it will be easier for the next person to find the problem as well.

I mentored Maouna on debugging / reproducing the problem and improving the code comments.
@Maouna you are doing great. I'm glad you are contributing to Drupal core :)

Enabling the testbot with "needs review" status

Ran into this bug with revision statuses of content types in #2972361: Node type's isNewRevision can be wrong and unchangeable when using content moderation

This is still an issue and IMO a critical one because of potential data loss.

In our case we had a classification field that should persist on all translations and because different users could add translations we wanted to make sure only the original author could do the classification change on the original language. The result was the new translations caused all classifications being lost accross all translations.

@Maouna patch didn't work for me unfortunately. Here is my take on it which made my field save correctly. It is not inline with the proposed resolution in the IS tho.

Please review

+++ b/core/lib/Drupal/Core/Render/Element/Checkboxes.php
@@ -94,7 +94,7 @@ public static function processCheckboxes(&$element, FormStateInterface $form_sta
+    if ($input === FALSE || ($input === null && isset($element['#attributes']['disabled']))){

- null should be NULL
- the way to disable an element is just $element['disabled'], not with #attributes
- should be !empty(), not isset(). isset() only checks for NULL. isset(FALSE) returns TRUE!

Working on a new patch...

Actually, patches #33 and #34 don't work for me at all.

Debugging shows that valueCallback() is called for the checkboxes element overall, not for the individual checkbox sub-elements.

Patch #26 looks like it has the right approach, but that's not working for me either...

Ok, so patch #26 does work in general, but my problem was that User module slightly misbehaves in the AccountForm:

    $form['account']['roles'] = [
      '#type' => 'checkboxes',
      '#title' => $this->t('Roles'),
      '#default_value' => (!$register ? $account->getRoles() : []),
      '#options' => $roles,
      '#access' => $roles && $user->hasPermission('administer permissions'),
    ];

The correct way, AFAIK, to set a default value for checkboxes is an array whose keys and values match, e.g.:

[
'foo' => 'foo',
'bar' => 'bar',

However... the Checkboxes form element class doesn't document this, so we can't treat my recollection as being correct. Therefore, the isset() check for the default value should be an in_array() instead.

Here's an updated version of patch #26. Credit should still be to @Maouna, my change is a 1-line tweak.

Argh, needed a reroll for 8.6.x for changes in the test class.

Same interdiff as before.

(Note to users of Commerce License linked here from the README: use the patch in #37 above for Drupal core 8.5 and earlier.)

@Joachim #32 probably didn't work for you because i was basing my disabled value on the attribute, which was incorect as you have stated.
But if instead you do

if ($input === FALSE || ($input === NULL && !empty($element['#disabled']))) {

It should work.

And this is probably the reason why @Maouna's patch did not work for me. She was basing it on the correct way of disabling the element.
I think this opens up a new problem here. We have more than one way of disabling form elements that work but some might cause this data loss.

> The correct way, AFAIK, to set a default value for checkboxes is an array whose keys and values match, e.g.:

Just an array of values is definitely valid and pretty common.

> if ($input === FALSE || ($input === NULL && !empty($element['#disabled']))) {

I tried that too. But $element is the whole of the checkboxes element, not each single checkbox. So that doesn't work.

> I think this opens up a new problem here. We have more than one way of disabling form elements that work but some might cause this data loss.

Using [#attributes][disabled] is incorrect, AFAIK. Or at least, it's only cosmetic.

> Just an array of values is definitely valid and pretty common.

Thanks! That confirms my change from #26 to #39 as correct.

This is used, for example, on the module overview page, where it is possible to check more modules, but impossible to uncheck already enabled modules:

Modules are not suppose to be disabled (and thereby, uninstalled) by core that way. the fact that the checkbox is disabled is simply to show that the module.. is active. the 'problem' on /admin/modules is it lack help test to clarify that a sitebuilder wouldn't be disabling a module on that tab but should be on the 'uninstall' tab (tab is poorly named. people don't get they are actually doing both then.)

-------
re:#31 -- sounding like your field translation and field access process in that project may need to be addressed differently.

This seems major to me. Can a core team member review?

Hi,
I tried on drupal-8.8.x-dev. The patch applied without any problem but give me no modification.
I follow the step for the case 2 but as my screenshot display the problem still occur:

• Add a field to article
• Configure the Manage form display
• Add this line of code
• Enable the book module
• Add an article
• screenshot of my field in the new content display
  
• Submit the node form
• Edit the new article node
• screenshot of my field in the edition content display
  

<!DOCTYPE html>
<html>
<head>
	<title></title>
</head>
<body>
<?php
var_dump($_GET);
?><form action="/check.php" method="GET">
  <input type="checkbox" name="vehicle" value="Bike"> I have a bike<br>
  <input type="checkbox" name="vehicle" value="Car"> I have a car<br>
  <input type="checkbox" name="vehicle" value="Boat" checked='checked' disabled='disabled'> I have a boat<br>
  <input type="submit" value="Submit">
</form>
</body>
</html>

a basic test. disabled checkboxes NO NOT HOLD A VALUE when submitted as the basic behaviour of html forms.

---
Using disabled as a UX approach to show previous selected items as 'selected, but now now un-selectible' in a group of checkboxes is not a workable idea due to constraints of Forms themselves.

possibly: you can use JS(ie: if you have disabled one of the checkboxes via a #state operation) and another hidden input to 'keep watch' of those items and pass that back through and process that within a _validate or #after_build. This is essentially a custom component/widget.

IMO: core shouldn't handle this kind of use case. it's a customization of the natural element.

Again, the module page uses individual checkbox items, not grouped to show 'selected/but frozen' in it's own process -- it is a highly customized core form in that way.

My opinion was sought on this issue (no wonder). Somewhat reluctantly (I am not the form API maintainer now! not any more) here it comes.

First of all, I do not quite understand what's critical here.

Second, I am surprised. I was not aware Drupal supports disabled individual checkbox elements under checkboxes. When was this added, where is it documented and where is it tested? Of course, any form API work I remember is now many years old but searching for the string "disabled" in the Checkboxes class finds nothing, as far as I can see Drupal\Tests\system\Functional\Form\FormTest doesn't test such, nor does ElementTest. In general, I feel #type checkboxes is a convenience, if your use case goes beyond the capabiities of it then just use individual #checkbox elements.

Re #51. Bringing up documentation is a good point. On checkboxes being poorly documented - check this issue and its linked issue: https://www.drupal.org/project/drupal/issues/2779999

Re #50 As for drupal "#disabled", it's a bit of a mystery for me, but I don't think it's relying only on standard form behavior.

If you want more standard and clear thing use "readonly" attribute. Related:
https://stackoverflow.com/questions/7730695/whats-the-difference-between...

Thanks for the pointers, now I have answers: this was added in #915936: Make it easier to define checkboxes/radios with customized sub-elements and it is tested in FormTestCheckboxesRadiosForm: while #disabled is explicitly not tested, that form tests the sub element customization. This is a big surprise for me: I never knew about this and this is old (it was in D7 already). Oh well. I still think this is neither critical nor a novice issue but what do I know, really.

#52 cheers.

Readonly would be the correct attribute for the situation. Regardless if using a group or individual checboxed/radios.

This problem exists in D7 too, so unless there is another issue for D7 specifically, tagging this for backporting.

Tagging for badcamp2019, thanks! (October 2-5)

I am removing the Novice tag from this issue because it's not clear to me what the next step is. There does not seem to be consensus on the approach. I removed the Needs manual testing tag because the patch was tested and screenshots were added. Thank you for adding screenshots, @fabienly.

The issue summary has a lot of documentation and hard work done several years ago, but it probably needs to be updated. I added the Needs issue summary update tag. I hesitate to add this now because the issue isn't clear but also the issue needs Accessibility and probably Subsystem maintainer review.

The issue still seems to be Critical based on user report of data loss caused by lost form input.

I'm facing this issue too.

I have a group type where non-admin can enable features. However, the Portal feature requires other configurations and it's available only for administrator roles.

The above is achieved with a form_alter + after_build method:

$form['field_application_features']['widget']['#after_build'][] = 'mymodule_field_application_features_after_build';

function mymodule_field_application_features_after_build(array $element, FormStateInterface $form_state) {
  // Disable portal feature for non-administrator users.
  $user_roles = \Drupal::currentUser()->getRoles();
  if (!in_array('administrator', $user_roles)) {
//    $element['portal']['#disabled'] = TRUE;
    $element['portal']['#attributes']['readonly'] = TRUE;
    $element['portal']['#description'] = t('If you want to enable/disable Private Portal feature, please contact the Tech Team.');
  }
  return $element;
}

The bug happens when an admin enables Portal feature and then a non-admin user edits the content. After the non-admin submits the form, Portal feature is disabled. (data loss)

Hi. I note the discussion in https://drupal.stackexchange.com/questions/832/how-can-i-disable-a-single-checkbox-in-a-checkboxes-fapi-element which advised that this can be achieved by the following code:

function mymodule_form_alter(&$form, &$form_state, $form_id) {
  $form['checkboxes_element']['#disabled'] = TRUE; //disables all options
  $form['checkboxes_element'][abc]['#disabled'] = TRUE; //disables option, called abc
}

I have used this and found is is a suitable work-around.

Re-roll of #39

Thanks for working on this. I agree it's a data loss bug, and therefore critical. Even though 'checkboxes' is a bit weird, if core's going to provide it, we need to support it working correctly.

Visual inspection of the patch:

1. +++ b/core/lib/Drupal/Core/Render/Element/Checkboxes.php
   @@ -73,12 +73,19 @@ public static function processCheckboxes(&$element, FormStateInterface $form_sta
   +        if (isset($value[$key]) || (in_array($key, $element['#default_value']) && isset($element[$key]['#disabled']))) {
   

   I believe we need ... && !empty($element[$key]['#disabled']. Otherwise, '#disabled' => FALSE would trigger this case, which we don't want.

2. +++ b/core/lib/Drupal/Core/Render/Element/Checkboxes.php
   @@ -115,6 +122,17 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
   +          if (isset($element[$key]['#disabled'])) {
   

   Same here.

I tried manually testing. Indeed, case #2 from the summary is still not solved by the patch, nor covered by the updated test.

Re-roll

Fixed the below issues..

FILE: /var/www/html/core/lib/Drupal/Core/Render/Element/Checkboxes.php
----------------------------------------------------------------------
FOUND 6 ERRORS AFFECTING 5 LINES
----------------------------------------------------------------------
129 | ERROR | [x] Line indented incorrectly; expected 8 spaces,
| | found 6
130 | ERROR | [x] Line indented incorrectly; expected 10 spaces,
| | found 8
131 | ERROR | [x] Line indented incorrectly; expected 12 spaces,
| | found 10
132 | ERROR | [x] Line indented incorrectly; expected 10 spaces,
| | found 8
133 | ERROR | [x] Line indented incorrectly; expected 8 spaces,
| | found 7
133 | ERROR | [x] Closing brace indented incorrectly; expected 6
| | spaces, found 7

Is there any interest in perhaps spinning off Case 2 from the IS into a separate issue? Here are some hopefully compelling reasons to split it off.

Technical Reason
1. Case #1 is a Form API issue while Case #2 is a Field API issue.

Practical Reasons
2. Case #1 seems to be nearly fixed and tested, while Case #2 has no fix and no tests.
3. Case #1 appears to be the more frequently encountered bug based on this issue's comments, while Case #2 looks like a pretty edgy edge case.

Thoughts?

#66 does not apply to 9.2, but #69 and #71 apply fine.

I am trying to apply this patch against 9.3.5 has anyone gotten #71 to apply? I am needing it for the commerce_license module.

Thanks in advance

Re-rolled #71

#50 and #51 make it clear that core has not explicitly intended to support this use, and that to do so would be a helper feature provided by #checkboxes, the absence of support for this use case is not a bug as HTML itself does not natively support this.

However, allowing #disabled and removing data unexpectedly is definitely a nasty bug. Either we should disallow #disabled, or it should work.

I think it's a useful feature to have this, so let's continue to allow it and make it work.

I agree with #72 that splitting out case 1 and case 2 from the IS is a good idea; smaller issues make for faster progress as a rule of thumb. I have created #3268325: Selected yet disabled individual options from checkboxes widget don't persist through save to handle case 2 and postponed it on this issue.

The fix has been extensively reviewed, and the test coverage looks good to me.

1. +++ b/core/lib/Drupal/Core/Render/Element/Checkboxes.php
   @@ -73,12 +73,19 @@ public static function processCheckboxes(&$element, FormStateInterface $form_sta
   +        if (isset($value[$key]) || (in_array($key, $element['#default_value']) && isset($element[$key]['#disabled']) && !empty($element[$key]['#disabled']))) {
   

   in_array() needs the strict param set.

   isset($element[$key]['#disabled']) && !empty($element[$key]['#disabled'])) is equivalent to !empty($element[$key]['#disabled'])

   !empty() is probably more performant than in_array() so should be done first.

   Therefore this is better written as:
   if (isset($value[$key]) || (!empty($element[$key]['#disabled']) && in_array($key, $element['#default_value'], TRUE))) {

2. +++ b/core/lib/Drupal/Core/Render/Element/Checkboxes.php
   @@ -115,6 +122,17 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
   +          if (isset($element[$key]['#disabled']) && !empty($element[$key]['#disabled'])) {
   

   Can be written as:
   if (!empty($element[$key]['#disabled'])) {

Made changes as per comment #78.

Thanks @ravi.shankar

Committed and pushed 28a3a8779d to 10.0.x and e5590b2254 to 9.4.x and e2443fbb83 to 9.3.x. Thanks!

diff --git a/core/modules/system/tests/src/Functional/Form/FormTest.php b/core/modules/system/tests/src/Functional/Form/FormTest.php
index 9b4c22fb7d..505bc76dbc 100644
--- a/core/modules/system/tests/src/Functional/Form/FormTest.php
+++ b/core/modules/system/tests/src/Functional/Form/FormTest.php
@@ -803,7 +803,7 @@ public function assertFormValuesDefault(array $values, array $form): void {
           $expected_value = $form[$key]['#default_value'];
         }
 
-        if (in_array($key, ['checkboxes_multiple', 'checkboxes_single_select', 'checkboxes_single_unselect'])) {
+        if (in_array($key, ['checkboxes_multiple', 'checkboxes_single_select', 'checkboxes_single_unselect'], TRUE)) {
           // Checkboxes values are not filtered out.
           $values[$key] = array_filter($values[$key]);
         }

Fixed on commit. in_array() should always have the strict param set.