Fix field_info_fields()

Hm, field_info_fields() was modified as part of #560780: Add Image Field to image.module, which I couldn't review properly.
The signature is buggy at least. We talk about object (or entity) types, and bundles, but not about bundle types...

According to the function, it looks like this should in fact read $obj_type : 'node', 'user', 'comment'...

Doh, that's in fact a clash with #470242: Namespacing for bundle names. That patch changed the field_info_instances() signature to take ($obj_type, $bundle) instead of just ($bundle) previously, and that was not mirrored in #560780: Add Image Field to image.module.

So, in the current state of the function, the first arg of field_info_field() works as a $obj_type. No code currently calls field_info_fields() with that 1st arg, though.

The whole function needs refactoring though.

Subscribe. Just hit this particular road block.

I ran into this bug too and took a stab at a fix for it.

As yched said, it seems like when the arguments of field_info_instances() were changed to include $obj_type, this change was not carried over to field_info_fields().

My patch adds $obj_type as an optional argument for field_info_fields(). This allows field_info_fields() to return the fields of an object type (i.e. fields on all nodes) or of a bundle type (i.e. fields on article-type nodes).

The patch also updates the only two uses of field_info_fields() within Drupal core: in file_get_file_reference_count() and file_get_file_references().

This looks good to me, but would be nice to get sign off from yched. Marking it RTBC.

Well, IMO those 'limit by object type, or bundle, or field type, or several of the above' features don't belong in field_info_fields() to begin with.

At some point during #560780: Add Image Field to image.module I suggested that some helper function used to filter fields by field types might be merged with field_info_fields(), but adding more criteria is bloat. We're reading from a cached array of field definitions, not writing a pseudo query builder.
The current implementation has several flaws / holes, so if we go that route, we'd need a battery of tests to make sure we caught all combinations - I don't really volunteer for writing those ;-)

So I'd rather go back to where field_info_fields() was before the imagefield patch: just get the list of all active fields on the site. It's easy enough for field_info_fields() callers to filter the results by whatever criteria makes sense for them.
In the end, File field / image field only need a subset of the conditions (limit to a specific field, limit to a specific field type) in *one* function, file_get_file_reference_count(), so I moved that logic back to there.

Turns out field_info_fields(some conditions) was called in two places in file.module, dunno why I missed the 2nd on my initial search.
That patch fixes both places.

Marked #637870: field_info_fields() does not work as documented and #637342: Missing param in field_info_fields() as duplicates.

---scribe
sub

Moving solution candidate from duplicate #637870: field_info_fields() does not work as documented.

Maybe this is not the right way to go, but I'd at least like to know why, since this is the only proposed solution that actually provides the documented functionality, as opposed to removing functionality.

#6 might be better, now that I understand that bundles aren't actually supposed to exists across entities. My patch is admittedly a lot of extra weight for a purely theoretical usage.

#10 has the advantage of getting rid of ugly code like theFunction(NULL, NULL, $stuff) which is a win.

Perhaps a best-of-both-worlds would be something that supports the form field_info_fields(array('bundle_type' => 'article', 'entity_type' => 'node')) ?

So I'd be happy to see any of these in, but I don't like the idea of decreasing the usefulness of a tool if we don't need to.

from my #8:
if we go that route, we'd need a battery of tests to make sure we caught all combinations - I don't really volunteer for writing those ;-)

+++ modules/field/field.info.inc	19 Nov 2009 23:43:50 -0000
@@ -499,6 +503,27 @@ function field_info_fields($bundle_type 
+ * Returns an array of entity types that use a given bundle
+ *
+ * @param $bundle_type
+ *   The bundle type on which to filter the list of fields.
+ * @return
+ *   An array of entity type names.
+ */
+function field_info_bundle_entity($bundle_type) {

Makes no sense, a bundle doesn't appear in several entity types. They might have the same name, but that doesn't make them the same bundle.

I still stand by the optinion that field_info_fields() should be what it originally was. I'm sorry that I let this rot until post freeze and it appears we're removing features, but that 'filtering' feature just doesn't belong in there.

I'm on crack. Are you, too?

I agree that an info function shouldn't do filtering, although it would be nice to have a follow up patch to add such a utility function.

+++ modules/file/file.module	19 Nov 2009 01:33:37 -0000
@@ -857,36 +857,49 @@ function file_icon_map($file) {
+    // Support $field as 'field name'.
+    if (is_string($field)) {
+      $field = field_info_field($field);
+    }

Minor - Do we really need to support the field name? I personally don't like ambiguous arguments that can be either array or a string.

+++ modules/file/file.module	19 Nov 2009 01:33:37 -0000
@@ -857,36 +857,49 @@ function file_icon_map($file) {
+    if (empty($field_type) || $field['type'] == $field_type) {

Why are we checking empty($field_type) - is there a use case for that?

I'm on crack. Are you, too?

re #15

Do we really need to support the field name? I personally don't like ambiguous arguments that can be either array or a string.

I don't either, but that's how filefield / imagefield were coded, changing that would be a different issue for which quicksketch's opinion is needed.

Why are we checking empty($field_type) - is there a use case for that?

file_field_delete_file() calls file_get_file_reference_count() without a $field_type param. That's because you want to react to a file being deleted in all fields that refer to it, be it image or file field.

We are actually in needs work. As yched noted, this makes very little sense:

+    $entity_types = field_info_bundle_entity($bundle_type);
+    foreach ($entity_types AS $entity_type) {
+      $instances = field_info_instances($entity_type, $bundle_type);
+      $fields = array();
+      foreach ($instances as $field_name => $instance) {
+        $key = count($entity_types) > 1 ? $entity_type . "-" . $field_name : $field_name;
+        $fields[$key] = field_info_field($field_name);
+      }
+    }

Bundle names are not global, they depend on the object/entity type.

I agree that #10 is the way to go. The only minor issue I have with it is the API inconsistency (not introduced by this patch), between the $field argument of file_get_file_reference_count() (a string or an array) and file_get_file_references() (an array). We really need to fix those things, but this is not in the scope of this patch.

RTBCing #10.

Committed #10. Thanks all.

#21 is spam. Funny.