If a request hits on a stale hook registry (during a deploy) i.e. the code on disk no longer provides an implementation the registry lists, CallableResolver throws. ModuleHandler::alter() reaches this code for every alter hook, so the whole site fatals. Similar situation arises in ThemeManager::alterForTheme().
ModuleHandler::getHookImplementationList() resolves every identifier in it with no guard:
// core/lib/Drupal/Core/Extension/ModuleHandler.php
foreach ($hook_list as $identifier => $module) {
if (isset($this->moduleList[$module]) || $module === 'core') {
$listeners[] = $this->callableResolver->getCallableFromDefinition($identifier);
$modules[] = $module;
}
}
Again. similar in: ThemeManager::getImplementationsForTheme().
Drupal 10 tolerated this: ModuleHandler::verifyImplementations() dropped implementations whose function had gone and carried on.
1. a procedural implementation is deleted
2. a hook class is removed
3. a hook method is renamed
1. add a hook_page_attachments_alter to any module
2. Rebuild, so the registry records it: drush cr
3. delete the hook (don't rebuild registry)
4. request any page
5. observe:
THROWN: InvalidArgumentException: Class "XXX_page_attachments_alter" does not exist.
at /var/www/html/docroot/core/lib/Drupal/Core/DependencyInjection/ClassResolver.php:32
Catch the resolver's InvalidArgumentException, skip that implementation and warn.
If the missing implementation is a hook_query_alter() that adds an access restriction, the site stays up but serves data it should not.
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 !17072
Comments
Comment #3
nicxvan commentedHi, thanks for the report.
Issues for Drupal core should be targeted to the 'main' branch, our primary development branch. Changes are made on the main branch first, and are then back ported as needed according to the Core change policies. The version the problem was discovered on should be stated in the issue summary Problem/Motivation section. Thanks.
Also, the title should be a description of what is being fixed or improved. The title is used as the git commit message so it should be meaningful and concise. See List of issue fields.
Finally, the issue summary could use some formatting so it is easier for reviewers to read. It looks like it is Markdown but should be HTML and is rather verbose. In fact, this reads like prose from an AI. If so, in Drupal core, we would like concise summaries that are in the reporter's own words. Of course, giving allowance for anyone who is not a native English speaker. See the orange warning box in the policy on the use of AI when contributing to Drupal.
Also i haven't read through the full issue summary, but search for duplicates as well please.
Comment #4
paranojik commentedComment #5
paranojik commentedComment #6
nicxvan commentedThank you for the cleanup!
Comment #7
paranojik commentedComment #8
ironnuts commentedHave reviewed the code, IS and comments. Added comments to the MR. MR is breaking due to cspell. Need to get pipeline green then can run the test-only test to check the test coverage.
Comment #9
ironnuts commentedComment #10
ironnuts commentedparanojik please declare it if you are using AI. It is a requirement.
Comment #11
ironnuts commentedPipeline is now green. Here is the test-only https://git.drupalcode.org/issue/drupal-3622228/-/jobs/12136101. There are 14 test failures. I think that is what should be output by that test. It is a bit confusing because there are errors about the new definitionExists() method not being found (because it is in the fix code that test-only excludes).
Comment #12
ironnuts commentedCode comment resolved. RTBTC.
Comment #13
ironnuts commentedComment #14
nicxvan commentedThank you guys, I'm going to put this in pmni because I'd like to take a careful review, there are a few related issues, but I can't look until early next week.
I'm going to assign it to myself too.
Comment #15
nicxvan commentedThis might be a better status with the issue tag, I'll keep this assigned to myself.
Comment #16
nicxvan commentedThis is a duplicate of #3495051: ModuleHandler::invokeAllWith() does not check listener is actually callable at the moment
Usually we close the newer issue, but this has an actual MR.
I took a quick look, I'm not sure a trait is the way to go? Why was that chosen over just a try catch?
Comment #17
ironnuts commentedThe steps to reproduce:
But drupal docs state that that hook is removed in 12.0.
Comment #18
paranojik commentedComment #19
paranojik commented@oily, thanks for the remark. I started with a 11.x patch and failed to check after re-rolling on main. Fixed the description.
@nicxvan:
A similar situation arises in
ThemeManager. I updated the ticket description.Comment #20
nicxvan commentedOne of the things that makes this really tricky is we don't necessarily know if the hook is safe to skip or not.
For example many preprocess hooks would be perfectly fine to just silently skip, but an entity presave hook may be destructive if it skips, a WSOD may be preferred.
Looking at
verifyImplementationsit doesn't seem to go as far as claimed, it checks once per request, not at call time.I think #3616465: Potential race condition in container rebuilds might be a better overall solution.
Also calling module load for every procedural implementation is not something we want to do.
Comment #21
paranojik commentedThanks @nicxvan.
Agreed we can't tell per hook whether skipping is safe. But how to we reconcile with the fact the D10 allowed this?
https://git.drupalcode.org/project/drupal/-/blob/10.5.12/core/lib/Drupal...
Couldn't find it discussed in #3442009: OOP hooks using attributes and event dispatcher.
Fair, I meant it skipped rather than WSOD. The latest MR also works per hook per request.
#3616465 looks complementary to me. It prevents stale writes, but falls back to the core version when deployment_identifier isn't set, so contrib-only deploys aren't covered. And once the registry is stale, it still fatals.
The per-implementation module load is removed.
Also, following @gabesullice, resolution now runs first and the staleness check only runs when it throws. I also removed the trait.
Comment #22
smustgrave commentedClosed #3495051: ModuleHandler::invokeAllWith() does not check listener is actually callable at the moment as a duplicate.
@nicxvan as submaintainer this would need your approval
Comment #23
nicxvan commentedActually let's follow our normal policy, I'll close this one since it's much newer and has fewer followers, @paranojik I've moved your MR over there so we don't lose your work, please join the discussion here: #3495051: ModuleHandler::invokeAllWith() does not check listener is actually callable at the moment