Problem/Motivation
Under certain circumstances the module files are not yet loaded.
This happens for example when invoking a hook inside the constructor of a http_middleware service; theses services are constructed very early as a dependency of http_kernel service.
A more concrete example is a middleware service using the entity_type.manager. Most of the times the entity type information is retrieved from cache (stored in the discovery cache bin). When this cache however is missing, hooks like hook_entity_type_build() and hook_entity_type_alter() need to be invoked at this early stage. The modules files however haven't been loaded yet, making ModuleHanlder::verifyImplementations() skip all the hook implementations (silently!). The entity definition cache is then incomplete resulting in various hard to debug (next to impossible) exceptions like #3031598: The "paragraph" entity type did not specify a translation handler. .
Steps to reproduce
Taken from this comment.
- Just uninstalled the dblog module from UI here /admin/modules/uninstall
- Then open any menu /admin/structure/menu/manage/[menu-name] and I see an error:
Uncaught PHP Exception Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException: "The "menu" entity type did not specify a "edit" form class." at /core/lib/Drupal/Core/Entity/EntityTypeManager.php line 211 - Then click to Flush views cache on another admin page (Flush all caches > Flush views cache via admin menu).
- After #3 I can see the error:
Uncaught PHP Exception Symfony\Component\Routing\Exception\RouteNotFoundException: "Route "entity.path_alias.collection" does not exist." at /core/lib/Drupal/Core/Routing/RouteProvider.php line 206
Why does it happen?
After #3, a lot of items were removed from router table from the database. You have to launch drush cr twice to fix such error.
Proposed resolution
If a hook is invoked before the modules being loaded, load all modules.
Remaining tasks
Update issue summary
Address #12 and #17
| Comment | File | Size | Author |
|---|
Issue fork drupal-3207813
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
casey commentedComment #3
bhumikavarshney commentedHi @casey ,
Patch applies clearly and works fine for me.
Now the modules files loaded easily.
Thanks.
Comment #4
sajiniantony commentedHi Casey,
I have applied the patch, the site worked fine. But at a later stage we are getting the same error The "paragraph" entity type did not specify a translation handler. in Drupal\Core\Entity\EntityTypeManager->getHandler().We are using Drupal 8.9.10 and paragraph 8.x-1.12. Also it is a multilingual site. We do have contents with paragraph but translation is not enabled as it does not support translations
Do you have any suggestions? Thanks
Comment #6
joel_osc commentedThank-you everyone for your work on this issue! I saw both the paragraph error and a problem where tokens were not working because the token hooks were skipped and token_module_implements_alter then would fail to add the core tokens that it provides like book, field, etc. Banged my head on it for a few hours until I found this issue and patch.
Comment #8
dmitriy.trt commentedWe experienced a very similar issue even with #2 patch applied. It turned out, the patch doesn't cover all the cases.
All of the following conditions must be met in order to trigger it with #2 patch applied:
hook_infocache entry exists,module_implementscache entry is missing,ModuleHandler::getImplementations()happens before all the modules are loaded.In this case the
ModuleHandler::buildImplementationInfo()starts the discovery without loading the module files. In our case it also resulted in empty list of implementations for thehook_entity_type_build()and with External Entity module it produced "The ... entity type does not exist." error. And since the list of hook implementations is then written to cache, this breaks the website with that error for everyone until the next cache rebuild.Worth mentioning that it was reproducible on a high traffic website during a cache rebuild executed from drush. Drush commands don't call the
ModuleHandler::writeCache(), since they don't dispatch thekernel.terminateevent. That seems to increase the chances and may be a separate bug, but it's minor. In this scenario it happened in 1 out of 3-5 cache rebuilds leaving the website broken. But even without that, it could be reproducible via multiple parallel requests, since thehook_infocache is written immediately after generation, while themodule_implementscache entry is only written at the end of the request and there is plenty of time for another request to get into that trap.Attaching patches with tests only and with the fix + tests. Hope that's fine to move the code of #2 to a different method.
I should explain the line removed from existing test. There is no actual reason why the test must load the module here. Actually, it looks like a workaround that hides the bug fixed by #2. Please let me know if that's not correct.
Comment #10
dmitriy.trt commentedThat's a test-only failure as expected, so setting "Needs review" back.
Comment #11
anybodyThank you @casey, this looks important and interesting as, as you wrote, it potentially might also have an impact on other issues like #3056633: It is not possible to uninstall a module that provides a filter plugin via config import.
Are there other scenarios you could imagine to also cause this?
Could you list some examples of modules which do this? Looking through our stack I found the following modules with a http_middleware service, unsure if they invoke hooks:
Comment #12
catchThis is a very old issue. The short answer is that middlewares shouldn't be invoking hooks, there might be a later kernel event listener that would allow you to achieve the same thing.
This might be a duplicate of #1905334: Only load all modules when a hook gets invoked which was trying to load hooks when a hook is invoked instead of at a particular point in bootstrap.
The other is to do #2237831: Allow module services to specify hooks, and deprecate procedural hooks (in Drupal 11 or 12 though).
Comment #13
catchComment #15
peterwcm commentedThank you @dmitriytrt for your patch. It was working fine with 9.3.x but it couldn't apply on 9.4.0 due to changes in ModuleHandlerTest I believe. getImplementations is also deprecated since 9.4.x so I have made some changes to the test.
Comment #16
dieterholvoet commentedI also encountered the issue after clearing caches through the admin interface. Some
hook_entity_type_buildimplementations were not called, causing errors indicating missing entity types. I found this issue after reporting my findings at #3107252: Missing plugin notice after every merge and cache clear, cache_discovery table. The latest patch fixes the issue for me.Comment #17
alexpottI think that #12 implies that this solution is not really the correct solution.
I think the issues around the caches getting written at different times is a real one and one that we should look to fix in a separate issue. The hook_info cache write should be written in \Drupal\Core\Extension\ModuleHandler::writeCache() and not in \Drupal\Core\Extension\ModuleHandler::getHookInfo().
Comment #19
thejimbirch commentedJust wanted to comment that I searched my code for the hooks mentioned in the initial description and the ctools_entity_mask implements hook_entity_type_alter. After uninstalling ctools_entity_mask my error went away.
Comment #20
heddnWe're seeing this on a handful of sites and I was pointed here by an associate. I'll apply the patch in #15 for now and report back if our random cache poison issues stop appearing.
Comment #21
heddnAlso of note, the 2 sites facing the issue are using chained fast cache (redis and memory) and are hosted on Pantheon.
Comment #22
heddnRandom cache poisoning is now decidedly fixed with the patch applied. I'm not sure what are the next steps here, so moving back to NR to get some feedback. As the patch fixes a very large problem on 2 active and large websites.
Comment #23
mrconnerton commentedUnderstanding that via #12 / #17 this patch might not be the final / "right way" to solve this issue, I wanted to add my feedback that the patch in #15 did resolve our eck issue @DieterHolvoet describes in #3107252-18: Missing plugin notice after every merge and cache clear, cache_discovery table and echo Lucas's comment in #22 that this could use eyes/feedback again to see what the next steps are.
Comment #24
joseph.olstadPatch 15 has test,
tests only patch fails as expected (#8),
patch with tests passes
patch reported to fix the issue
patch 15 applies cleanly on the latest HEAD of:
@TODO: figure out how to write a version of the patch to satisfy #17 and #12
Comment #25
smustgrave commentedIssue summary should be updated with steps to reproduce.
Added addressing #12 and #17 to remaining tasks.
Comment #26
solideogloria commentedA comment on this issue seems to have reproducible steps
https://www.drupal.org/project/pathauto/issues/3254939#comment-14389534
Comment #27
joseph.olstadpatch 15 had fuzz, removing fuzz to be able to try testing with newer versions.
straight up reroll, no interdiff, just fuzz removal.
Comment #28
joco_sp commented#27 worked for me also on core 9.5.5.
Thank you all for making that patch, because it solved a big issue :D If I see the error reappearing, I'll report it here.
Comment #29
solideogloria commentedWas unable to add a new view suddenly for some reason.
Patch #27 fixed the issue.
Comment #30
alexpottThis is a important patch that makes the ModuleHandler more robust. Looking at the code I'm pretty sure that we do not need to call
loadAll()frombuildImplementationInfo()because we already do areload()inbuildHookInfo(). I'm pretty use we only need the call inverifyImplementations(). I think we should be adding a middleware test that calls a hook prior to the regularloadAll()call from\Drupal\Core\DrupalKernel::preHandle().Comment #32
adamps commentedFYI I believe this caused #3367047: Caching bug enabling/disabling overrides. I found a fix which is to check
ModuleHandlerInterface::isLoaded(), and skip any further processing if FALSE.So yes this would be really useful to fix as a priority. As already noted, the resulting problems are pretty tricky to debug. The symptoms (to "randomly" skip calling hooks) are quite severe, potentially even security related.
Comment #33
berdirI'm unsure if we should support this. Yes, it makes it more robust, but at the cost of possibly unpredictable performance issues? Forcing modules to load in a middleware will basically eliminate the reason middlewares exist in the first place.. to have very fast pre-full-bootstrap logic, like page cache.
What about throwing an exception instead if we detect this (trying to invoke a hook before a full bootstrap), so it fails consistently and reliably when adding such a hook? If you do that then you know that you have to implement things differently. And if you really really want to do this, you could still call loadAll manually? For BC, we could add a deprecation message with the loadAll() fix as an intermediate step?
Comment #34
adamps commentedGood point. The currently behaviour is: run no hooks and cache the result. This seems clearly undesirable, however it's a good idea to think carefully about what is right.
We could try it, however It's pretty easy to trigger a hook - the call to invoke the hook could be buried within a function that we want to call. There might be places in existing middleware code that trigger a hook as a side-effect of their function. Or, like #32, places in ordinary module code that can "unexpectedly" run early in the bootstrap cycle.
We might need a way to disable the exception, i.e. run no hooks and don't cache the result.
Comment #35
berdirNot running the hooks does not work IMHO. We don't have a way to pass that information back, and caching happens elsewhere. One of the most common issues related to this is incomplete entity type definitions because build/alter hooks weren't invoked. Things will be completely unpredictable then. For example, information about bundle classes might be missing and then it's not the class you'd expect to get back in your code.
And yes, hooks are typically called several steps further down, for example the entity type thing, if you load a node in a middleware for example. But an exception will fail right there and then and the stacktrace will tell you exactly which middleware and line of code is the problem. Unlike now, where it will suddenly blow up in a completely different place, for example when editing a node with paragraphs.
Comment #36
adamps commented@Berdir, yes thanks it makes sense in general. However it seems to imply that middleware cannot do anything that triggers hooks (such as to load a node) because that would trigger the exception. Are you suggesting that this becomes a strict limitation of middleware? It could imply even that adding a new hook anytime in the future would be a BC-break because middleware might be using the code where the hook is being added. And when we say middleware, we actually mean anything that might run "early", for example implementations of
ConfigFactoryOverrideInterface.I expect that we would want a function that code can use to check if it is yet safe to use hooks.
So I was wondering if this might be too strict, and we could allow middleware to call a function to disable all hook exceptions (run no hooks and don't cache the result), or even just for a specific hook. However I can see that could be problematic because disabling some hooks could even have security implications.
Comment #37
berdirThere's no such thing as only loading one/some hooks, what's this about is loading the .module files of all modules so that hook discovery can figure out if a certain function exists or not.
And yes, middlewares should absolutely be as fast and slim as possible. They are specifically designed to run in very-early-bootstrap and use the decorator pattern, so they're always invoked including their dependencies. You should only use a middleware if there is a benefit to running before modules are loaded and the general bootstrap happens. For example page cache and the IP ban module, both benefit from being able to return a response as quickly as possible to achieve fast response times and minimal overhead if a certain IP attempts to send a massive amount of requests. If it doesn't need to be that fast or if you have quite a few dependencies, use a request event subscriber.
It's like hook_boot() in D7, that also only supported a limited set of API's.
For example, instead of using the config system in a middleware, you could dynamically set a parameter on it with a service provider, or only support configuration through container parameters or settings in the first place instead of config with a UI.
Comment #38
FleetAdmiralButter commentedWe had been running into an issue for some time on one of our Drupal instances where performing a cache-rebuild (via Drush) while the site was serving web traffic would result in a corrupt module_implements cache.
I managed to trace the problem down to something weird happening in Shield and chanced across https://www.drupal.org/project/shield/issues/3277210. Although the patch in that issue did resolve the cache corruption, we ultimately ended up going with https://www.drupal.org/files/issues/2023-03-10/3207813-27.patch which addresses the root cause of the issue rather than patching Shield.
Comment #39
andysipple commentedI was directed to this issue from https://www.drupal.org/project/pathauto/issues/3254939
We've been grappling with this error for some time now. It would trigger the White Screen of Death (WSOD) after deploying changes to the Pantheon live site using Redis or clearing the cache. Although it seems random, this error occurs most consistently in our high-traffic live site. We would fix it temporarily by uninstalling and installing a module or just clearing the cache several times until the WSOD clears up.
We applied the patch mentioned in #27.
No longer get the WSOD.
Comment #41
jeffreysmattsonAfter three weeks of banging our heads against the wall. With seemingly random site crashes that occurred only in production. With errors like this:
These errors were not only for the field_last_password_reset. They would error on all kinds of different fields that didn't have field storage, but actually did have field storage.
The site would become unavailable and a simple cache clear would resolve it... until a few hours later it would go down again.
This fixed it. Thank you!
I used #15 because we are still on Drupal 9.
Comment #42
joseph.olstadre: #41,
I haven't seen "field storage doesn't exist" in relation to password_policy, or if I did see it I may have massaged the configs to resolve it. I've seen other issues such hook password policy updates that break on anything other than mysql.
***EDIT***
ah, actually we're using a version of this patch so that's probably why I didn't see the "field storage doesn't exist" error.
Comment #43
joseph.olstadStill needing steps to reproduce. So far I see mention of the password_policy module in relation to this issue.
It would be nice to simplify the steps to reproduce in the wild.
"missing cache entry for hook implementations"
It would be nice to see what the performance cost of this solution is also. Looking at the code it seems like it would only be affecting a cold cache (rebuild).
Comment #44
catch@joseph.olstad password policy is just an example in #41, not the cause of the issue or the only symptom.
This bug gets triggered (usually) when a middleware (which runs before modules are loaded) ends up invoking hooks, this is pretty well documented as the issue and there are lots of code paths that can trigger it. I still think #1905334: Only load all modules when a hook gets invoked is closer to the correct solution here - i.e. always load modules when hooks are invoked and don't load them until they are.
Comment #45
joseph.olstadok thanks for that, ya the other approach to this issue at
#1905334: Only load all modules when a hook gets invoked
does sound better!
Comment #46
berdir> I still think #1905334: Only load all modules when a hook gets invoked is closer to the correct solution here - i.e. always load modules when hooks are invoked and don't load them until they are.
The concern I have with that issue is that it becomes very unpredictable. Yes, triggering a hook in a middleware will work, but it will also cause a considerable performance regression for sites that happen to use such a module, without any indication about that unless they do some profiling.
The current cases likely only trigger such hooks in edge cases or they would consistently fail and not work at all. But with this, you could for example load an entity in a middleware. Middlewares should be as slim as possible with as few dependencies as possible otherwise you're better off doing a request subscriber.
Comment #47
catchI think that might be what's happening with some of these reports - loading an entity, or especially config entity, that's usually cached, then suddenly it's not cached and the middleware loads it, triggering the bug.
Comment #48
casey commentedI think I agree with Berdir that the right solution is to throw an exception (when hooks are invoked before all modules are loaded).
Maybe we should only do this for D11, and fix it for D10 by loading the modules and logging a warning. This way we don't break existing sites that do trigger hooks in middlewares. But we will get some telemetry on which modules do need changes.
Comment #49
rakshith.thotada commentedI see if this is cache issue during middleware service request -
Comment #50
rakshith.thotada commentedAdding patch for 10.1.x version.
Comment #51
smustgrave commentedWas previously tagged for issue summary which still needs to happen.
Current development branch is 11.x
If you are going to upload patches please include interdiffs
Comment #52
nitesh624Is there any patch available for drupal 10.0.x release? We are also running into same problem
Comment #53
nitesh624I tried patch from #27 but did not resolve issue
Comment #54
solideogloria commentedI'm using patch #27 on Drupal 10.1. Everything seems to work.
Comment #55
nitesh624For me this failing during
blt synccommand. Which actually pull down the production database to local. and it runs couple of drush commanddrush sql-sync-> this ran fine without any issue
drush cr -> Here the error came "The "paragraph" entity type did not specify a translation handler."
We have the multilingual enable on site and paragraph field also available on node, which allowed to be translated via
content_translationmoduleComment #56
bobthebuilder commentedDrupal 10.2.3. Paragraphs 8.x-1.17. Paragraphs Library 8.x-1.17.
I'm still receiving the error message when trying to create a Paragraph. Patch #27 does not fix the issue and Patch #50 can no longer be applied with Drupal 10.2.3. Patch #50 did work for me pre-Drupal 10.2.3.
Comment #57
solideogloria commentedThis issue needs a MR to be created instead of using patch files.
Comment #58
joseph.olstadI re-read #44 through 47, continue on.
As discussed in #44 through #47, prioritize effort to 1905334 instead.#1905334: Only load all modules when a hook gets invoked
Comment #60
solideogloria commentedI created a MR from #27, since that's what most people said is working. #50 and #51 seems to do something entirely different, and it also doesn't have any tests.
Comment #61
quimicPatch #27 can no longer be applied on Drupal 10.2.5
Comment #62
jsst commentedFor us too, patch #15 brought an end to a long running terror bug, causing spontaneous crashes due to this issue. Our site was running fine for five years, but somewhere during the upgrade to 10.2 the random cache corruption started to hit. Our http middlewares don't do anything special, something in Drupal Core made this bug worse. The bug is hard to debug because it seems to depend on concurrent usage of the site and clearing the cache can fix the bug (after running it up to 10 times) but also cause it.
So we have a major bug that is hard to debug and causes a site to go completely offline, with an unreliable fix (drush cr until it works). Seems to me we should be merging #15 in Drupal 10.2 as soon as possible.
Comment #63
solideogloria commentedWell, we shouldn't be merging a patch at all. We should merge the merge request.
The MR targets 11.x, and it could also be cherry-picked to earlier branches.
Comment #64
solideogloria commentedComment #65
solideogloria commentedI added a steps to reproduce in the issue summary that I copied from a comment. The comment said that the patches here fixed the issue, so it's at least one example.
However, there may be more ways to reproduce the problem, and the example I added requires another module. It'd be helpful if anyone can reliably reproduce the problem in another way, such that a test can be written for it.
Comment #66
nicxvan commentedShould we be using the patch in 27 as the base? I think it seems the suggestion was to throw an exception if invoking hooks in middleware?
Comment #67
solideogloria commentedAn exception is already thrown and is the cause of the issue in the first place. How would throwing an exception fix the issue? Wouldn't it just break in a different place?
The patch the MR is based on fixes the issue because the information needed is already there, and it is helpful for anyone patching their code, like me.
If you want to change the approach, you'll need to open a new MR.
Comment #68
nicxvan commentedI'm not sure to be honest, I was reading comments 33+ which came after the patch in 27 saying that the solution in 27 could have unpredictable performance issues and the exception description.
I know the later patches don't have tests, but I think the concerns around comment 33 still need to be addressed.
Comment #69
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 #70
joegl commentedWe cut a patch from the merge request and applied it to a Drupal 10.2.7 multisite stack with about 100+ sites. During deployment about 20% of sites were losing home page content and content on a few other pages inconsistently with no determinable pattern. We do not have Memcache enabled. The patch resolved the issues we were having.
In logs, we noticed a few errors pop up during these deployments and perusing the issues queues they all seemed to point back to this issue, including:
I understand the methodology here isn't ideal, but it was a critical issue to resolve for us.
Comment #71
joseph.olstad@joegl, please try the solution in #1905334: Only load all modules when a hook gets invoked
It is more likely to go forward than this one.
Comment #72
k.wiktorowicz commentedPatch from the comment https://www.drupal.org/project/drupal/issues/3207813#comment-15332466 working with 10.3.x version.
Comment #73
nicasso commentedHi,
I updated patch #27 or at least the essence of it, so it applies for 10.3.5.
Comment #74
nicxvan commentedCan someone confirm if this still happens on 11.1, that method no longer even exists.
Comment #75
akoepke commentedHave applied the patch from #73 in 10.3.10 and it has resolved this issue for us.
I noticed this after enabling the IP Login module which uses Entity Type Manager in a middleware service: #3490122: Exceptions caused by middleware service using the entity_type.manager
Comment #76
hanan alasariWe had the same issue on drupal 10.4.3 , but could not figure out which module was causing the issue, patch #73 worked for me, thx.
Comment #77
berdirI'm fairly certain this is resolved in 11.1+, yes. Moving back to 10.5.x, there's still an option to commit there as a workaround. Instead of rerolling patches, please createa merge request, specifically against 10.5.x.
Comment #80
liam morlandI have created a new merge request targeting 10.5.x which is a cherry-pick of the commit in the existing merge request. All tests are passing.
Comment #81
liam morlandCurrent state of !12355.
Comment #84
nicxvan commentedHiding the files and other MR. Can't review right now though.
Can someone confirm the next steps comments have been addressed and update the issue summary please?
Comment #85
vlyalko commented#72 worked and applied perfectly for 10.2.10 Core version. Thank you for working on this issue!
Comment #86
igorgoncalves commentedJust tried to reproduce the error following the steps at summary but didnt work.
With simplytest, and Drupal 10.5.1-dev
Needs a summary update indeed.
If i find the way, i will edit the summary.
Comment #87
nicxvan commentedI'm not sure what would have changed between 10.5 and 10.2 to fix this.
I would expect this to be fixed by 11.1
Postponing for steps to reproduce.
Comment #88
gregn0r commentedRunning a site split using Domain, utilising Memcache we were digging though a lot of random errors and services to try and resolve caching issues with a large live site.
Digging deeper into what was happening we understood the combination of modules / configuration we had were invoking Drupal hooks early in the page load and that modules had not been loaded. The module_implements cache gets built incorrectly and you end up with an incomplete site cache state which can result in a multitude of errors largely revolving around missing entitles or field definitions
The issue was repeatable when the cache clear was particular slow i.e. on a large live site or done while using xDebug talking to an IDE locally, potentially triggering race conditions on the rebuild of the cache
Applying the MR patch on our 10.5 site resolved the issue, many thanks
Comment #89
berdirYes, I'm certain this is still an issue.
Comment #90
smustgrave commentedSeems there are some remaining tasks in the summary
Comment #91
himynameisseb commentedWe were experiencing many different errors following cache clearing on a site under load, with the most common errors being:
- RouteNotFoundException: Route "entity.node.latest_version" does not exist.
- Error: Call to a member function getFieldStorageDefinition() on null in scheduler_content_moderation_integration_scheduler_hide_publish_date()
- InputBag::get(): Argument #1 ($key) must be of type string, null given
Putting the site in maintenance mode and doing a couple more cache clears kicked the site back in to life.
Upon digging, discovered that it was caused by the same issue that this patch fixes.
Tested and released on a site running Drupal 10.4 (10.4.8 specifically).
We have experienced this sort of "caching strangeness" in the past. I cannot say for sure if this was the cause in the past, but it could well be. It would be great to see this merged in to a future release if others experiences matches ours!
Many thanks!
Comment #92
kevinquillen commentedThis seems like it needs a reroll for 11.2.
Comment #93
berdir> This seems like it needs a reroll for 11.2.
No, it does not, because this problem does not happen anymore on 11.1+, this is a D10 only issue at this point.
Comment #94
nicxvan commentedComment #95
eric_a commentedOn Drupal 11.1 Rest API & JSON API 500 errors on authenticated requests with :
Error: Call to undefined function \ckeditor5_module_implements_alter() in Drupal\Core\Extension\ModuleHandler->alter()Issue went away when hacking in this:
So maybe same underlying issue?
Comment #96
nicxvan commentedInteresting, this might still be an issue then! 11.2 won't have this, but only because we removed all hook_module_implements_alter implementations.
Comment #97
joseph.olstad*** EDIT ***
Please see my next comment. modules_implements_alter is indeed still included in 11.2.x.
*** END EDIT ***
@nicxvan, the API documentation for 11.x mentions that this hook will be removed in 12.0. It's not very clear what 11.2.x has to do with this. What exactly is changing in D11.2.x that would mean "won't have this?".
Example I just pulled up from one of my clients that has search_api and facets along with a custom bit of helper code in a module designed to combine two facet options as one.
My commit message for this module was squashed so I'm not even quite sure why I had the module_implements_alter however it's there and the module works as designed currently. The commit message was: "Combine article and landing_page into one single Type selection checkbox in the Content Type facet." , of course the module code is more than just the module_implements_alter.
@nicxvan, since you mentioned 11.2 won't have this, can you please explain why the change to 11.2.x that you just mentioned is not mentioned in the api documentation? Where's the change record for this?
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension...
This is a very muddy explanation. Versions prior to Drupal 11.2.0. What exactly happens in Drupal 11.2.0? Not deprecated. "won't have this"?
Also, is there some bit of reassurance that can be added to the api documentation saying that "if you change your approach to xyz that abc will continue functioning"? in this case the hook we're dealing with "search_api_query_alter" has not been changed and is a contrib hook.
This particular client code I reference is being used in D11.1.8. D11.2.x is right around the corner and since you "just" mentioned "won't have this", maybe when my client upgrades to D11.2.x, this client won't have combined facet options for content type either?
Comment #98
joseph.olstad@nicxvan , I just looked in the 11.2.x branch, 64 lines of code or comments with "modules_implements_alter"
In 11.1.x 46 lines of code mention modules_implements_alter
What exactly do you mean by 11.2.x "won't have this"?
Comment #99
joseph.olstad@nicxvan, I found your document, a reference to this should probably be included in all of the hooks documented in the 11.x api
https://nlighteneddevelopment.com/hook-timeline/
Comment #100
nicxvan commentedhook_module_implements_alter is still supported in 11.2 but will be removed in 12.
However drupal core has no implementations of hook_module_implements_alter after 11.2 so ckeditor5_module_implements_alter won't throw an error. That is all I meant.
The article you found links to the change record deprecating hook_module_implements_alter.
Comment #101
steven jones commentedWe're seeing this issue too, in our case, it was a combination of using the shield module, and key module (with key overrides) triggering this issue for us.
Bizarrely enabling the dblog module sorted the issue, as did the patch from #81.
Comment #102
mkdok commentedI can also confirm that the issue still occurs on 11.2.4 and shield module enabled.
Comment #103
catchMoving this back to 11.x. However hook_module_implements_alter() won't be in 12.x at all (although most other procedural hooks will be eventually deprecated for removal in 13.x, not 12.x).
Comment #104
nicxvan commented@mkdok can you add some more steps to reproduce this based on your comment in the issue summary so we can work on this?
Comment #105
herved commentedI believe I am facing this bug on my project.
This happens during deployment in the config:import phase, when there are both modules to be uninstalled and installed at the same time.
I did manage to reproduce on clean 11.x with demo_umami and devel
Steps (I use ddev):
- ddev drush si demo_umami -y
- ddev drush cex -y
- Now go to sites/default/files/sync/core.extension.yml:40 and replace
page_cache: 0withdevel: 0- ddev composer require --dev drupal/devel
- ddev drush deploy -y
Stacktrace: https://gist.github.com/vever001/a80414b43d317d84e1d1aacc91cbb895
It seems to involve
\Drupal\language\EventSubscriber\LanguageRequestSubscriber::onContainerInitializeSubrequestFinished.Maybe there should be a check to
\Drupal::isConfigSyncing()somewhere?But why is it attempting to run devel_entity_type_alter while devel is not even installed yet.
Comment #106
berdir@herved: I'm pretty sure that's a new and different issue than what this is about. Can you create a new issue? It would be helpful to know if this happens in 11.1 or only 11.2. It very likely doesn't happen on 11.0.
And no, this doesn't have anything to do with config syncing, this isn't about changing the container. This happens in the middle of installing the devel module, when the container is built, it finds the hooks, but the module file is only loaded afterwards. And yes, invoking hooks as part of a container rebuild is more than just a little bit scary but I don't think that's new.
Comment #107
herved commentedThank you @Bedir, indeed it looks like a different issue but possibly with the same root cause?
I created #3549397: [regression] Uninstalling and installing modules during config:import can lead to fatal errors. Would it make sense to add a check to
is_callable($callable)inModuleHandler::getFlatHookListeners?Comment #108
eric_a commentedThis specific error went away after updating to Drupal 11.2, as expected. But the underlying problem is still there. It just manifests itself at another point:
Error: Call to undefined function \field_group_module_implements_alter() in Drupal\Core\Extension\ModuleHandler->alter()Comment #109
eric_a commentedSo the common thing that I see is the ordering of implementations of hook_form_alter() with hook_module_implements_alter(). Some specifically move the implementation to the end.
With ckeditor5 not implementing this anymore, the problem surfaces with field_group. I wonder: if field_group would move to OO, could then webform < 6.3.0-beta5 hit this...
Comment #110
berdirCan you provide a full backtrace of how you hit this error?
Comment #111
anybody@eric_a feel free to create an issue and a MR over at field_group. I'd be happy to merge it, once RTBC'd. Still I'm not sure if it will be the "final" fix. Often the module shown in the error isn't really causing it, just affected by it. At least we had that experience.
Thanks!
Comment #112
eric_a commentedComment #113
berdirOk, shield module again, as usual.
Loading entities in a middleware will *maybe* work reliable in D13 with the concept of loading modules gone, but even then it might have side effects. https://git.drupalcode.org/project/shield/-/blob/8.x-1.x/src/ShieldMiddl... is also really bad in regards of performance. It loads a ton of dependencies and basically undoes all the improvements that went into https://www.drupal.org/node/3538740 in 11.3.
If does stuff like then it should be responsible to properly bootstrap Drupal.
I understand it needs to do that to run before page cache, but that comes at a serious hit to performance, you should at least make sure to not have that enabled on production and I'd recommend using an alternative way outside of Drupal to protect dev environments.
Comment #114
nicxvan commentedCreated an issue in shield to track this: #3561355: Properly bootstrap drupal in the middeleware with oop hooks
Comment #115
luke.leberAre there other enabling criteria other than middleware layers that may cause this condition?
Our hosting provider (Acquia) refers to this issue as a root cause of the symptom outlined in #3277728: Call to undefined method isLayoutBuilderEnabled(), but I've audited our entire code-base numerous times and have confirmed there isn't any code at play that does anything remotely related to invoking inappropriate things in constructors.
If it helps at all, it appears that an out of memory error occurred just prior to the last time we've recorded this behavior.
Comment #117
marttir commentedIf this is indeed still an issue in 11.2.x, perhaps it is still an issue in 11.3.x as well?
In any case, this needs a reroll for 11.x.
Comment #118
luke.leberHello all,
This comment is one of the results from making S.D.O. issue #184413 public.
I've added a functional test case that should prove this is an issue on Drupal 10. The chain of event goes...
1. The discovery cache is empty
2. In the next request, an exception is thrown from the early kernel bootstrapping
3. A logger object is instantiated by
errors.inc, which invokes discovery before all modules are loadedIn the wild, this can happen with a combination of core modules and certain popular contributed modules:
syslogandkey.1. Layout builder is enabled
2. The discovery cache is empty (which on Acquia hosting, can happen randomly as the platform cycles memcached k8s pods out!)
3. In the next request, an exception is thrown from the early kernel bootstrapping
4. A syslog object is instantiated by
errors.inc, which calls the config factory service5. The config factory service dispatches the appropriate API events to discover and apply config overrides
6. The key module implements a config override which invokes entity type discovery via the entity type manager
7. The entity type cache item is corrupted and has the
\Drupal\Core\Entity\EntityViewDisplayclass set and not the layout builder flavor8. Subsequent requests show no layout builder content on pages until the discovery cache is flushed and can rebuild appropriately!
Comment #121
joegl commentedWe recently upgraded from 10.5.x to 11.2.x and are using the Shield module. I wanted to quickly provide an update.
We had previously used a patch from the merge request for 10.5.x to resolve the issue. However, there is no longer a merge request or patch for 11.2.x, and we removed it. Unfortunately, the error popped up again:
TypeError: Drupal\Core\Entity\EntityTypeManager::Drupal\Core\Entity\{closure}(): Argument #1 ($hook) must be of type callable, string given, called in /var/www/html/docroot/core/lib/Drupal/Core/Extension/ModuleHandler.php on line 357 in /var/www/html/docroot/core/lib/Drupal/Core/Entity/EntityTypeManager.php on line 117Based on the comments and guidance in related issues, while it seems a core fix would be desirable, the preference now is to improve the middleware in modules to not make these calls.
We applied a patch from the most recent merge request for the Shield issue: https://www.drupal.org/project/shield/issues/3277210. This is a one-liner that adds the following to the middleware:
It's clearly not a true fix and is non-optimal but it does clean up the error on our end.