While un-common in core, the following pattern is used a lot in the wild:

if ($items = field_get_items($entity, $field_name, $langcode)) {
  foreach ($items as $item) {
    ...
  }
}

So couldn't we just always return an array?

-  return isset($entity->{$field_name}[$langcode]) ? $entity->{$field_name}[$langcode] : FALSE;
+  return isset($entity->{$field_name}[$langcode]) ? $entity->{$field_name}[$langcode] : array();

So in core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/views/argument_default/Tid.php

$items = field_get_items($node, $name);
if (is_array($items)) {
  foreach (field_get_items($node, $name) as $item) {
    $taxonomy[$item['tid']] = $field_info['settings']['allowed_values'][0]['vocabulary'];
  }
}

would become

foreach (field_get_items($node, $name) as $item) {
  $taxonomy[$item['tid']] = $field_info['settings']['allowed_values'][0]['vocabulary'];
}
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Alan D.’s picture

i.e are there any known PHP bugs that result in array() != FALSE

Dave Reid’s picture

Status: Needs review » Reviewed & tested by the community

This is such a DXWTF in Drupal 7. Please please please let's have this change.

Crell’s picture

array() == FALSE, but array() !== FALSE. In all common cases I can think of, it should be safe.

This is such a simple but such a major improvement. +1 on RTBC!

tim.plunkett’s picture

Issue tags: +Needs backport to D7

/me crosses fingers

Crell’s picture

Issue tags: +Quick fix

To get this in faster.

amateescu’s picture

You can call it a quick fix, sure, but it's still an API change for D7. And the doxygen block needs to be updated as well.

fietserwin’s picture

+1 for the change and RTBC, but I'm not sure about the backport. I had a look at places where I use it and in most cases I check for $result !== FALSE. So backporting that, would break lots of my code. IMO, there's no need to introduce such a small but nasty API change.

Alan D.’s picture

Second no backport to D7. Too great of risk, I have seen the redundant !== FALSE in a few places now. albeit harmless in the code that I have seen! Removed tag.

Sorry missed the doco.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal-1927884-7-return-array-from-field_get_items.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
1.02 KB
1.87 KB

Test failure is unrelated - it at least passed locally. Changed Tid.php as mentioned in the issue summary.
I would almost argue that this is not really a feature request (although we'd need a change notice)

Dave Reid’s picture

Status: Needs review » Reviewed & tested by the community

Ok this looks better, includes the docs change, and also cleans the use up in core. Back to RTBC.

Alan D.’s picture

A preemptive strike for the change notice :)

Summary

Updates to field_get_items() so that it always returns an array.

Examples

Checking that a field contains values using field_get_items().

In both Drupal 7 and 8, it is safe to directly use the returned values:

if (field_get_items($node, $name)) {
  $classes[] = 'has-value';
}

Drupal 7
Developers can check the returned type to see if the field is empty (this is not recommended):

$items = field_get_items($node, $name);
$has_values = ($items !== FALSE);
$has_values = is_array($items);

Drupal 8
The returned type is always an array, so type checking can not be used.

Accessing field items.

The following example was taken from the Taxonomy module in core.

Drupal 7
Since FALSE may be returned by field_get_items(), you must always check the results before looping through the array:

$items = field_get_items($node, $name);
if (is_array($items)) {
  foreach (field_get_items($node, $name) as $item) {
    $taxonomy[$item['tid']] = $field_info['settings']['allowed_values'][0]['vocabulary'];
  }
}

Drupal 8
You can directly iterate through the returned values of field_get_items().

foreach (field_get_items($node, $name) as $item) {
  $taxonomy[$item['tid']] = $field_info['settings']['allowed_values'][0]['vocabulary'];
}
webchick’s picture

Category: feature » task
Status: Reviewed & tested by the community » Fixed

I don't see this as a feature; just a clean-up task, which is fine since that's the phase of the release we're in.

And that's much nicer, thanks!

Committed and pushed to 8.x.

I copied/pasted Alan D.'s change notice text to http://drupal.org/node/1957218, so hopefully we're good to go marking this fixed, since it sounds like a D7 backport is too risky.

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