Unify @param $field explanation

None of the hook_field_is_empty() implementations in core do anything with their $field parameter.

Searching through contrib now, but it will take a while, and I'm not sure that if I did find anything it would be useful as a standard.

When in doubt, do as the most popular contrib module does?

Example hook_field_is_empty() implementations which do not ignore their $field parameter:

commerce_customer_field_is_empty()

/**
 * Implements hook_field_is_empty().
 */
function commerce_customer_field_is_empty($item, $field) {
  if ($field['type'] == 'commerce_customer_profile_reference') {
    // profile_id = 0 îs empty too, which is exactly what we want.
    return empty($item['profile_id']);
  }
}

computed_field_field_is_empty()

/**
 * Implements field hook_field_is_empty().
 */
function computed_field_field_is_empty($item, $field) {
  $data_type = $field['settings']['database']['data_type'];
  if ($data_type == 'int' || $data_type == 'float') {
    return !is_numeric($item['value']);
  }
  return empty($item['value']);
}

date_field_is_empty()

/**
 * Implements hook_field_is_empty().
 */
function date_field_is_empty($item, $field) {
  // Sometimes a $item is a date object.
  // Coming from repeating dates. Why?? 
  if (!is_array($item)) {
    return FALSE;
  }
  if (empty($item['value'])) {
    return TRUE;
  }
  elseif ($field['settings']['todate'] == 'required' && empty($item['value2'])) {
    return TRUE;
  }
  return FALSE;
}

Conclusion:

It would appear that $field is expected to be an array structure as returned from field_info_field().

Tentatively rolling a patch that describes the consensus implementation, even though there is considerable disagreement among these three examples:

• The commerce_customer_field_is_empty() function returns NULL if $field is not the expected type.

  So commerce_customer_field_is_empty($item, NULL) would return NULL.

• The date_field_is_empty() function always returns FALSE if $item is not an array.

  So date_field_is_empty(NULL, $field) would return FALSE.

• The computed_field_field_is_empty() function returns TRUE if $item is not of the expected numeric field type.

  So computed_field_field_is_empty(NULL, $field) would return TRUE.

• All other examples not shown implicitly assume that $item is an array of the expected field type, and many will produce PHP warnings if this assumption is violated.

  So hook_field_field_is_empty(NULL, $field) would (in most cases) produce a run-time warning.

None of the Field System hooks currently state whether or not they are required. I know that hook_field_is_empty() is required because of comment #517430-7 by bjaspan and also because of the code and comment in _field_filter_items(). I don't know which, if any, of the other field hooks are required. This documentation bug should be reported and addressed in a separate (meta?) issue, as bjaspan suggested over three years ago.

@#6 by jhodgdon on October 6, 2011 at 3:48pm:

(a) through (d)

Corrected and re-rolled. Also:

Anyway, the field API is not so stupid that it will invoke this hook with $item not being an instance of type $field,

If these functions should never be called from userland, they should say so, and I need to remove the call to og_field_is_empty() from my contrib module. (Which, BTW, is what prompted me to file this issue in the first place.)

Also note the undocumented fact that field_info_field() may return NULL.

(e)

As previously stated:

I know that hook_field_is_empty() is required because of comment #517430-7 by bjaspan and also because of the code and comment in _field_filter_items().

Follow the links if you want; that's why I wrote them.

(Webchick thinks I should spend more time composing my comments, but do people even bother to read them, before replying?)

BAH!!!!

That patch file name was completely wrong, even though the contents are (I believe) correct.

Well, if hook_field_is_empty() is not required, then the following code from _field_filter_items() certainly needs to be changed:

    // Explicitly break if the function is undefined.
    if ($function($item, $field)) {

Note that the "suspicious call to function_exists()" was removed (today) by webchick.

See: #1300482: Remove cruft function_exists() from _field_filter_items() leftover from the (removed) function registry.

Sorry to be so "snarky", but if you are unwilling to read what I wrote the first time, and are unwilling to read what I wrote the second time, I'm not overly anxious to write it a third time.

I'm not sure what the intention is about whether the is_empty() hook should be required or not,

The people who developed the field API were crystal-clear that it should be required. If you're unwilling to read their testimony (linked above and quoted in #1300482) then go ask them yourself.

One more thing, maybe "An array of field data that may or may not be considered empty." would be clearer as "Data for a single field value to test." or something like that? "An array of field data" makes it sound like it could be multiple field values together (it can only be one at a time), and "may or may not be considered" seems a bit redundant?

Revised as requested.

@chx: Since you're basically asking me to review and/or rewrite the entire modules/field/field.api.php file, may I get (from you or someone else) a list of the field hooks which, like hook_field_is_empty(), are required of all field type implementations?

I do not know how to link to a group page from doxygen.

I figured it out. I just need to add the following to the end of the function header:

* @see @link field Field API data structures @endlink.

When I find exceptions to the general rule that @param $field refers to a field structure, should I submit a patch to rename the parameter?

DrupalWebTestCase::assertField()

   * @param $field
   *   Name or id of field to assert.

DrupalWebTestCase::assertNoField()

   * @param $field
   *   Name or id of field to assert.

locale_language_list()

 * @param $field
 *   'name' => names in current language, localized
 *   'native' => native names

@#16 by Lars Toomre on October 7, 2011 at 5:21am:

Shouldn't this patch correct/modify each docblock touched for @param type hinting?

No, the issue discussing type-hinting still lacks sufficient "core developer buy-in".

See #711918-69:

... But we do need buy-in from the core devs on all coding standards... and I'm not sure we're there yet on this idea.

@#17 by Lars Toomre on October 7, 2011 at 5:26am:

Also line '+ * @ingrou pfield' needs to be corrected. I believe you meant '@ingroup field'.

Thanks. Corrected.

@#20 by Lars Toomre on October 7, 2011 at 5:41am:

Maybe this patch should be postponed until that issue is settled one way or another?

Sure; why not. Updated issue summary.

@#22 by salvis on October 7, 2011 at 7:31am:

I'm confused by the term "field structure" when used to describe an actual instance of field data.

Actually, "field structure" or $field refers to the definition of the field type.

A "field instance" or $instance describes the usage of that field type within a content type or "bundle".

A "field item" or $item refers to the data contained in a field within a particular node or entity object.

See the Field API page.

Added a couple of missing end-of-sentence dots.

According to chx, there is absolutely no ambiguity in the term "field structure", especially when used with @param $field. Therefore, according to coding standards, type-hinting should not be used.

But since discussions about coding standards are a complete mis-use of time, energy, and focus, I'll wait until y'all decide what to do before I spend another two hours hand-editing code.

Create issue summary for what is no longer a trivial issue.

Updated status.

@#27 by jhodgdown on October 7, 2011 at 3:54pm:

... couldn't we pick a better term that wouldn't confuse anyone?

Please re-define the word we so that it does not include pillarsdotnet, or else magically unsubscribe me from this issue.

EDIT: updated summary so that "we" is defined as "Core developers".

Waiting for chx or another Field API maintainer to chime in on this one.