Currently the field_set_empty() function is blindly calling [module]_field_is_empty() without checking that it exists or pulling it in if necessary through drupal_function_exists(). This makes it impossible to put all the field-related hooks in a separate file such as [module].field.inc. This patch wraps the function call in drupal_function_exists().

CommentFileSizeAuthor
#2 field_check_empty_exists.patch932 bytesquicksketch
field_check_empty_exists.patch864 bytesquicksketch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch failed testing.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented
Status: Needs work » Needs review
FileSize
field_check_empty_exists.patch932 bytes

Good ol' test bot. Now re-indexes the array of $items after dropping entries.

yched’s picture

yched CreditAttribution: yched commented

We definitely miss a drupal_function_exists() to load the file if needed, but I've been trying to resist the 'fail silently' feature in D6.
A field-type module that doesn't implement the hook should be considered broken IMO, and a fatal error at least makes that clear.

Or maybe we can fallback to a reasonable default : field value is empty if (empty($item[$column])) for all columns ?

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

Maybe we could set $items = array(), so no data will save at all and set a watchdog error?

else {
  $items = array();
}
Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

Related: #332733: Implement a way to require that some functions are available

yched’s picture

yched CreditAttribution: yched commented

Actually, both 'reasonable default behaviors' mentioned in #3 and #4 have the drawback that they hide errors under the carpet, making them much more difficult to actually detect and fix.
With a 'fatal error, undefined function mymodule_field_is_empty', you can't really ship a broken module ;-)

bjaspan’s picture

bjaspan CreditAttribution: bjaspan commented

Assuming hook_field_is_empty() is intentionally a required function to implement the Field Types API, the ideal behavior given a field type module that does not implement it is for the code to fail to compile. Since we can't get that in Drupal, I agree that fatal error is the next best thing. Thus, the current code is correct.

If we want to declare that hook_field_is_empty() is actually optional because there is a reasonable default behavior, that's fine too. I have no opinion about this particular function, yched is the one qualified to have an opinion on it.

I do note the following points:

1. hook_field_is_empty() does not seem to be documented in field.api.inc or http://api.drupal.org/api/group/field_types/7.
2. The Field Type API docs should specify explicitly which hooks are required and which are not.

ISTM that the original point of this issue is a "won't fix" but that we should repurpose the issue for the items above.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

I agree that fatal error is the next best thing. Thus, the current code is correct.

Whoa, no it is not! Per my original statement:

This makes it impossible to put all the field-related hooks in a separate file such as [module].field.inc.

This is not ideal behavior and nullifies the entire purpose of the function registry. If we intentionally want to break sites we might as well put an exit() in there or call function "goobablygookfunction()".

If you really want to continue calling it even if it doesn't exist, we don't *have* to put it in an IF statement, it's just natural to do so.

drupal_function_exists($function);
foreach ((array) $items as $delta => $item) {
  if ($function($item, $field)) {
    unset($items[$delta]);
  }
}
bjaspan’s picture

bjaspan CreditAttribution: bjaspan commented

Right. My bad; it was late. :-)

yched’s picture

yched CreditAttribution: yched commented

re quicksketch: yes, I wrote in #3 "We definitely miss a drupal_function_exists() to load the file if needed" :-)

Status: Needs review » Needs work

The last submitted patch failed testing.

yched’s picture

yched CreditAttribution: yched commented
Status: Needs work » Fixed

This went in with Field UI, and is now dead anyway after the removal of the function autoloader.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.