Some modules like automated_cron and history should not be hard dependencies of the Sector. Having those as hard dependencies means that websites could not disable them if they are not required and have to live with those enabled facing some performance penalties.
Review the list of dependencies and remove some like automated_cron and history from the list. Those that are used for convenience of the first time installation or just to show case some optional functionality or behaviour should be enabled with sector_install_tasks().
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | sector-review_dependencies-3232986-6.patch | 241 bytes | dieuwe |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
danielvezaThanks for raising this - @dieuwe and myself have been discussing this lately internally and agree it is needed.
In Sector D7 we did this with a batched install inside hook_install. We now need something similar for D9.
I'm concerned this will be a lot of work and our config files will no longer work. I think you can trigger them manually within the configFactory I believe.
Lets target this for 9.3 at the latest. I think we can slip this in with a minor release since it will only effect new installs, it does't break BC.
Comment #5
xurizaemonKeen for review on the smaller change of removing dependencies on History and Automated Cron. These don't ship with any configuration in config/install from Sector, so I think the concern about config files no longer working doesn't apply there.
If it helps discussion for me to move that MR to a specific issue on those two modules, happy to do so. However that could be discussed and landed here as part of the wider discussion, so I'l leave it here for now.
Comment #6
dieuweAfter reviewing and a bit of testing, what we want to do here is very simple.
Non-critical modules need to be shifted from the "dependency" key in
sector.info.ymlto the "install" key.This would be much preferable to just removing them from being enabled by default as that would require consultation with the wider team. Once moved to the "install" key, users will be able to disable those modules on a per-site basis.
For example, the following patch candidate would make every single Sector module uninstallable. That might not be behaviour we want, but it's an option. We can split out the modules between "dependency" and "install" as needed. We should really use the new "module:module" syntax too at some point.
Comment #8
xurizaemonMoving from dependencies: to install: seems like a really sensible first step, if it's doable; that unlocks a current point of rigidity.
Agree on project namespacing, looked around and confirmed that Thunder namespaces install: entries. (Most core profiles seemed to have not namespaced install: entries when I looked?)
Where there are "real" dependencies I think that should be handled at the component level, eg Sector installs Sector Blocks, and Sector Blocks depends on Block.
MR !15 above some tidy up while I was having a think about this, mostly to get a feel for what modules were in there and which of them were core or contrib. MR should work equivalently to the patch, but also adds namespaces and sorts into core then contrib (which I found a lot clearer to consider).
EDIT: OMG just saw Thunder doesn't project namespace its themes when I was closing a bunch of tabs. Hecksakes :)
Comment #9
danielvezaI've tested this, it seems to actually work really well. All config still seems to get imported, so great start.
As you mentioned, themes don't get namespaced, so the MR just needs to resolve that issue.
Comment #10
xurizaemonDone, thanks Daniel! Back to NR.
Comment #12
dieuweIt's looking good now - we might want to have a discussion if there are any modules that we want ensure cannot be uninstalled. But I'm learning towards just leaving this open-ended for now.
Comment #14
dieuweMerged.