localgovdrupal / localgov_core

LocalGovDrupal Core module, for helper functions and core dependencies.
GNU General Public License v2.0
3 stars 6 forks source link

Break open LocalGov_core submodules into stand alone modules #111

Closed andybroomfield closed 2 years ago

andybroomfield commented 3 years ago

As a lot of extra dependencies are now being added (seeing scheduled transitions) which might not always be required by sites if they are not using the workflow, I think it is time that at least localgov_workflow and localgov_roles where made into their own module and the dependencies added there.

This is important as Localgov_core is required by the majority of Localgov modules, which means installing a lot of modules that end up not being used. Ideally core should be minimal for any shared configuration and dependencies added only.

Alternatively, the dependencies such as scheduled_transitions are added as composer suggestions instead of composer require. This means the core module can be installed for common required code like the title block without having to pull down a lot of dependencies.

finnlewis commented 3 years ago

@andybroomfield mentioned this in standup. Would be good to gather other people's thoughts. @ekes @stephen-cox @Adnan-cds

Adnan-cds commented 3 years ago

As far as I understand, localgov_core's submodules are always needed. So packaging them as separate modules probably won't do any good as they will be downloaded anyway.

If we really don't want to have lots of hard dependencies for localgov_core, then its code/config will have to be tweaked to make those dependencies optional. For example, the localgov_directories module has an optional dependency on the localgov_services_navigation module. Kind of like that.

andybroomfield commented 3 years ago

Thanks @Adnan-cds Just to clairfy, We've never so far needed to enable any of the sub modules in localgov_core (They replicate existing BHCC functionality). At least within the services stack (localgov_services, localgov_guides, localgov_step_by_step) they do not require the enabling of anything within localgov_workflows or localgov_roles and these two modules are the main things I think are candidates for seperation. localgov_media I think does get used by news and subsites so theres a case for that one to stay.

Yes I like the idea of selctive enabling functionlity, though I think that is more suitable for resolving the inclusion of scheduled transitions for the alert banner module

ekes commented 3 years ago

On a general level I'm in favour of keeping localgovcore to really just that. Absolutely essential functionality that all localgov modules need and can just assume exists. Everything else can be switched on optionally, even if it means sometimes having to add a logic check to see what else is enabled.

The question here though is really about the submodules that don't need to be enabled but are bundled together with core right? Bundled such that their code dependencies are in the same composer.json. This should mean that if they have grown up to be substantial modules in their own right, rather than small things that didn't really have a home elsewhere, they can easily just be spun out of core to sit on their own?

stephen-cox commented 3 years ago

Splitting localgov_workflows out of core makes sense to me as this is not enabled by default and as @andybroomfield has pointed out now includes quite a few dependencies that may not be required on a site just using a few LocaGov modules.

I'm not so sure about localgov_roles as quite a few modules need this, it is enabled during a LocalGov site install and it just contains some configuration for for roles with no further dependencies. Site selectively using Localgov modules don't need to enable it.

As a matter of process, I would suggest reviewing and merging the outstanding pull requests for workflow before splitting workflow out as it will be easier and less work to review and merge them first.

finnlewis commented 3 years ago

Thanks all for your comments. I agree that splitting localgov_workflows makes sense, but also agree that we should get this work that is in flight and has been defined and tested merged in first for efficiency.

@andybroomfield is that OK with you?

andybroomfield commented 3 years ago

Just to confirm the descision in the standup. We will be spliting localgov_workflows into its own module.

stephen-cox commented 3 years ago

There's now a new LocalGov Workflows module: https://github.com/localgovdrupal/localgov_workflows

There's a PR to remove workflows from core: #112

Once that's merged we'll want to do a stable release of workflows (https://github.com/localgovdrupal/localgov_workflows/pull/1), a 2.1.0 release of core and a 2.1.2 release of the profile.

I believe doing things this way should mean that sites are not broken by the change, but I think we should do the 3 releases together to minimise the risk of disruption.

finnlewis commented 3 years ago

Thanks @stephen-cox. Shall we make a time to work on this together to co-orindate and let people know too?

finnlewis commented 3 years ago

(I'm about now if you are!)

ekes commented 2 years ago

Should we close this and open new individual issues for anything else that want spinning out?