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
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
oily 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
oily commentedComment #10
oily commentedparanojik please declare it if you are using AI. It is a requirement.
Comment #11
oily 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
oily commentedCode comment resolved. RTBTC.
Comment #13
oily 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
oily 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.