Issue #3453509: Make modules responsible for their own message request fields

[Kovalskiy266]

Related PRs

#3937
#3928

Problem

We currently have a hook in social_group_request which adds a field to all group types that use the "Request Access" plugin. This creates problems for group types where a different request flow is needed and where we don't want the "Message" field.

We tried to be clever in the past because we initially had 4 group bundles that were different group types and we wanted to share behavior between them.

Since then we've understood that that's not a maintainable solution and we want to have these kind of functionalities be opt-in (our only "group" type is now also flexible_group rather than open/public/closed/secret)

The automatic generation also creates other problems because it creates config in an install hook that other modules might depend on. This works if we install modules one-by-one using Drush but it doesn't work well if we install the module during tests because these hooks may not fire before the config is attempted to be imported from config/install folders.

Solution

The solution is to remove the magic hook here and instead add the config files to config/optional in the different modules that support group requests (e.g. social_group_flexible_group, social_course, etc.). That way if social_group_request is enabled Drupal will automatically install the config at the right moment.

So, we put field_request_message into the optional folder where it is needed. So, we have a list of these modules, which have their own group type:

1. social_discussion_group - has own group type, but doesn't have group_membership_request plugin, which means we don't need to put field_request_message field
2. social_course_advanced_request - has social_group_request module in dependencies and field config in /install
3. social_course_basic_request - has social_group_request module in dependencies and field config in /install
4. social_discussion_secret_group - has own group type, but doesn't have group_membership_request plugin, which means we don't need to put field_request_message field
5. social_flexible_group - put field in /optional, since this module doesn't have social_group_request in dependencies

Issue tracker

https://www.drupal.org/project/social/issues/3453509

Theme issue tracker

N/A

How to test

• Install social_group module
• Install social_group_flexible_group
• Install the social_group_request module
• Everything should work as previously
  It would be better to test also for social courses

Screenshots

N/A

Release notes

Change Record

N/A

Translations

N/A

[nkoporec]

I do see an issue here, the current implementation is making sure that each new group type has the field_grequest_message field, the field is then used is GroupRequestMembershipRequestForm.

With your change we cannot longer assume that this field exists, the field would need to be created by the user when creating the new group type. There are also other implementations referencing this field , for example SocialGroupRequestConfigOverride. So we cannot just remove the hook without at least adding checks to classes mentioned above to check if the field is present in the group type.

For me the current implementation, makes sense, the module relies on field being there so it basically enforce it via hook. If entity_type and field_name are missing can't we just add that instead of removing the hook ? If only the tests are the issue then for me this is an indication that something is missing in the tests it self or the error is in the test.

[Kovalskiy266]

Hi, @nkoporec
Thanks for the review!

| With your change we cannot longer assume that this field exists, the field would need to be created by the user when creating the new group type
Yeah, so, when the new group_type is created, and we want to enable the membership request plugin for this, we should add the field manually for this group_type

| If entity_type and field_name are missing can't we just add that instead of removing the hook ?
Yeah, that makes a sense :)

| If only the tests are the issue then for me this is an indication that something is missing in the tests it self or the error is in the test.
In tests we should install the social_group_request module, when I enable this module in tests I have errors related to entity_type and field_name , so the problem not in test itself

Discussion in slack about it https://opensocial.slack.com/archives/C0RQ56PFX/p1717575097345489

[Kingdutch]

I slightly reframed the problem statement in the PR (we likely want to update the Drupal.org issue to match). I hope this change in problem statement shows why removing the hook itself is the correct way forward.

Nejc is right that if we have other pieces of code that reference this field, then removing the hook is not enough to make this work.

GroupMembershipRequestForm expects the field to be always there, this is a problem because we're creating a situation where that may not be the case.. We ran into this form before and concluded that it should really be using a proper Content Entity Form so that it can get its fields from the form display configuration. If we were to make that change then that would remove the dependency of that form on this field. That's a change we can make in a separate PR (since it's a change that should work on its own) but it likely would be needed before this PR is merged.

The other place where this field is referenced in the distribution is SocialGroupRequestConfigOverride. We know from past experiences that config overrides cause us a lot of pain (although they were needed when we were still using the features module). We now have different tools such as the config_optional module which allows us to install partial config objects where needed. To be able to remove this hook we would likely need to convert that config override to YML files as well. I believe we can do this in a standalone PR as well to remove that override and make it proper config management. We may run into the same issue of ordering we had in our test in which case we'd need to merge it into this PR instead.

It's unfortunate that we're running into some scope creep here. However, I think the underlying issue is tech debt that we're now encountering for at least a second time. So I also think that that, combined with the path of tackling them in clearly scoped PRs, is a good reason to clean up the tech debt now and not push it forward any longer.

(As a bonus, if we fix the form in social_group_request we're likely to be able to replace a custom customer form in a follow-up)