When drupal_check_profile() checks installation requirements, it doesn't check the type of the values returned from hook_requirements implementations before sending them to array_merge(). This results in an error if an implementation doesn't return a value for the 'install' phase.
e.g.
function mymodule_requirements($phase) {
if ($phase !== 'runtime') {
return;
}
return [
// ...runtime requirements...
];
}
The current check:
if (function_exists($function)) {
$requirements = array_merge($requirements, $function('install'));
}
For comparison, ModuleHandler::invokeAll() only merges if isset() passes:
if (isset($result) && is_array($result)) {
$return = NestedArray::mergeDeep($return, $result);
}
elseif (isset($result)) {
$return[] = $result;
}
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | drupal-2942014-2-drupal-check-profile.patch | 575 bytes | gapple |
Issue fork drupal-2942014
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
gappleThis patch allows
hook_requirements('install')implementations to returnnullwithout causing an error.Comment #3
joelpittetIMO it's nice when we know to always expect a single type of value and don't have to put more code to deal with those. The return value from that hook is always an array so I'd rather keeping what we have and the fix you did in the contrib module is the way to go. Others may disagree...
Comment #4
joachim commentedThe documented return value of hook_requirements() is an array.
So currently, this is a wontfix, as @joelpittet says.
If we want to allow a return of NULL, then we need to change more than this. We need to change:
- documentation
- everywhere that invokes the hook
Comment #12
lendudeAs pointed out in #3 and #4 the current documented return type is an array, so this would not be a bug. Moving to a feature request, but I agree with #3 and #4 that I'm not sure we should support this,
Comment #13
gappleMy justification is that currently
hook_requirements('runtime')is fine with implementations returningNULL, due to the behaviour ofModuleHandler::invokeAll(), whilehook_requirements('install')is not because it handles calling implementations slightly differently.I could settle for improving the documentation to explicitly specify that an empty array must be returned if a phase does not have any requirements. I think ideally a better exception would be thrown so that it's easier to diagnose and resolve the issue if someone attempts the early return pattern like I did (so they know to
return [];without looking at core's code).Comment #14
gapple(I think there's potentially an argument for separating
hook_requirements('runtime')to a separatehook_status()or such, since 'install' and 'runtime' execute with different access to Drupal APIs, and while 'install' and 'update' can halt execution of their respective operations, 'runtime' is only able to display information on the site status page)Comment #15
gappleLooking at the code for other invocations of
hook_requirements,install.inc:drupal_check_module()already has a check against the returned value's type:update & runtime use invokeAll which is fine NULL:
update.inc:update_check_requirements():SystemManager::listRequirements():