-
Notifications
You must be signed in to change notification settings - Fork 8
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
Group as dev dependency #290
Comments
Sounds like a good idea. |
@andybroomfield this is where it will get complex as microsites uses If it's not targeting a specific version we can probably do; "drupal/group": "^2 || ^3", |
Hmm it also requires It is starting to feel that this submodule should exist as its own module or a submodule within |
@millnut I don't see localgov_microsites_group as a test dependency here , and thats the only test. I think the sub module can be used as a generic group plugin, there are just some extra (optional?) config that gets installed on microsites. If group 3 is the what microsites uses it makes sense thats what we target. |
Fix #290 Set group v3 as a dev dependancy.
Fix #290 Set group v3 as a dev dependancy.
@andybroomfield sorry that was my bad, it was flagging as it couldn't work out the class due to microsites_group missing when the |
This snippet from the Github test runner should have taken care of this issue. Not sure why it didn't. Anyway, having drupal/groups:^3 as a dev dependency is not a bad idea. I can confirm that there is no dependency on localgov_microsites_group. |
Hi @Adnan-cds the localgov_microsites_group dependency I’m seeing here for the helper https://github.com/localgovdrupal/localgov_alert_banner/blob/1.x/modules/group_alert_banner/group_alert_banner.module#L55. cc: @andybroomfield |
That's right. The relevant function is a hook implementation. It is triggered only if the localgov_microsites_group module is available and ignored otherwise. |
Great thanks for clarifying @Adnan-cds I now have a better understanding of how that works now. |
No problem. |
I think
drupal/group
now needs adding as a dev dependency so tests can run see failures here https://github.com/localgovdrupal/localgov_project/actions/runs/6316343354/job/17318874731I believe that is how
drupal/scheduled_transitions
is handled in https://github.com/localgovdrupal/localgov_alert_banner/blob/1.x/composer.json for the same reasons@finnlewis, @Adnan-cds, @andybroomfield on thoughts on this?
The text was updated successfully, but these errors were encountered: