Problem/Motivation

Logically, when a boolean (checkbox) field is unchecked, then logically the inherited isEmpty() method should return true. Because the "On" and "Off" labels are both required in the field's configuration, the isEmpty method returns FALSE even when the checkbox is unchecked when the entity is saved.

Steps to reproduce

Add a boolean field to a content type, and then save a node of that type with the field's checkbox unchecked. Use a debugging tool to test the value of the field's isEmpty() method and observe that the value returned is FALSE.

Proposed resolution

Override the isEmpty() method to evaluate the field value against the "On" label, and return TRUE if the field's value is anything else.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3203038

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mandclu created an issue. See original summary.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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

cassioalmeida’s picture

Hi,

I faced this issue recently, and I added an extra check for boolean types and the value 0.

Please, check the MR and let me know if that works.

cassioalmeida’s picture

cassioalmeida’s picture

I added two asserts in the core/modules/field/tests/src/Kernel/Boolean/BooleanItemTest.php
Without the fix, that test fails.

cassioalmeida’s picture

Assigned: Unassigned » cassioalmeida
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 6: 1183.patch, failed testing. View results

b_sharpe’s picture

Status: Needs work » Needs review

Hiding original patch and set to NR to trigger new test run as seems to be an unrelated failure.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

cassioalmeida’s picture

Version: 9.4.x-dev » 9.3.x-dev

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

minorOffense’s picture

I was getting undefined method errors. Swapped the type check for an equivalent set of methods.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

jonathanshaw’s picture

Logically, when a boolean (checkbox) field is unchecked, then logically the inherited isEmpty() method should return true.

I am not sure that this assumption in the IS is correct.

isEmpty() is TRUE when the field is null, because it has never been set by an editor (i.e. never on a saved form) or set to null programatically.

Changing this expectation has a significant risk of causing confusion and breaking BC. Some sites may be choosing to interpret null as true as a way of defaulting to true for entities that have not been manually inspected by an editor.

I do not encourage anyone to work on this issue without a more high level discussion of whether the approach is correct and how to preserve BC.

gilmord’s picture

Status: Needs review » Closed (works as designed)

The logic behind isEmpty is to check if the field is empty.

Boolean field can have 2 states:
- NULL - the entity was never saved with the field, the field is empty
- 0/1 - the field has a value

If you save the form and the checkbox is unchecked - you set the value 0, and not NULL.
1 is not empty, 0 is not empty (it is value Off), NULL - is empty, and the current implementation is correct and works like that.

If you want to check if the value is 0 you have to check if the value is 0, and not if it is empty
If you want to check the value is empty - check if it is NULL

andre.bonon’s picture

From the Interface point of view (checkbox), it doesn't matter if the value is "NULL" or "OFF" to display unchecked in the browser.
Also, checked will be displayed only if the value is True/ON.
So, having this said, does it make sense to have another method, such as "isChecked" or "isOn", that would return the result of PHP function !empty($value)?

jonathanshaw’s picture

So, having this said, does it make sense to have another method, such as "isChecked" or "isOn", that would return the result of PHP function !empty($value)?

