Spinning this issue off from: #1806334-185: Replace the node listing at /node with a view
In #1806334: Replace the node listing at /node with a view an additional reloading of modules got added to \Drupal\Core\Extension\ModuleHandler::buildHookInfo
// Make sure that the modules are loaded before checking.
$this->reload();
It was and is unclear why this is needed.
Find the root cause of why $this->reload(); is needed here and fix that and remove this call.
Or find the root cause of why $this->reload(); is needed and document it at this call.
#1806334-185: Replace the node listing at /node with a view @sun
+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php @@ -234,6 +234,8 @@ public function getHookInfo() { + // Make sure that the modules are loaded before checking. + $this->reload();It does not really look like we got to the bottom of why modules are suddenly not loaded when they ought to be loaded.
I additionally do not understand why this calls into reload() instead of loadAll().
We definitely need to change this into loadAll(), and we should also figure out why this is suddenly necessary, as it should not be necessary — instead, there's likely a race condition or bogus/incomplete procedure elsewhere in the code that needs to be fixed.
#1806334-188: Replace the node listing at /node with a view @rootatwc
Actually loadAll() wont work, because $this->loaded is TRUE..
This is not a problem when using module_enable() because it explicitly calls load() on the new enabled module:
So if you manually update the moduleList somewhere in your code, you *must* call load or reload methods if you want the new hooks from this module to be available..
#1806334-191: Replace the node listing at /node with a view @sun
1) the problem only exists in the installer
| Comment | File | Size | Author |
|---|---|---|---|
| #28 | 1940274-28-test.patch | 1.29 KB | quietone |
| #24 | 1940274-24.patch | 634 bytes | longwave |
| #11 | 1940274-11.patch | 2.06 KB | sam hermans |
Comments
Comment #0.0
stpaultim commentedAdding more information from the previous discussion
Comment #1
xjmSee #1806334-185: Replace the node listing at /node with a view comments #185, #187, #188, and #191. #189 contains a patch in progress. Tagging for a complete issue summary.
Comment #2
dawehnerFrom my perspective though this is a bug.
Comment #3
ParisLiakos commentedI still see the same debug messages when enabling modules, but it seems random so far, so this is definitely not fixed.
I plan to look into this and fix the little hack i did once i find time..also the test i posted over to the other issue fails for me locally:/
Comment #3.0
ParisLiakos commentedRemoved separate topic to create it's own issue.
Comment #3.1
stpaultim commentedCleaning it up a bit and adding the issues summary template.
Comment #3.2
yesct commentedmoved the cut and paste
Comment #3.3
stpaultim commentedIdentifying some of the quotes used to describe problem.
Comment #3.4
stpaultim commentedCleaning up the summary under guidance of Core Mentor.
Comment #3.5
stpaultim commentedMaking final changes (I hope) under guidance of my Core Mentor.
Comment #4
ParisLiakos commentedsee #1919002-27: Upgrade to D8 broken when D7 has more then one language enabled too
Comment #5
ParisLiakos commentedok, what happens is, that
install_begin_request()callsdrupal_maintenance_theme()which get us to _system_rebuild_theme_data which invokessystem_theme_infoAnd modules are not loaded because
_drupal_bootstrap_code()has not run yet..I am not sure what to do here..call loadAll() in _system_rebuild_theme_data()/install_begin_request()? Does not sound good to me..
I thinks we should just replace
reload()withloadAll()ingetHookInfo()and add$this->loaded = FALSE;inresetImplementations()Comment #6
ParisLiakos commentedpatch with the proposal above + a phpunit test to confirm..i hope it fails now, cause locally it does
Comment #7
ParisLiakos commentedComment #8
jibran#6: drupal-1940274.patch queued for re-testing.
Comment #9.0
(not verified) commentedSome final formatting changes to issues summary.
Comment #10
star-szrTagging for reroll.
Comment #11
sam hermans commentedRerolled against HEAD
Comment #12
dawehnerIt is certainly not right to force a reload here.
We kinda test more than just GetHookInfo but we "testHookInfoWithNewModule" or something like this. It would be cool if the test method would describe exactly what is going on there.
Comment #24
longwave@lendude mentioned this in #bugsmash. The issue stalled years ago with no clear direction, maybe the underlying bug has been fixed since, so let's just remove the
reload()and see what breaks today.Comment #26
lendudeNice one @longwave
Updated the IS and title to make it more clear (to me at least) what the goal here is.
Comment #27
lendudeDiscussed this in #bugsmash with @larowlan and we both agree that in the current state this should be a task. Nothing is broken as far as we can currently tell (this might change if we find the root cause here).
Comment #28
quietone commentedI decided to investigate this one a bit.
I learned that for each module in the passed in list of module to moduleInstaller::install calls updateKernel(). When the container is rebuilt the injected dependencies are also updated. One of those dependencies is the moduleHandler which has the information on the loadedFiles. When it is updated that data is cleared and thus the state of the ModuleHandler is lost and a reload is needed so hooks are discovered.
IF that is correct then this patch should have less errors. It just adds the reload() to updateKernel().
Comment #30
nicxvan commentedComment #31
nicxvan commentedThe whole hook system has since changed.
Comment #32
xjmAdjusting saved issue credits according to credit guidelines.