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
Comment #2
donquixote CreditAttribution: donquixote as a volunteer commentedComment #3
donquixote CreditAttribution: donquixote as a volunteer commented