ModuleHandler::setModuleList() should call resetImplementations() in order to avoid edge cases

Hm. I think I did exactly this change for another issue already.

Oh yeah, #1911178-33: Remove hook_exit()

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.

I guess it is to call getHookInfo() and fill up the variable for first time?

#1: drupal-1929656-1.patch queued for re-testing.

This is blocking a major, so it is also major.

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

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 of module_disable() is bogus — HEAD clears the hook implementations too late, which means that watchdog() 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, whereas module_enable() only calls setModuleList().

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.

I agree that module_enable is covered by this change, but setModuleList is not called only in module_enable right?
That said, this covers most cases and this is major, so i am fine with rtbc

  // 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.

Thank you! :)

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. :(

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.

This 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