Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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: Replace the node listing at /node with a view for one problem caused by that.
Let's just call resetImplementations once we set new modules.
Comment | File | Size | Author |
---|---|---|---|
#18 | drupal-1929656-18.patch | 6.02 KB | dawehner |
#18 | interdiff.txt | 1.13 KB | dawehner |
#16 | drupal-1929656-16.patch | 5.66 KB | dawehner |
#16 | interdiff.txt | 844 bytes | dawehner |
#11 | drupal-1929656-11.patch | 5.72 KB | dawehner |
Comments
Comment #1
dawehnerThis 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.
Comment #2
sunHm. I think I did exactly this change for another issue already.
Oh yeah, #1911178-33: Remove hook_exit()
Comment #3
sunOr 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.
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.
Comment #4
ParisLiakos CreditAttribution: ParisLiakos commentedI guess it is to call
getHookInfo()
and fill up the variable for first time?Comment #6
dawehner@rootawtc
Yeah this was exactly my intention, but yeah as wrote above, it doesn't really work as expected.
Comment #7
ParisLiakos CreditAttribution: ParisLiakos commented#1: drupal-1929656-1.patch queued for re-testing.
Comment #8
tim.plunkettThis is blocking a major, so it is also major.
Comment #10
ParisLiakos CreditAttribution: ParisLiakos commentedFor the failure see #1911178-18: Remove hook_exit()
Basically doing
would turn this green
Comment #11
dawehnerIt 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/
Comment #12
dawehnerAdd tag back.
Comment #13
ParisLiakos CreditAttribution: ParisLiakos commentedComment #14
sunI 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.
Comment #15
ParisLiakos CreditAttribution: ParisLiakos commentedI agree that
module_enable
is covered by this change, butsetModuleList
is not called only inmodule_enable
right?That said, this covers most cases and this is major, so i am fine with rtbc
Comment #16
dawehner@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%.
Comment #17
sunComment #18
dawehnerOh that's way better.
Comment #19
sunThank you! :)
Comment #20
webchickThat 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. :(
Comment #21
webchickAnd 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.
Comment #22
YesCT CreditAttribution: YesCT commentedThis was (I think) to help with the view for /node, but the debug messages are still there on install #1806334-157: Replace the node listing at /node with a view