A required boolean field behaves differently depending on the widget

Adding to summary. Upping to major as this could cause data loss through misunderstood configuration, or users not signing a legal agreement / data protection permission, etc.

This seems to work for me. I think the IS is absolutely on point. Note that this is rather problematic because we would like to mark the published field as properly required over in #2841291: Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field.

I added a lengthy comment. Feel free to improve on that or make that less verbose. But I thought since this is quite a weird subtlety, I'd rather explain with a bit too much detail as too little.

The patch looks like it will provide the proposed fix:

> When the single checkbox widget is used on a required field, you should not be forced to check it. Off is a valid state (as long as the value being used is an actual 'no' value and not NULL).

However, is that the way we want to fix this?

When we say a boolean field is required, do we mean that you must say TRUE, or do we mean that you must say something non-NULL?

Furthermore, if we go with this fix, we need a CR and we maybe also need to think about an upgrade path, because this is going to break every site that's used a boolean checkbox for a field such as "Ticky this box to confirm you accept our terms and conditions".

Yes, I guess you're right. So I guess we could add a setting to BooleanCheckboxWidget that allows configuring whether or not to set #required And we could only show that setting in the UI if it makes sense in the first place i.e. if $cardinality === 1 && $required === TRUE. Then we could set that to TRUE in an update hook and keep FALSE as the default value.

Here's a first start. Not sure if the update path test passes, having some issues with that locally.

Also botched up my local so that there's no interdiff, but I pretty much moved everything around anyway, so hope that's alright.

+++ b/core/modules/system/tests/src/Functional/Update/BooleanCheckboxWidgetSettingsUpdateTest.php
@@ -0,0 +1,63 @@
+use Drupal\system\Entity\Action;

Oops, that's unused.

Fixes #11 and attempts to fix the update path.

This should be green.

Also updates all default form displays so they match the imported status.

This should be ready for some reviews.

Thought about this some more. It really doesn't make sense to set this value to TRUE for non-required fields. It doesn't actually break anything because the checkboxes will not end up being required, but it's confusing when looking at the config export. So we should both change the exported config to required: FALSE where applicable (which I think should be everywhere) and also make the update path smarter about this.

