The following code looks wrong:

function field_group_info_groups($entity_type = NULL, $bundle = NULL, $view_mode = NULL, $reset = FALSE) {
  [..]
  if (!isset($entity_type)) {
    return $groups;
  }
  elseif (!isset($bundle) && isset($groups[$entity_type])) {
    return $groups[$entity_type];
  }
  elseif (!isset($view_mode) && isset($groups[$entity_type][$bundle])) {
    return $groups[$entity_type][$bundle];
  }
  elseif (isset($groups[$entity_type][$bundle][$view_mode])) {
    return $groups[$entity_type][$bundle][$view_mode];
  }
  return array();

E.g. if $bundle is NULL, but $groups[$entity_type] is missing, then this logic will move on to the next elseif() block. When instead, it should just return array().

Besides, it is good practice (e.g. recommended by PhpStorm EA Extended plugin (*)) to test for === NULL instead of isset, if we already know that the variable exists.
Using isset() can hide bugs, where we check a variable that does not exist.

Solution:

  if (NULL === $entity_type) {
    return $groups;
  }
  elseif (NULL === $bundle) {
    if (isset($groups[$entity_type])) {
      return $groups[$entity_type];
    }
  }
  elseif (NULL === $view_mode) {
    if (isset($groups[$entity_type][$bundle])) {
      return $groups[$entity_type][$bundle];
    }
  }
  else {
    if (isset($groups[$entity_type][$bundle][$view_mode])) {
      return $groups[$entity_type][$bundle][$view_mode];
    }
  }
  return array();

Also, some (again, the EA Extended plugin (*)) prefer array_key_exists() over isset() for clarity, to make it easier for an IDE to spot bugs.
This is a tiny bit slower, technically, but in this case it does not matter, this usually only happens once per request.

E.g. the following should work:

  foreach (array($entity_type, $bundle, $view_mode) as $key) {
    if (NULL === $key) {
      return $groups;
    }
    if (!array_key_exists($key, $groups)) {
      return array();
    }
    $groups = $groups[$key];
  }
  return $groups;

One aesthetic problem here could be that we reuse the variable $groups.
Personally I don't see this as a problem in this case.

Comments

donquixote created an issue. See original summary.

donquixote’s picture

Issue summary: View changes
donquixote’s picture

Issue summary: View changes