Posted by dawehner on February 27, 2013 at 11:03pm
14 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | base system |
| Category: | bug report |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | VDC |
Issue Summary
There have been edge cases (for example when running the installer) on which the hookInfo in the module handler was outdated, but the moduleList was updated.
One possible call route is install_finished -> module_set_weight ... drupal_flash_all_caches() -> call some hook,
see #1806334: Convert the node listing at /node to Views for one problem caused by that.
Let's just call resetImplementations once we set new modules.
Comments
#1
This is not a 100% bugfix yet, as for example there are no tests, which proove something is not working.
Additional CachedModuleHandler also plays a certain role in the game.
#2
Hm. I think I did exactly this change for another issue already.
Oh yeah, #1911178-33: Remove hook_exit()
#3
Or in other words, the main change here is the one from #1766036-44: ♪♫ Question the sun ♥, the moon, and the stars ♫♪ [Test that stuff.]
This change makes absolutely sense to me, so +1.
+++ b/core/modules/system/lib/Drupal/system/Tests/Module/ModuleApiTest.php@@ -97,11 +97,15 @@ function testModuleImplements() {
+ // Warm up the module handler, so ensure that the internal caches are
+ // cleared when setting the module list.
+ $module_handler = drupal_container()->get('module_handler');
+ $module_handler->invokeAll('test');
That's the only part that doesn't make sense to me.
The comment apparently tries to help, but I do not understand why that is needed.
#4
I guess it is to call
getHookInfo()and fill up the variable for first time?#5
The last submitted patch, drupal-1929656-1.patch, failed testing.
#6
@rootawtc
Yeah this was exactly my intention, but yeah as wrote above, it doesn't really work as expected.
#7
#1: drupal-1929656-1.patch queued for re-testing.
#8
This is blocking a major, so it is also major.
#9
The last submitted patch, drupal-1929656-1.patch, failed testing.
#10
For the failure see #1911178-18: Remove hook_exit()
Basically doing
- $this->assertLogMessage('system', "%module module disabled.", array('%module' => $module), WATCHDOG_INFO);+ if ($module != 'dblog') {
+ $this->assertLogMessage('system', "%module module disabled.", array('%module' => $module), WATCHDOG_INFO);
+ }
would turn this green
#11
It seems to be that this problem can only appear when we don't use the cached module handler, so it maybe is time for a separate unit test case,
as this specific detail could be tested without a full bootstrapped drupal/
#12
Add tag back.
#13
#14
I do not think that this needs any further tests. These tests would essentially try to assert that a previously contained bug no longer exists.
The test failure for dblog.module's
hook_watchdog()implementation in #1 only appears, because the current procedure ofmodule_disable()is bogus — HEAD clears the hook implementations too late, which means thatwatchdog()is invoked with the old list of hook implementations (which include dblog.module, even though it was just disabled).In other words, all we could test here is that a hook implementation is first found, and no longer found after calling
setModuleList()to remove that module.But that's essentially what the test changes I outlined in #3 are doing: ->invokeAll() is called to prime the hook implementation cache, so as to verify that the subsequent
module_enable()also clears the hook implementation cache, whereasmodule_enable()only callssetModuleList().Consequently, I think this patch is RTBC.
However, could we quickly improve the inline comment for #3? It doesn't really state and clarify what I just explained above.
#15
I agree that
module_enableis covered by this change, butsetModuleListis not called only inmodule_enableright?That said, this covers most cases and this is major, so i am fine with rtbc
#16
@sun
Do you mean something like that? To be honest I'm still not convinced 100% that this test works as expected but I trust you 100%.
#17
// Prime ModuleHandler's hook implementation cache by invoking a// random hook name. The subsequent module_enable() below will only
// call into setModuleList(), but will not explicitly reset the
// hook implementation cache, as that is expected to happen
// implicitly by setting the module list. This verifies that the
// hook implementation cache is cleared whenever setModuleList() is
// called.
$module_handler = yada.
#18
Oh that's way better.
#19
Thank you! :)
#20
That special-casing of dblog looks a bit strange, but I guess we were already previously doing that elsewhere in the function.
Unfortunately, no longer applies. :(
#21
And by that I mean, committed and pushed to 8.x. :P
Hopefully that worked.. I'm about to be offline for a few hours, but I'll check in again before I hit the sack.
#22
This was (I think) to help with the view for /node, but the debug messages are still there on install #1806334-157: Convert the node listing at /node to Views
#23
Automatically closed -- issue fixed for 2 weeks with no activity.