I agree that loading a stored boolean value with some version of !$entity->get('field_myfield')->isEmpty() && !(empty($entity->get('field_myfield')->value)) is a weird Drupalism (and one that I have done sometimes because I'm so unsure what is possible here).

To be able to use some new method or property like $entity->get('field_myfield')->boolean might seem good, but actually I think that technically (bool) $entity->get('field_myfield')->value already works.

The semantics of $entity->get('field_myfield')->value actually seem ok because if $entity->get('field_myfield')->isEmpty() then I think $entity->get('field_myfield')->value === NULL.

cassioalmeida’s picture

The semantic of is means something that is or isn't. `Is` in this case means this could change over time.
By following gilmord's explanation, the result of the method isEmpty behaves like a was. The field was empty; after you update it only once, it does not matter how many times you use the isEmpty to check; it will always be false and depends on your custom code to check for the value.
Due to BC, I'd work and add the isChecked method, as andre.bonon said. isEmpty will be available and keep the same behavior, but isChecked delegates the verification to the object (instead of custom code), and the method name reflects the behavior.

gilmord’s picture

The case here is a wrong usage of the isEmpty() method - the author of the issue tried to check the value of the field and used the emptiness check instead.

Introduction of isChecked() will lead to more misunderstanding and inconsistency (a field type-specific method)

If you want to know if the boolean field is checked - use the {field}->value or {field}->getString() - both will be TRUE for checked and FALSE for empty AND unchecked (which basically is what the author wants to find out). Introducing isChecked() to do the very same logic is unnecessary.

cassioalmeida’s picture

I closed the MR until I didn't have the time to work on a complete solution.

That said, I don't believe that passing the responsibility of knowing the underline implementation of a method called isEmpty will return true only once for the Boolean field, a good thing.

Mainly because it's not the same behavior for other field types, although they share the same interface.

The interface docs mention the expected results:

Drupal\Core\TypedData\ComplexDataInterface

  /**
   * Determines whether the data structure is empty.
   *
   * @return bool
   *   TRUE if the data structure is empty, FALSE otherwise.
   */
  public function isEmpty();

Beyond that, we have a "field-especific method" - we can see it on other field types. For example:

Drupal\Core\Field\Plugin\Field\FieldType\EmailItem overwrites the isEmpty method and checks for the value:

Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem also does it.

Drupal\Core\Field\Plugin\Field\FieldType\StringItemBase and Drupal\Core\Field\Plugin\Field\FieldType\UriItem too.

No matter how many times you edit a node, all those fields, if the field value is empty, the method isEmpty return true; if not, false.

The goal is to:

1-) Avoid custom code to check if the field is empty by accessing its value.
2-) Expect the exact behavior of fields that implements the interface.

gilmord’s picture

Hi @cassioalmeida

Again - this makes no sense to push this issue

It looks like you are confused by the "Checkbox" form widget, as it looks the same for NULL and for FALSE
Please try using the Radio Buttons widget, and you will see the boolean field has actually 3 options:
- NULL
- FALSE
- TRUE

And only NULL is EMPTY, FALSE and TRUE are NOT EMPTY
and this is how the isEmpty() works now, and it is logical.
Treating the value FALSE as EMPTY is not logical.

Please see the attached image:
radios

No matter how many times you edit a node, all those fields, if the field value is empty, the method isEmpty return true; if not, false.

This is the wrong statement. use Radio Buttons and set the field value to empty as many times as you want, not only once.

1-) Avoid custom code to check if the field is empty by accessing its value.

This is wrong also. You do not need to check value to know if the field is empty. You can use the isEmpty() method. If you want to check if the field value is FALSE - you HAVE to check the field value. Do not mix EMPTY and FALSE.

Please stop pushing this.

jonathanshaw’s picture

@gilmord is right AFAICS.

In php empty(FALSE ) === TRUE. But the semantic of empty is just different in Drupal.

Drupal's semantic is more like php's is_null() than empty().
is_null(FALSE ) === FALSE

cassioalmeida’s picture

Thanks for the explanation.

This is the wrong statement. use Radio Buttons and set the field value to empty as many times as you want, not only once.

That statement was about the EmailItem and some other classes under the namespace Drupal\Core\Field\Plugin\Field\FieldType;

I did the following test — a CT with two fields. Email and Boolean (checkbox widget).
Then I created a node and left both fields without filling them out.
Then, on a node preprocess, I created the following example:

//Boolean field
$node->get('field_featured')->isEmpty();
//Email field
$node->get('field_email')->isEmpty();

The result of calling isEmpty on the Email field changes every time I update the node. If I save the node with some value in the Email field, the isEmpty result is false; if I update the node and leave the field without any value, the result of isEmpty is true. So, in that case, I can "trust" the isEmpty method to check against the value.

