You are here

Move module install/uninstall implementations into ModuleInstaller

Spin off from #2001310: Disallow firing hooks during update. In that issue, we want to swap the ModuleHandler service during update.php, and use that to change what enable/disable/uninstall do. But to do that, we need to make the procedural functions just wrappers around the service methods. This issue is for doing that.

Please credit chx on commit, since he wrote the initial patch from which this is extracted.

Fair enough. This moves those 3 methods to a new class/service: ModuleInstaller. This patch only does that (and changes corresponding calls from $this to $this->moduleHandler). There's also some good feedback on #2001310-10: Disallow firing hooks during update not yet implemented here, but perhaps that can be a follow up?

Yeah, I am not going to move it around in that other issue. Not really related and just makes reviewing harder anyways although it looked nice at a first glance. We should keep this postponed for now anyhow as it's just going to end it pointless re-rolls otherwise.

I like the idea of ModuleHandler being read-only regarding to the $moduleList, so everything wanting to change the current set of enabled modules or wanting to load or run non-enabled code needs to use ModuleInstaller instead:
- install()
- loadInstall()
- enable/disable/uninstall
- setModuleList()
Did I forget something?

The alternative would be isolating install() only because it works on a SystemListing object, so loads and runs code that is currently uninstalled. #2010380-9: Add ModuleHandler::loadInstall goes that route even though the patch currently only covers loadInstall() and not install() itself. @tstoeckler didn't want to "taint" the ModuleHandler with a SystemListing object which makes a lot of sense. But I think it's a different thing if we're tainting the ModuleInstaller only.

Also, I don't see why we necessarily have to keep this here postponed to #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed. Certainly re-rolls suck, but that other issue is deeply stuck in the decision phase, not rolling out patches. Also it might take weeks, if not months until it finally lands, so I'm not 100% sure we will see a solution in D8 at all.
At the same time our issue here isn't necessarily dependent on that one. If we see a solution there, it would either be that disabled status completely goes away (clean but UI-wise hardly acceptable) or be transformed into some kind of enabled-but-hidden kind of status. While disabled might turn into uninstalled, the general paradigm we'd come up with in this issue, would still hold in all possible outcomes.

Therefore I hope you're okay with this one here being reopened, @fuhby. Also recategorizing it as major, because we should IMHO really get these basic paradigms settled way before code freeze. I'd be fine if you're reverting it.

*supposedly* OOP was about encapsulation (although Drupal 8 is happy to abuse it for autoloading which, combined with impossible-to-find-anything-known-to-unfit-for-drupal-before-it-was-a-standard-psr0 is a fairly bad choice) and the module list is something that strikes me as one of those things that we need to encapsulate so we do not end up with module_list again taking a fixed list and other unholy things. You have the module list, it's a property on the class, maybe you can read it -- or better actually if you can't even read it! -- and you have the limited methods that manipulate it in standardized ways. At least this is how I understood OOP and this is why we have protected properties in place... no?

What about this one?
IMHO it would be a great improvement to separate out whatever doesn't work on the module list to a separate ModuleInstaller service.
But do you see a slight chance to get this in after July 1st, if we're at least faster than the rollout of the first beta?
Second-best, we could leave the 3 methods deprecated in ModuleHandler.

However, in the end ModuleHandler should be unaware of how modules function and how they are discovered. Optimally it will only be a wrapper around the list of modules that are currently installed and their namespaces + methods that operate on that list like ->invoke() etc.