list_field_is_empty() always returns FALSE for list_boolean fields

i've never committed a patch before so i'm going to give this one a whirl.

I am unable to re-produce this issue. Here is what I tried:

1) Made a content type with multiple value boolean field. I let it have up to two values.

2) Created 3 nodes; one with two checked, one with one checked and one with none checked.

3) I wrote some PHP code in Devel that retrieved all three nodes from the database and then tested the results of running list_field_is_empty() on each of them:

$empty = list_field_is_empty($node->field_values['und'][0], 'field_values') ? "empty" : "not empty";

4) Ran the script and the function correctly saw that the first two where not empty and that the last one was.

I would love to continue working on this; would someone be able to send me some steps to reproduce that worked for them?

Thank you!

I would like to work on this issue.

I am able to reproduce this issue when test a boolean field in field collection.

$entities = entity_load('field_collection_item', array(1));
$entity = reset($entities);
$is_empty = list_field_is_empty($entity->field_check_me['und'][0], 'field_check_me');

$is_empty is always set to FALSE no matter $field_check_me has the value 0 or 1.

I checked the Options module in 8.0.x. It looks like this same code still exists there, so it must be fixed in Drupal 8 first.

@dcam, thanks for your suggestion and I will keep working on it for Drupal 8 first. Now I need to figure it out why my patch was failed by testbot. Can any community member give me some clues?

As per 1969728, this functionality/check is now in \Drupal\Core\TypedData\Plugin\DataType\Map, and I can confirm that isEmpty() on BooleanItem's is returning TRUE for unchecked boxes. It also appears to function correctly when a new field gets added and there is no value set on an entity for a boolean field.

At the DrupalCon sprint and am looking into this

This issue no longer applies to Drupal 8 since the list module was removed from Core and merged with the Options module: https://www.drupal.org/node/1691614

However the same function seems to exist in D8 at core/modules/options/src/Plugin/Field/FieldType/ListItemBase.php on line 80:

public function isEmpty() {
  return empty($this->value) && (string) $this->value !== '0';
}

Unassigning and changing to appropriate component

Instructions on writing a reroll https://www.drupal.org/patch/reroll

Hello, the patch seems to be really simple to reroll by hand (just one line) by putting it to right location. Plus I can see a what a problem is in the patch so I'll fix it and push new patch soon.

On a second thought isn't isEmpty method of abstract class \Drupal\Core\TypedData\Plugin\DataType\Map which is extended by \Drupal\Core\Field\FieldItemBase responsible for checking that. Why do we need to define a separate method? Unless we are altering the behaviour of method isEmpty.

Hi guys, I've created a patch that should be solving the problem with "list_boolean" type. I wasn't able to test the method ListItemBase::isEmpty() it locally as I would need to play with it (and Drupal 8) more and I have no time for it now. Sorry.

My patch contains if-else clauses instead of one complex line (that is in my opinion not readable enough). I believe that if-else substitution for some &&, || should result in the same performance, but provides more readability (and thus better understanding for future modifications).

One question about best practice: wouldn't be the best to write tests first when someone find a bug that wasn't recognized by any existing test?

It would make fixing the bug easier since when we have failing test its easy to see whether the failing test is passing after the fix or not.

I think it would be preferred if the ListBooleanItem class would override the isEmpty() method. Then we don't need an if-else at all.

Re-rolled most recent patch.

1. +++ b/core/modules/options/src/Plugin/Field/FieldType/ListItemBase.php
   @@ -78,7 +78,18 @@ public static function generateSampleValue(FieldDefinitionInterface $field_defin
   +      return (string) $this->value === '0';
   +    }
   

   return value should be typecast to bool instead of (string)

2. +++ b/core/modules/options/src/Plugin/Field/FieldType/ListItemBase.php
   @@ -78,7 +78,18 @@ public static function generateSampleValue(FieldDefinitionInterface $field_defin
   +      return (string) $this->value !== '0';
   

   return value should be typecast to bool instead of (string)

Addressed #30.Thanks!

Another small change.

+++ b/core/modules/options/src/Plugin/Field/FieldType/ListItemBase.php
@@ -78,7 +78,18 @@ public static function generateSampleValue(FieldDefinitionInterface $field_defin
+    if(!empty($this->value)) {

Following coding standards, the '(' after the if should have a space before it, like 'if ('

Adding a space. Thanks for pointing that out.

Fixing code formatting issue.

Is it possible to create a test which fails on HEAD that demonstrates this bug?

Also some feedback:

1. +++ b/core/modules/options/src/Plugin/Field/FieldType/ListItemBase.php
   @@ -78,7 +78,18 @@ public static function generateSampleValue(FieldDefinitionInterface $field_defin
   +    // Based on the PHP empty() function the value is not empty, so there's no need to continue.
   

   This is longer than 80 columns.

2. +++ b/core/modules/options/src/Plugin/Field/FieldType/ListItemBase.php
   @@ -78,7 +78,18 @@ public static function generateSampleValue(FieldDefinitionInterface $field_defin
   +    // Type of list_boolean
   

   Comment seems superfluous and is missing a full stop.

3. +++ b/core/modules/options/src/Plugin/Field/FieldType/ListItemBase.php
   @@ -78,7 +78,18 @@ public static function generateSampleValue(FieldDefinitionInterface $field_defin
   +      return (bool) ($this->value == '0');
   

   The expression doesn't need to the parens and does not need to be cast to bool.

4. +++ b/core/modules/options/src/Plugin/Field/FieldType/ListItemBase.php
   @@ -78,7 +78,18 @@ public static function generateSampleValue(FieldDefinitionInterface $field_defin
   +      return (bool) ($this->value != '0');
   

   Same here.

Done as suggested.

Does this need the <h3>Why this should be an RC target</h3> info & RC phase evaluation table? Also there's the "rc target triage" tag.

D7 version base on https://www.drupal.org/files/issues/list_field_is_empty-2028085-41.patch

We'll take this issue at Drupalcon Dublin

Are we good at this ?

Adding test in patch #41 for version 8.5.x :)

#41 still applies

Needs review

The patch can apply well on branch 8.6.x and 8.8.x

Re-rolling patch for Drupal 8.8.x

The patch #63 applied successfully and working fine on the current 9.4.x version. Added a screenshot of the patch.

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.

Patch for Drupal version 10.1.x

Adding Reroll for 10.1.x

Re-rolled #63 for 10.1

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

As a bug this will need a test case