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.
Comment | File | Size | Author |
---|---|---|---|
#24 | drupal8.base-system.2004784-24.patch | 94.65 KB | fubhy |
#17 | module-installer-18.patch | 43.15 KB | Pancho |
#5 | module-installer-5.patch | 42.25 KB | effulgentsia |
#1 | module-handler-enable-1.patch | 37.76 KB | effulgentsia |
#1 | interdiff.txt | 5.16 KB | effulgentsia |
Comments
Comment #1
effulgentsia CreditAttribution: effulgentsia commentedDocs fixes.
Comment #2
msonnabaum CreditAttribution: msonnabaum commentedLooks good to me.
Comment #3
alexpottCommitted 09db04d and pushed to 8.x. Thanks!
Comment #4
catchSorry re-opening this, no time for a long comment but see #2001310-3: Disallow firing hooks during update.
Comment #5
effulgentsia CreditAttribution: effulgentsia commentedFair 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?Comment #6
effulgentsia CreditAttribution: effulgentsia commentedRetitling issue accordingly.
Comment #7
chx CreditAttribution: chx commentedNope.That's a bad idea. The module enable operates on the module list as does the rest of the module handler.
Comment #8
catchI 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?
Comment #9
fubhy CreditAttribution: fubhy commentedI'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?
Comment #10
effulgentsia CreditAttribution: effulgentsia commentedI'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.
Comment #11
fubhy CreditAttribution: fubhy commentedYeah, 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.
Comment #12
PanchoI 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.
Comment #13
fubhy CreditAttribution: fubhy commented@chx Can you elaborate why you are against ModuleInstaller as a separate service?
Comment #14
PanchoDecision-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.
Comment #16
chx CreditAttribution: chx commented*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?
Comment #17
PanchoLet'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.
Comment #19
Pancho#17: module-installer-18.patch queued for re-testing.
Comment #20
PanchoWhat 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.
Comment #21
fubhy CreditAttribution: fubhy commentedI 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.
Comment #22
PanchoFull 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.
Comment #24
fubhy CreditAttribution: fubhy commentedRe-roll now that #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed got committed.
Comment #26
sunThe 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?
Comment #27
fubhy CreditAttribution: fubhy commentedYeah and this issue is about moving them out of there into a dedicated service (namely ModuleInstaller).
Comment #28
dawehnerLet's mark it as duplicate of #2324055: Split up the module manager into runtime information and extension information.