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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia’s picture

Docs fixes.

msonnabaum’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 09db04d and pushed to 8.x. Thanks!

catch’s picture

Status: Fixed » Active

Sorry re-opening this, no time for a long comment but see #2001310-3: Disallow firing hooks during update.

effulgentsia’s picture

Status: Active » Needs review
FileSize
42.25 KB

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?

effulgentsia’s picture

Title: Move module enable/disable/uninstall implementations into ModuleHandler » Move module enable/disable/uninstall implementations into ModuleInstaller

Retitling issue accordingly.

chx’s picture

Nope.That's a bad idea. The module enable operates on the module list as does the rest of the module handler.

catch’s picture

I don't see why that makes it a bad idea. Why should discovery and metadata parsing be put in the same class as hook invocation?

fubhy’s picture

I'd actually also support this as it seems just saner to provide multiple helper methods and simplify the logic in ->install() and ->uninstall() thereby.

Can we agree on solving this in #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed as I need to do quite a few other, related things in there as well?

effulgentsia’s picture

Status: Needs review » Postponed

I'm fine with postponing this to whatever shakes out of #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed. If that issue doesn't get resolved in 2 weeks or so though, it might be worth unpostponing this to get it done before code freeze.

In #7, @chx was opposed to this though, so might be similarly opposed to it being done in that issue too.

fubhy’s picture

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.

Pancho’s picture

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: Deprecate module_load_install() and replace it with 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.

[edit:] struck-through some plain wrong assumptions of mine. Patches are rolled in #2003966: Helper issue for #1199946 : Disabled modules are broken beyond repair and will probably land before we get settled on this one. Still we can keep working here.

fubhy’s picture

Priority: Normal » Major
Status: Postponed » Needs work

@chx Can you elaborate why you are against ModuleInstaller as a separate service?

Pancho’s picture

Title: Move module enable/disable/uninstall implementations into ModuleInstaller » Move module install/uninstall implementations into ModuleInstaller

Decision-making is finished on #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed, so enabling will always be installing and disabling will become uninstalling.

chx’s picture

*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?

Pancho’s picture

Status: Needs work » Needs review
FileSize
43.15 KB

Let's start with a straight reroll of #5 after #1852454: Restrict module and theme name lengths to 50 characters and more.
Will re-iterate on this later.

Status: Needs review » Needs work

The last submitted patch, module-installer-18.patch, failed testing.

Pancho’s picture

Status: Needs work » Needs review

#17: module-installer-18.patch queued for re-testing.

Pancho’s picture

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.

fubhy’s picture

I am very much +1 on this especially after speaking to @katbailey in Dublin. But let's postpone it on #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed as that changes ModuleHandler quite a bit and we really don't want to re-roll that too often.

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.

Pancho’s picture

Full ACK on #21.
Let's wait a bit longer, but it would really be a shame to miss deadline + x because of the other issue.

Status: Needs review » Needs work

The last submitted patch, module-installer-18.patch, failed testing.

fubhy’s picture

Status: Needs review » Needs work

The last submitted patch, drupal8.base-system.2004784-24.patch, failed testing.

sun’s picture

Issue summary: View changes
Issue tags: +API clean-up, +@deprecated

The functions have been moved into ModuleHandler in the meantime.

Do we want to repurpose this issue or open a new one to remove the @deprecated wrappers?

fubhy’s picture

The functions have been moved into ModuleHandler in the meantime.

Yeah and this issue is about moving them out of there into a dedicated service (namely ModuleInstaller).

dawehner’s picture

Status: Needs work » Closed (duplicate)