Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Break open LocalGov_core submodules into stand alone modules #111

Closed
andybroomfield opened this issue Jul 9, 2021 · 11 comments
Closed

Break open LocalGov_core submodules into stand alone modules #111

andybroomfield opened this issue Jul 9, 2021 · 11 comments

Comments

@andybroomfield
Copy link
Contributor

andybroomfield commented Jul 9, 2021

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
Copy link
Member

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

@Adnan-cds
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Member

ekes commented Jul 12, 2021

On a general level I'm in favour of keeping localgov_core 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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

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

@stephen-cox
Copy link
Member

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 (localgovdrupal/localgov_workflows#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
Copy link
Member

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

@finnlewis
Copy link
Member

(I'm about now if you are!)

@ekes
Copy link
Member

ekes commented Oct 12, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants