The attached one-line patch removes a bare function_exists() from _field_filter_items() that was missed in the clean-up patch at #497118-154: Remove the registry (for functions).

Files: 
CommentFileSizeAuthor
remove-function_exists-from-_field_filter_items.patch544 bytespillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 33,301 pass(es).
[ View ]

Comments

History of that line of code:

#517430 by quicksketch on July 13, 2009 at 3:37am
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().
#517430-7 by bjaspan on July 14, 2009 at 3:32am
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.

#516138-74 by quicksketch on August 11, 2009 at 12:48am
--- modules/field/field.module 11 Aug 2009 14:59:40 -0000 1.22
+++ modules/field/field.module 12 Aug 2009 23:21:56 -0000
@@ -280,14 +280,15 @@ function field_associate_fields($module)
  */
function field_set_empty($field, $items) {
   // Filter out empty values.
-  $filtered = array();
   $function = $field['module'] . '_field_is_empty';
-  foreach ((array) $items as $delta => $item) {
-    if (!$function($item, $field)) {
-      $filtered[] = $item;
+  if (drupal_function_exists($function)) {
+    foreach ((array) $items as $delta => $item) {
+      if ($function($item, $field)) {
+        unset($items[$delta]);
+      }
     }
   }
-  return $filtered;
+  return $items;
}
/**
#516138-75 by yched on August 11, 2009 at 3:03am
drupal_function_exists() is needed, but I'd rather not fail silently if function doesn't exist. The field module is broken. + we should keep this in #517430: field_set_empty() missing drupal_function_exists()
#516138-94 by yched on August 14, 2009 at 1:22am
@@ -279,15 +305,15 @@
  * TODO D7: poorly named...
  */
function field_set_empty($field, $items) {
-  // Filter out empty values.
-  $filtered = array();
   $function = $field['module'] . '_field_is_empty';
+  // We ensure the function is loaded, but explicitly break if it is missing.
+  drupal_function_exists($function);
   foreach ((array) $items as $delta => $item) {
-    if (!$function($item, $field)) {
-      $filtered[] = $item;
+    if ($function($item, $field)) {
+      unset($items[$delta]);
     }
   }
-  return $filtered;
+  return $items;
}
/**
#516138-134 August 18, 2009 at 3:54am by quicksketch
@@ -279,15 +305,15 @@ function field_associate_fields($module)
  * TODO D7: poorly named...
  */
function field_set_empty($field, $items) {
-  // Filter out empty values.
-  $filtered = array();
   $function = $field['module'] . '_field_is_empty';
+  // We ensure the function is loaded, but explicitly break if it is missing.
+  drupal_function_exists($function);
   foreach ((array) $items as $delta => $item) {
-    if (!$function($item, $field)) {
-      $filtered[] = $item;
+    if ($function($item, $field)) {
+      unset($items[$delta]);
     }
   }
-  return $filtered;
+  return array_values($items);
}
/**
Wed, 19 Aug 2009 13:31:14 +0000 by Angie Byron
#516138 by yched, KarenS, quicksketch, bangpound, et al.: CC-FREAKING-K IN CORE! OH YEAH! :D
Mon, 24 Aug 2009 00:02:05 +0000 by Angie Byron
*** empty log message ***
Mon, 24 Aug 2009 00:10:46 +0000 by Angie Byron
Of all the patches to accidentally commit without a message. :( Rolling back registry rip. Let's try that again.
Mon, 24 Aug 2009 00:14:23 +0000 by Angie Byron
#497118 by chx, catch, pwolanin, JoshuaRogers, and Jacob Singh: Remove the function registry. While the hope was that this would result in improved performance for low-end hosts, it comes at the expense of critical development experience problems and less benefit than something like APC. Class registry remains intact to facilitate autoloading.

Status:Needs review» Reviewed & tested by the community

If it's required, is it documented so?

@#3 by chx:

If it's required, is it documented so?

Apparently not, but that is a separate issue.

In fact, none of the hook implementations listed on the Field Types API page state whether they're required or optional.

As noted by bjaspan in #517430-7 (see history above):

The Field Type API docs should specify explicitly which hooks are required and which are not.

This has not yet happened, and probably should be opened as a two-year-old documentation bug.

Version:8.x-dev» 7.x-dev

Thanks! Committed and pushed to 8.x, moving back to 7.x.

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 7.x. Thanks!

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

Issue summary:View changes

fixed the original issue link