Refactor away ModuleInstaller::invokeAll()

$this->invokeAll

Is called explicitly to call loadAll before calling ModuleInstaller

protected function invokeAll($hook, $args = []): void {
    $this->moduleHandler->loadAll();
    $this->moduleHandler->invokeAll($hook, $args);
  }

We would have to call loadAll before each instance of invokeAll in ModuleInstaller

There is no single call to $this->moduleHandler->invokeAll($hook, $args); outside of ModuleInstaller::invokeAll

$this->invokeAll('module_preinstall', [$module, $sync_status]);
$this->invokeAll('modules_installed', [$modules_installed, $sync_status]);
$this->invokeAll('module_preuninstall', [$module, $sync_status]);
$this->invokeAll('modules_uninstalled', [$module_list, $sync_status]);
$this->invokeAll('cache_flush');

Those 5 hooks are all invoked in other modules than the one being installed and this allows them to be OOP.

The thing that might be confusing is there is also:

ModuleInstaller::invoke()
This is called on the modules being installed and cannot be oop cause they are not loaded yet.

$this->invoke($module, 'update_last_removed')
$this->invoke($module, 'install', [$sync_status])
$this->invoke($module, 'uninstall', [$sync_status])
$this->invoke($module, 'schema')

This uses function_exists

protected function invoke(string $module, string $hook, array $args = []): mixed {
    $function = $module . '_' . $hook;
    return function_exists($function) ? $function(... $args) : NULL;
  }

I'd love to factor that away, but I think that should be handled elsewhere.

    // Let other modules react.
    $this->invokeAll('modules_uninstalled', [$module_list, $sync_status]);

    // Flush all persistent caches.
    // Any cache entry might implicitly depend on the uninstalled modules,
    // so clear all of them explicitly.
    $this->invokeAll('cache_flush');

would be much clearer to me as

    // Load all modules because ...
    $this->moduleHandler->loadAll();

    // Let other modules react.
    $this->moduleHandler->invokeAll('modules_uninstalled', [$module_list, $sync_status]);

    // Flush all persistent caches.
    // Any cache entry might implicitly depend on the uninstalled modules,
    // so clear all of them explicitly.
    $this->moduleHandler->invokeAll('cache_flush');

Similarly

    foreach ($module_list as $module) {
...
      // Allow modules to react prior to the uninstallation of a module.
      $this->invokeAll('module_preuninstall', [$module, $sync_status]);

Why do we need to call loadAll() every time we go around the loop? Clearer to call it once at the start, with a reason.

The only concern I have for the former is if they get split up, but that would be in another issue, this way it's all unified.

Why do we need to call loadAll() every time we go around the loop? Clearer to call it once at the start, with a reason.

That's actually probably worth considering.
In fact I wonder if the loadAll is needed for the uninstall ones at all.
Install maybe especially now that we are considering doing them in batches.

So the loadAll was added in 📌 OOP hooks using event dispatcher Needs review

module_preinstall
modules_installed
module_preuninstall
modules_uninstalled
cache_flush

Previously called: $this->moduleHandler->invokeAll

They were changed to:
protected function invokeAll($hook, $args = []): void {
$this->moduleHandler->loadAll();
foreach ($this->moduleHandler->getModuleList() as $module => $extension) {
$this->invoke($module, $hook, $args);
}
}

and invoke does the function exists.

This was then consolidated to what it is today here: 📌 Fix invocation of hook_module(s)_(pre(un))install(ed) so they support OOP hooks Active and here 📌 Clean up how ModuleInstaller invokes hooks around installing other modules. Active

I bet we can go full circle as you pointed out, and remove:
ModuleInstaller::invokeAll() and just call $this->moduleHandler->invokeAll(
Directly, that should just work.

Oh that triggers Exception: The theme implementations may not be rendered until all modules are loaded. maybe we can use this to find out how to handle the hook_theme issues too.

That exception is removed in https://git.drupalcode.org/project/drupal/-/merge_requests/6290/diffs?fi...

I think it's going to be OK to remove it, because:

1. It doesn't stop other early hook invocation like the cases mentioned in 🐛 ModuleHandler skips all hook implementations when invoked before the module files have been loaded Needs review .

2. As we convert hooks to OOP, the concept of loading modules is going to eventually disappear.

3. Drupal\Core\Utility\ThemeRegistry and \Drupal\Core\Theme\Registry both check if all modules are loaded before writing to the persistent theme registry cache, so that cache can't be polluted anyway. The worst that can happen is the early theme call fails, but it would already fail now with the exception.

4. It was added in #2346937: Implement a Renderer service; reduces drupal_render / _theme service container calls → which predates the code in #3.

The only related issue is this: 🐛 Theme hooks not found warning on Umami install Active

nicxvan → changed the visibility of the branch 3492282-refactor-away-moduleinstallerinvokeall to hidden.

smustgrave → closed merge request !10478

Seems pretty straight forward. Matches what the issue summary is proposing and think we would know pretty quickly if it broke anything :)