+++ b/core/modules/system/system.post_update.php
@@ -111,3 +111,26 @@ function system_post_update_change_action_plugins() {
+      if (isset($component['type']) && ($component['type'] === 'boolean_checkbox')) {
+        // While the required setting is FALSE by default, setting it to TRUE
+        // matches the previous behavior.
+        $component['settings']['required'] = TRUE;

Hmm this update will cover only instances of this type. What about those that are extending from that widget? If I get it right then they suddenly will have a different behavior? Should we instead load the widget and check if it extends from the BooleanCheckboxWidget?

field_update_8001() and field_update_8003() for example handle also descendants of EntityReferenceItem.

#17 makes sense to me. Fixes that and #16 and also cleaned up a bunch of more stuff and added a required boolean field to the upgrade path.

So node_update_8002() was causing troubles for me locally, so I uncommented it. Didn't mean for that to end up in the patch. Reverted that and also added an explicit test for the setting. This is looking pretty reviewable to me now.

This needed a re-roll. Also fixed the coding style violations and added the new setting to a bunch of new displays that were added in the meantime.

This should fix the tests and the coding standards violations.

Updated the issue summary. Will add a draft change record now, so hopefully nothing else prevents this from being RTBC.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/BooleanCheckboxWidget.php
@@ -39,6 +40,23 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
+    // Strictly speaking the checkbox widget should only be available for
+    // required field definitions in the first place, but enforcing this would
+    // both break existing sites and present a confusing user interface to users
+    // who are not aware of this subtlety.

I don't really understand the first part. Could you please explain why "strictly speaking the checkbox widget should only be available for required field definitions"?

The new setting defaults to FALSE so that - by default - checkboxes will not be marked as required. This is a change to the previous behavior.

For all existing required boolean fields that are using the checkbox widget we set this setting to TRUE to match the previous behavior.

I was just thinking about an use case for having a required checkbox and such example is where the user has to agree with "Terms and conditions", where we might want to force the user to click the checkbox.

However I think that we need a sign off from a product manager for this change - tagging accordingly. I think the patch looks good, it is just that I cannot decide on what the default behaviour should be.

I wasn't really following this issue for a while.

I don't think my question from earlier has been figured out:

> When we say a boolean field is required, do we mean that you must say TRUE, or do we mean that you must say something non-NULL?

The problem in this issue stems from the fact that we know what it means for a checkbox form element to be required: it MUST be enabled. But it's not clearly defined what it means for a boolean field to be required.

When we say a boolean field is required, do we mean that you must say TRUE, or do we mean that you must say something non-NULL?

The required flag on any field does not have anything to do with the value - in every case and for every field it means that there should be a field value.

WidgetBase flags the form element as required if the field is required, which most probably not by design has resulted into the CheckboxWidget to force users to check the checkbox in order to submit. Therefore the solution in this issue is the right one, as it leaves it to the corresponding form display to decide whether to force the user or not.

[Crosspost with #29]

Sorry if I steamrolled the discussion a bit with my patches, but to me the situation is fairly clear. I would love to hear your thoughts! Here is how I see it:

When we say a boolean field is required, do we mean that you must say TRUE, or do we mean that you must say something non-NULL?

Field API definitely means the latter. To Field API the boolean field is "just another" field so the semantic must be the same as it is for any other field.

The problem is that basically "by accident" (because the required state of the field definition is "blindly" turned into the #required state of any widget) the current behavior of the checkbox widget is the former. Additionally, checkboxes don't have an "empty state" or in other words there is no distinction between FALSE and NULL with checkboxes. So speaking strictly in terms of the data model (not in terms of usability) the checkbox widget should only be allowed for required boolean fields, as they cannot adequately cope with the tri-state situation that is required for non-required boolean fields.

In practice, in most cases the distinction between a NULL and a FALSE value is often times irrelevant, so for many people the current situation is perfectly fine, because (if you do not distinguish between FALSE and NULL) you can serve both use-cases from the quote with current Drupal: If you want the former behavior, make the field required, if you want the latter make it not required.

Per the issue summary what we want, however, is to be able to have a non-required checkbox for a required boolean field. We cannot "just" change the behavior wholesale, because that would remove the possibility for people to configure the former use-case. I.e. we would break everyone's "Do you agree to the ToS" fields, that they implemented with a required boolean field with checkbox widget.

So to resolve this, the current patch provides a setting for the checkbox widget which is basically a toggle between the former and the latter use-case. It is only available if the boolean field is required, as otherwise it does not make sense in any case to have a required checkbox.

> So to resolve this, the current patch provides a setting for the checkbox widget which is basically a toggle between the former and the latter use-case. It is only available if the boolean field is required, as otherwise it does not make sense in any case to have a required checkbox.

I think what feel wrong to me with this is that the new setting is on the widget.

Consider that we want to build a field that requires users to agree to terms and conditions. With this new setting, we make it a required checkbox, and set the 'must be TRUE' setting on the widget. Then the site builder exposes that entity type to REST and BOOM the entity validation doesn't enforce this and the site gets loads of malformed entities saved via the REST endpoint.

If I were designing this from scratch, rather than fixing this bug, I'd do one of:

a. add a second setting to boolean fields for 'must be TRUE'

b. add a new field type called something like 'confirmation' which stores like a boolean, but must be set to TRUE. (This is the same pattern that core uses for its special 'created' and 'changed' fields; they are field types that are basically just timestamps, but with special behaviour baked-in.)

@joachim, I already came up with the Terms and conditions example in #27. The only problem I see with that is whether the default widget setting should be FALSE or TRUE, which I decided to leave for a product manager to decide.

I think what feel wrong to me with this is that the new setting is on the widget.

It is on the widget, because it is only about the representation and nothing else.

Consider that we want to build a field that requires users to agree to terms and conditions. With this new setting, we make it a required checkbox, and set the 'must be TRUE' setting on the widget. Then the site builder exposes that entity type to REST and BOOM the entity validation doesn't enforce this and the site gets loads of malformed entities saved via the REST endpoint.

The widget does not have anything to do with the rest calls, which will be behaving after the change as they did before that.

The widget is used by the form API and whether REST nor jsonapi make use of it, as they purely deal with the entity API and there nothing changes because if the boolean field is required they will fail the entity validation if no value is provided.

The field and its representation are two different things. And I can see use cases where I want that the field is required - to have a false or true value saved, but I also want depending on the field usage in a form to change the behavior of whether I want to force the user to agree or let the user also disagree. So depending on different conditions using the very same field I might wanna provide different configuration for different form displays and this is only possible when the corresponding widget is configurable.

If we leave the default value to TRUE then there will be absolutely no change and I think we'll not even need a product manager review as we'll not be changing a behavior, but making it possible to configure the existing behavior.

Reverting accidental change in the issue summary.

Hi,

I tried patch #24 and it fails to apply with core 8.9.x. So, I am changing the status to "Needs Work".

Below is an error screenshot.

Thanks,
Ren

I think putting this in the widget is fine. (I don't understand why our on/off label definitions are on the field, and understanding that would lend understanding to the question here)

First I rerolled for 8.8.x (and 8.9.x).

Then I noticed several places where 'required' was not set, possibly because core has changed since this was first written. Since the default is 'false', we don't need to add it everywhere, but for completeness I think we should.

I didn't check if any of these new places should actually have set 'required' to TRUE, so first I'm going to queue the test bot, and assuming it pass, then I'll ask that someone review this for me.

Fixing test case failures.

Updating patch to fix the test cases. Fixed incorrect array format in assertEquals

Uploading the correct patch and interdiff.

Validated the patch on core version 8.8.2

Can anyone provide patch for 9.1 ?

I think it makes sense to merge this with #2306331: "Single on/off checkbox" widget makes no sense for boolean fields with cardinality > 1 and "Check boxes/radio buttons" makes little sense for boolean fields with cardinality > 2? They are slightly different in that this one takes issue with required aspect where the other one takes issue with cardinality, but it seems like the the solution should factor in both scenarios and solve them in one go.

Adding credit for aditya.ghan and pameeela - marked #2306331: "Single on/off checkbox" widget makes no sense for boolean fields with cardinality > 1 and "Check boxes/radio buttons" makes little sense for boolean fields with cardinality > 2 as a duplicate of this

Re-rolled the patch in #42 against 9.1.x branch as requested in #45. Kindly review.

Thank you!

Updating the patch. Please review the same. Thanks!

I came here to review and test the patch but it no longer applies. Tagging for a reroll.

I then read the issue and reviewed the patch.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/BooleanCheckboxWidget.php
   @@ -26,7 +26,8 @@ class BooleanCheckboxWidget extends WidgetBase {
   +] + parent::defaultSettings();
   

   This formatting looks wrong.

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/BooleanCheckboxWidget.php
   @@ -39,6 +40,23 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
   +    // There is no way to distinguish between the "Off" value and no value at
   

   This comment is verbose and hard to follow. A request to improve this was also in #27.

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/BooleanCheckboxWidget.php
   @@ -39,6 +40,23 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
   +      '#title' => t('Force users to check the checkbox.'),
   

   Is 'force' used in the Drupal user interface? Can't it change to use 'requires'. 'Requires the user to select the checkbox'.

There are no tests in the patch for this change. That is needed as well as a fail test.

Fixed 1st and 3rd points in the comment #56

Re-rolled the patch. Currently adding patch to see if we get any failures etc. with the updated changes.
Later, I'll add the test cases in the next patch.

Patch #57 applied successfully for me in drupal 9.3.x-dev version.
I have reviewed the patch as per #56 and 1st and 3rd points have been fixed in this patch. The required message is appearing fine now.

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/BooleanCheckboxWidget.php
@@ -39,6 +40,23 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
+    // Strictly speaking the checkbox widget should only be available for
+    // required field definitions in the first place, but enforcing this would

I don't understand this comment.

Why should a checkbox widget only be available for required fields? Does it mean that it's because you can never unset a value with a checkbox widget?

Re-roll.

Fixed the custom command errors in #61, thanks!

I did some testing with the latest patch on Drupal 9.4.x, standard install and this appears to work as intended.

What I find most confusing is that boolean, field marked required will not be displayed as required (red asterisk) unless that new widget setting is also checked. Is there some way to inform the user?

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/BooleanCheckboxWidget.php
@@ -39,6 +40,23 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
+      '#title' => t('Requires the user to select the checkbox.'),

Should this be 'Require' or 'Requires'?

I think this would benefit from a usability review, adding tag.

I didn't review the patch.

+1

also I have a potentially related issue. I have a boolean field with radio buttons in the node form. It is set to "required". However, the node can be saved even when the boolean field isn't filled. Since my code tries to work with the bool value it might even crash the entire node view page so this is a pretty critical error for me.

A possible workaround to have not any NULL but only 0 or 1 values for a boolean field is the module field_defaults. Nevertheless I wish to have this issue properly solved.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Picking up again. Summarising all the comments and feedback so far.
- Re-rolled the patch.
- Added two test cases to validate the field widget settings and behaviour of checkbox option on the form.
- Re-roll diff mostly contains the test cases added by me.
- Keeping "needs tests" tag to see if we need further tests.

Happy to add additional test cases based on feedback from folks.

Pending action items:
#63 Needs some review from usability team.
#59 We need to address feedback from @joachim.
I tried to made sense of the description, however this is there since beginning so I think @tstoeckler is the right person to comment on that.
#56 Second point. I think someone with more command over english can rephrase it. I am somewhat confused it.

Regarding the comment: Yes, @joachim in your words because you can never unset a value with a checkbox. I actually tried to explain the situation in the comment directly preceding that line that is quoted in #59. Re-reading it again now I think one thing that is missing in that whole comment is that for Drupal a required field means that it has a value, which in case of boolean fields may include FALSE. Maybe the following table will make clear what I'm trying to get at (and what this issue is about):

The problem is that both scenarios have valid use-cases, the first one is, for example, "Do you want to receive our newsletter?" where it totally makes a difference whether someone has explicitly said "No" or not yet made a choice. The second is "Do you agree to the terms of service?" where that distinction is meaningless and the "required" aspect really means the "Yes"-value is required.

To make things even more confusing you can actually model both things with Drupal form displays already. In both cases you configure the boolean field to be required and in the first case you choose the radios widget so that the user can explicitly choose the "No"-value and in the second case you choose the checkbox widget.

And that is exactly what this issue is about: The choice of widget should not be a determining factor for the data model. It should only control how the data is input, not what kind of data is valid.

Now from a pure data modelling perspective we could just say "the checkbox widget is doing it wrong" and just never mark the checkbox input required. Since the checkbox input would effectively still yield a FALSE value, this would be in-line with the field being required from a Field API perspective. But that would mean we would break the second (terms of service) use-case which has worked since (at least) Drupal 8.0.0 so that is presumably not a viable option.

So this patch attempts to rectify the situation by adding a setting to the checkbox widget which basically lets you choose between the two scenarios. In terms of the table this then becomes:

Hope that makes the situation more clear.

Not sure how exactly the comment in the code should be updated, but my suggestion would be to add something like "for Drupal a required field means that it has a value, which in case of boolean fields may include FALSE." to the beggining of the paragraph.

Fixing the CCF issues with #69

Fixing the test case failures.
I think it was related to incorrect namespace path.
Test is already passing on local.

What's a use case for a required Single On/Off checkbox? If the user is required to check it why not just make the default value be true?

Agree it's weird you can make the boolean required and if you use that widget it can be saved without checking just having a hard time with the use case. Maybe some warning text on the field setting for a boolean could be used?

What's a use case for a required Single On/Off checkbox? If the user is required to check it why not just make the default value be true?

Something like a data privacy or terms of service agreement. Even though you cannot submit the form without checking the box, you need to make sure that the user explicitly checks the box and you need to have a record of that.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Update patch for 10.3.0

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/BooleanCheckboxWidget.php
@@ -38,6 +39,23 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
+    // Strictly speaking the checkbox widget should only be available for
+    // required field definitions in the first place, but enforcing this would

Why?

I don't understand this at all.

If the checkbox is 'Agree to site terms and conditions' then yes.

But a checkbox could be 'Promote to front page' or 'I want a child meal'.

Re #79: That's the entire point of this issue: For those examples that you mentioned there is no point in distinguishing between the off-value ("Do not promote this to the front page" / "I do not want a child meal") and no value ("Not known whether to promote this to the front page or not" / "Not sure if I want a child meal"). And in terms of Field API "required" means "there must be a value", not necessarily "the on-value / yes-value / ... must be chosen". And if you do in fact want to allow making a distinction between the off-value and no value then the checkbox is not a suitable widget for that, because it inherently cannot make that distinction.

That said, I absolutely welcome any suggestions on improving the in-code documentation, as apparently it's not as clear as it could be.

Usability review

We discussed this issue at #3459320: Drupal Usability Meeting 2024-07-12. That issue will have a link to a recording of the meeting. For the record, the attendees at today's usability meeting were @AaronMcHale, @benjifisher, @rkoller, @shaal, @simohell, and @worldlinemine.

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

At first thanks for working on the issue. In general we had a clear consensus that this feature is useful and the implementation looks good.

In regards of the question in #63, we've agreed to go with Require changing the string to Require the user to select the checkbox.. That way, the label would be consistent with the other label Use field label instead of the "On" label as the label. on that form display widget.

And we've noticed a detail about the single on/off radio buttons option, which is out of the scope for this issue. If you create a boolean field, set the form display widget to Single on/off radio buttons, then set the field to required within the field settings, and check the Set default value checkbox, you then get three options: N/A, Off, and On. With the Required field checkbox checked the only possible options are either Off and On. If you now set the default value to N/A, save, and reopen the field setting you will notice that the previously ticked Set default value checkbox got unticked and the Set default value checkmark got dropped again. If you take a look at the configuration for the field you notice either way if the Set default value checkbox is checked without an option or with N/A, default_value: { } remains empty after saving the field settings. So the N/A option is sort of without any purpose except the potential to confuse user and it would be way more clear if the N/A option would be removed from the list of options when a boolean field is required. But that should be moved to a follow up issue (Update: I've opened: [#3461121: Remove the N/A option from the list of default values for a required boolean field).

Awesome, that's great to hear, thanks for the usability review to all those involved. Will try to get an updated MR going here soon, but can't promise anything...

Update patch for 10.3.5