Router table is not cleared when modules are uninstalled

We should summon Crell here too I think.

I am wondering, if we go down this path or flushing before rebuilds we will have to decide what to do with code in MatcherDumper::dump() as alot of that may not be applicable anymore if we work under the assumption that all routes will be cleared out.

Just a general note: the old menu system ensured that the table was cleared before. This is a safe assumption, sure, thought I wonder whether we should have some bigger transaction, to be sure that we never loose our sort of working routing.

@BTMash
For this usecase I really think we need some integration kind of webtest, as we want to test the full system, not just each component.

Would be better to dump just before writing the routes as opposed to before collecting them - much shorter window where there's an empty table.

Do we already trigger a rebuild when there's no routes? Or is there already a lock/wait cycle? If not that should be added here.

Was talking to catch about this, whilst doing so I had another idea. We could clear out any router items on module uninstall instead. That way we do not mess with any exiting functionality or have to worry about multiple rebuilds/locks/empty router tables.

Super small patch.

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
@@ -759,6 +759,9 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
       $schema_store->delete($module);
+
+      // Make sure any route data is also removed for this module.
+      \Drupal::service('router.dumper')->dump(array('provider' => $module));
     }
     drupal_get_installed_schema_version(NULL, TRUE);

It is sad that we couple the module handle to even more bits. Can we at least inject the service?

@BTMash: The dumper already flushes and re-inserts... per provider/module. The only problem should be when modules no longer exist, then they wouldn't be flushed.

@dawehner, that would then the the only service injected. Seems better to just follow suit in this case? Then maybe fix all of this when I have done this work on the module event?

@berdir, exactly, Our current code in the MatcherDumper already handles all other cases. This is the approach that is the least invasive also. Preserving the current functionality completely.

Also, just created #2206347: Use event system in ModuleHandler to hopefully untangle the rats nest that is the module handler.

The fix seems to be the way to go on the midterm

Agreed. Also updating the issue title to better reflect what we're doing. Slightly updated the proposal to reflect what we're doing in now.

Yep, this looks great now. Committed/pushed to 8.x, thanks!