Right now, when implementing field_collection_is_empty alter to sense for a particular field collection should be saved or not, without worrying about the dreaded default values making the collection be seen as non-empty (see #1614578: Allow other modules to determine if collection item is empty, avoid saving empty item or rendering empty collection, #1662998: Create configuration option to ignore chosen collected fields when checking for an empty field collection ), the implementer needs to do a lot of work because they get whether the field collection has been detected as non empty by the default field_collection method, but can't see what field triggered the state.

Therefore the implementer of this alter hook has to iterate through the whole field collection again before setting the is_empty value. Something like #1614578-9: Allow other modules to determine if collection item is empty, avoid saving empty item or rendering empty collection That's if you want to affirmatively check in code exactly the right fields.

Mostly though, folks will just want to correct some field that has a default value when carried through, that should register as empty rather than filled. The problem is what about the other fields? It's not safe to assume that the field you care about is the only one that has set $is_empty to false. Because we don't know how other fields validated before the alter hook, and want to leave them alone, we basically have to just copy the entire contents of field_collection_item_is_empty() without the alter hook into your own module.

To provide this information, I've a three line patch that indicates which subfields had content that set $is_empty = false and passes it in variable $non_empty_instances. This reduces the hook implementer effort to a couple lines in the case of a single custom handled field:

function my_module_field_collection_is_empty_alter(&$is_empty, $item, &$non_empty_instances) {
  if ($item->field_name == "my_field_collection_name" &&  ! $is_empty && ! empty ( $non_empty_instances["subfield_i_care_about_name"])) {
      $is_empty = TRUE;
      unset($non_empty_instances["subfield_i_care_about_name"]);
    }
}

It should also allow multiple modules to implement this hook without clobbering each other's work, or repeating it.

CommentFileSizeAuthor
#1 field_collection_is_empty_which_fields_2529180.patch1.21 KByareckon
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yareckon’s picture

yareckon CreditAttribution: yareckon commented
FileSize
field_collection_is_empty_which_fields_2529180.patch1.21 KB
yareckon’s picture

yareckon CreditAttribution: yareckon commented
Issue summary: View changes
artfulrobot’s picture

artfulrobot CreditAttribution: artfulrobot as a volunteer commented

Love the patch, but on your sample use of the new hook I don't get it. If you have a field collection with, say, two fields: text and colour, and say the colour field has a default value.

Now you edit something using this collection and the first row has text and colour set, and the second row has no text and the default value for colour.

When you get to your hook on the first row we have this:

$is_empty = FALSE;
$non_empty_instances = [ 'text' => 1, 'colour' => 1 ];

And the test ! empty ($non_empty_instances["text"]) will therefore return TRUE, which will trigger it to set the field as empty. But it's not!

Now the second row (the empty one except for the default 'colour' value) has just 'colour' in the $non_empty_instances. So the test ! empty ( $non_empty_instances["text"] will therefore return FALSE, which will not trigger setting the field collection to empty?

This seems the opposite of what you indented? Did you mean the test to be empty() (without the !) It's like there's too many double-negatives going on (non_empty_fields being not empty). We want to know whether a field we care about has a value. I think here you're asking is it not not not not empty, whereas I think we need not not not empty!

What I've done is instead test that the $non_empty_instances only contains the fields with default values. So in this example: $non_empty_instances == ['colour'=>1]. That way it's only set to empty if there's any non_empty_values in fields other than the ones that have default values.

Also, what's the point of the unset() that marks the field you care about as empty?

Thanks.

Chris Matthews’s picture

Chris Matthews CreditAttribution: Chris Matthews commented
Status: Needs review » Needs work

Due to artfulrobot's comments above and that the patch does not apply to the latest 7.x-1.x dev snapshot I'm setting the issue status back to 'Needs work'

error: while searching for:
    return $field['type'] != 'list_boolean';
  });

  foreach ($instances as $instance) {
    $field_name = $instance['field_name'];
    $field = field_info_field($field_name);

error: patch failed: field_collection.module:604
error: field_collection.module: patch does not apply