localgovdrupal / localgov_paragraphs

Configuration and dependencies for paragraphs components for the LocaGov Drupal distribution.
GNU General Public License v2.0
0 stars 5 forks source link

Add an update hook to install localgov_numbered_text #161

Closed rupertj closed 9 months ago

rupertj commented 10 months ago

As part of the development of the publications feature, we added a new numbered text paragraph. Later on, we realised that this paragraph could be useful elsewhere in an LGD site, not just publications, so Finn moved it from publications to this module in this commit: https://github.com/localgovdrupal/localgov_paragraphs/commit/e755f93fc15e9964c3a65d5362dfdf722e7c0998

This works fine for new installs, but as it won't be added to existing installs, it blocks people who already use LGD from installing publications, like in this issue: https://github.com/localgovdrupal/localgov_publications/issues/94

So we need to add an update hook to this module to set up the localgov_numbered_text paragraph for people that don't have it yet.

finnlewis commented 10 months ago

I wonder if there a way for the localgov_paragraphs to have a hook_install that detects if the paragraph is enabled and if not, load the config from the localgov_paragraphs module. Would that be cleaner?

@ekes @stephen-cox @andybroomfield have we done something like this elsewhere?

rupertj commented 10 months ago

I don't think that is cleaner. IMO the paragraph type should live in one module and that module should have responsibility for installing it.

Adding an install hook to localgov_paragraphs to install the numbered text paragraph on sites that don't have it yet seems like the best solution to me.

finnlewis commented 10 months ago

Thanks @rupertj

The only reason I suggest this is that in general we've tried to avoid changing configuration with update hooks in a given module. I guess adding new content entities like paragraphs is unlikely to cause problems for existing installs.

Keen to hear other's thoughts on this too.

andybroomfield commented 10 months ago

I think you need to check if the config exists, and if not then install it. Same for the dependent config which should be there. I'd consider doing this in the localgov_publications module as that is where the dependency is needed, but either should be fine.

rupertj commented 9 months ago

Closing this as the change for it is now merged and released.