Probably that was the motivation to create the issue. By using the Radio widget as an example, it does make sense. But this thread can clarify for anyone in the future looking for why it "does not work" for Boolean fields.

cassioalmeida’s picture

Assigned: cassioalmeida » Unassigned
Steven Snedker’s picture

Boolean field can have 2 states:
- NULL - the entity was never saved with the field, the field is empty
- 0/1 - the field has a value

A boolean with three values (NULL, 0, 1) has no upside whatsoever. At least I cannot imagine a single example where it's nice or convenient to use a boolean field to find out if a user has saved his/her user profile, webform or some node.

Booleans with three values (or two states and two values) makes a routine entityQuery awfully complex.

Here's an example:

$query = \Drupal::entityQuery('node')
  ->condition('type', 'measurement_result')
  ->condition('has_been_processed.value', 1, '!=')
  ->sort('changed', 'DESC')
  ->range(0, 20)
  ->accessCheck(FALSE)
  ->execute();

will not give me the 0 and NULL nodes. Only the has_been_processed=0 nodes.

So I have to make an additional

  $query = \Drupal::entityQuery('node')
  ->condition('type', 'measurement_result')
  ->notExists('has_been_processed.value')
  ->sort('changed', 'DESC')
  ->range(0, 20)
  ->accessCheck(FALSE)
  ->execute();

"Ah," you may say. "You can make that more compact. Try:

$orGroup = $query->orConditionGroup()
  ->condition('field_saved_as_single.value', 1, '!=')
  ->notExists('field_saved_as_single.value');

But no. That does not work. notExists's and isEmpty's are not welcome in orConditionGroup's.

Is there a simpler way to find out if a checkbox has been checked?
And if not, can we make it?

gilmord’s picture

Hi @Steven Snedker

This question is not new, and it was discussed many times with the core contributors. Long story short - it works as designed (=

The "good" way for your case (get unchecked and never checked) will be to force 0 as a default value.
But here comes the problem - what if you add a boolean field to the existing entity? The problem appears when you have millions of existing entities.
It does not allow us to implement any fast filling of the values - it can get timeouts and staff like that.
So this has to be handled by the developer by adding batch processing, like Drush command or views bulk operation. There are contrib options for that - https://www.drupal.org/project/field_defaults

I suppose this is the main reason we face empty values - as you can see here https://www.drupal.org/project/drupal/issues/1919834 - it works as designed (you can check the thread to find explanations).

Another possible option is to use a query instead of an entity query and use the LEFT JOIN to relate the boolean field table.

gilmord’s picture

A boolean with three values (NULL, 0, 1) has no upside whatsoever. At least I cannot imagine a single example where it's nice or convenient to use a boolean field to find out if a user has saved his/her user profile, webform or some node.

Sounds right, but as far as a boolean is not just a value but a separate table - the problem appears, you have to JOIN the table to use it, and default join (INNER JOIN) will skip rows with unexisting values, and LEFT JOIN is much heavier to execute.

Just thought of one more workaround for you - you can add boolean field as a base property (https://www.drupal.org/docs/drupal-apis/entity-api/defining-and-using-co...).
It can inherit all the benefits of the field but will be stored in the base entity table as single column. Like a "status" of the node.
I think this will simplify the queries as there will be no need to join the boolean table.

The bad side of this option - you may end up with many extra columns in the base entity table.

Steven Snedker’s picture

You are absolutely correct. I added a boolean field to an existing content type and pretty soon had

"NULL" in 5000 nodes
"0" in 200 nodes
"1" in 150 nodes

Loading all nodes without "1" took two fairly long entityqueries.

That sucks.

Thank you for pointing me to Field Defaults

That is the fastest, most robust and least sucky solution to this unfortunate problem.

Base property is a novel solution and creative. Well spotted. But knowing how Drupal is increasingly creaking under it's own weight I'll go with the simpler Field Defaults solution.

Thanks a lot for the answer.