Sometimes you want to invoke a hook for a single module. Currently you need to use invoke which restricts to one implementation or a really awkward invokeAllWith:
$requirements = [];
\Drupal::moduleHandler()->invokeAllWith('runtime_requirements', function (callable $hook, string $module) use (&$requirements) {
if ($module === 'system') {
$requirements = array_merge($requirements, $hook());
}
});
return $requirements;
Add two hooks in one module and execute them with invoke()
Add a check for array if there are more than one listener, and merge them before returning.
Review
| Comment | File | Size | Author |
|---|
Show commandsStart 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:
changes, plain diff MR !15953
Comments
Comment #2
berdirQuoting myself from https://git.drupalcode.org/project/drupal/-/merge_requests/15190:
> (I think my recommendation would be to only throw an exception when there are results that can't be merged. Meaning anything except array and null/void, but, separate issue)
In other words, we extend invoke() to support multiple callbacks as long as they don't return results that we can't merge.
I think the current situation is problematic, because you can write code that works until an arbitrary module decides to add more than one hook implementation and then it explodes. ultimate_cron for example is struggling with that right now and I'll need to adopt the pattern in the issue summary there I think.
There are still edge cases, but I think hooks that return something that isn't an array is pretty rare.
Comment #4
nicxvan commentedComment #5
nicxvan commentedComment #6
smustgrave commented1 small comment for consideration.
Comment #7
nicxvan commentedI addressed your feedback I think.
Comment #8
smustgrave commentedThanks for humoring me. No objection to the rest of the changes.
Comment #9
catchCouple of comments on the MR.
Comment #10
nicxvan commentedI updated the comments and added some test coverage for objects and ArrayObject.
Comment #11
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #12
nicxvan commentedComment #13
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #14
nicxvan commentedRebased
Comment #15
smustgrave commentedDoes this need a CR?
Comment #16
berdirNot sure, no code needs to be adjusted for this, it will just work now in cases where it can work.
It means that I can close #3501360: Multiple crons task should be possible in the same module as that's no longer a problem for example.
I tried adjusting the title a bit. We're not introducing a new API here, we just allow the existing invoke() to work in more cases than it did before. We could use the CR to explain this, but we already have some adjustments on the interface docs, for which I just added a suggestion to update it.
Comment #17
nicxvan commentedThanks! I addressed your feedback and updated the comment.
Comment #18
berdirBack to RTBC then.
Comment #19
amateescu commentedPosted a comment on the MR. That point might've been discussed and agreed upon already and I haven't seen it, so leaving at RTBC for now.
Comment #22
amateescu commentedCommitted and pushed 1b4da6d to main and bdf4b395e8c to 11.x. Thanks!