Request Membership feature

Hi, thanks for kicking this off! It looks like a great start to replicate the D7 functionality with.

Here's a few comments, not including whitespace errors, documentation, etc.

1. +++ b/group.install
   @@ -172,3 +172,21 @@ function group_update_8002() {
   +function group_update_8003() {
   

   This hook should not add a field to group content as a whole. Instead, we need to add a field to memberships like 'group_roles' is added.

2. +++ b/group.links.task.yml
   @@ -34,6 +34,12 @@ group.members:
   +group.pending_members:
   +  title: 'Pending Members'
   +  base_route: 'entity.group.canonical'
   +  route_name: 'entity.group_content.group_membership.pending_collection'
   +  weight: 15
   

   This looks fine for now, although we want to get rid of "hard-coded" tabs in favor of optional views in the future.

3. +++ b/src/Entity/Controller/GroupContentListBuilder.php
   @@ -139,13 +146,34 @@ class GroupContentListBuilder extends EntityListBuilder {
   +    $current_path = explode('/', \Drupal::service('path.current')->getPath());
   +    if ($current_path[3] == 'pending-members') {
   

   This we don't want to do. Preferably we'd check the route or something.

4. +++ b/src/Entity/GroupContent.php
   @@ -345,6 +361,13 @@ class GroupContent extends ContentEntityBase implements GroupContentInterface {
   +    ¶
   +    $fields['request_status'] = BaseFieldDefinition::create('integer')
   +      ->setLabel(t('Request Status'))
   +      ->setDescription(t('Request mebership status.'))
   +      ->setSetting('unsigned', TRUE)
   +      ->setDefaultValue(1)
   +      ->setTranslatable(TRUE);
    
   

   Again, we'd want this on group memberships only (a bundle field).

All in all, the idea is good. I definitely want to see this land before a 1.0 is released.

Looking forward to seeing this feature as well, it's crucial for a project I'm working on. @visabhishek let me know if there is a way we can collaborate.

Rerolled patch against latest in 8.x-1.x-dev branch.

I'm going to make an attempt at fixing the issues in #6.

This patch follows the same idea as the patch from #9, with these changes:

- A boolean field is added to the GroupMembership content plugin
- Pending members are shown in an optional view, with approve/reject operation links
- Pending members are given the "outsider" role until approved

Please let me know if it needs any changes.

This patch fixes a bug in the patch from #13 that prevented users joining groups without requesting access.

Rerolled so it works with the latest version. Increaded hook_update_N value to 10.

The latest patch contains some clutter by including code from other issues. Could you please have a look at what may have gone wrong when re-rolling and set it to needs review after?

I'm going to take a shot at re-rolling this patch.

Re-rolled patch from #17 to account for new update hooks.

To use, edit a group type's permissions and check the "Request group membership" permission for the Outsider role. Make sure the "Join group" permission is not checked, otherwise users will see both options.

+++ b/group.install
@@ -347,3 +351,66 @@ function group_update_8009() {
+      FieldConfig::create([
+        'field_storage' => FieldStorageConfig::loadByName('group_content', 'group_requires_approval'),
+        'bundle' => $group_content_type_id,
+        'label' => t('Requires Approval'),
+        'settings' => [
+          'on_label' => 'Requires Approval',
+          'off_label' => 'Requires Approval',
+        ],
+      ])->save();

This is missing the field_name and the entity_type which are both required for the FieldConfig.

Upgrade path now creates the view and the field storage which was causing the errors I was facing in #25 regarding missing entity type and missing field. I've also fixed a couple of schema issues to hopefully get the tests back green.

When requesting membership the redirect sent you to a 403 so i've fixed that and added a drupal_set_message. There a few inconsistencies, GroupContentForm sets the redirect in save instead of submitForm() like the rest of this patch.

Rerolling #27 against latest dev branch.

Attached reroll of #28 against latest dev branch.

Few nitpicks, but otherwise very well written patch:

• The group_members view doesn't seem to get updated in an update hook.
• There are some unrelated hunks in there that are purely code style, probably done automatically by your IDE
• I'm not too sure about using a field for this, it seems a bit too simplistic and hard to extend. In Group 7 we had the notion of membership states; these could be anything ranging from Active to Pending or even Blocked and behind the scenes it would tell Group whether the membership was active or not. I'm contemplating whether we should rebuild that system on Group 8.

All in all great work, though!

This is definitely is a great feature. I'm following the issue.

Great work. This feature is really valuable for "closed groups" and we are thinking about adding it to the Open Social distro in the short term. What would still be needed to get this into Group?

As I mentioned earlier, I'd like to come up with a solid system of membership states. If we start adding a field for "requires approval" and then one for "is temporarily banned" and then another one for "is invited" etc. we are going to have a ton of fields which basically ask the same question: "What state is the membership currently in?"

P.S.: All the patch-unrelated changes introduced by the author's IDE need to be removed so that we have a clear view of the patch.

I tried to use it on real project, but it breaks drush cim

Rerolled patch from #29 against current 8.x-1.x branch.

This feature also interests me

Hi,

I'm trying to apply this patch to test out the feature, without any success. How should it be applied? Thanks in advance.

Currently I've got this as feedback when I do a git apply --check for the latest patch on 1.x-dev:
> error: patch failed: src/Controller/GroupMembershipController.php:23
> error: src/Controller/GroupMembershipController.php: patch does not apply

Frank

I've successfully applied patch #35 and looks good to me. Therefore I changed this issue's status. It would be good if someone else can test it as well.

how does this work?

Updated the patch to fix typo. Changed 'You membership request...' to 'Your membership request...'

Here is an updated patch that should apply cleanly. Regarding #40 @SocialNicheGuru it looks like this patch updates default views, so you may have to re-import or uninstall and re-install the module after patching.

I still haven't reviewed this as it applies to a working install base, just running it through the test suite to see how things work out.

I'm starting work to follow @kristiaanvandeneynde's advice and remove superfluous changes, as well as implement a membership state in favor of using fields.

> implement a membership state in favor of using fields

Maybe it's worth considering https://www.drupal.org/project/state_machine as an alternative?

I was thinking this could be implemented with Content Workflows now that they are part of core now, and handle state transitions. However it has been stated that revisioning must be approached with care in regards to the access chain.

I wrote this patches that takes the model of a role and creates a similar membership state to be used in a reference on the membership group content. On on the fence about whether or not it addresses any of the above problems though.

I've been using #46 for a while and have added some improvements to make the select entity reference on a membership form work properly. Also, the membership states list view now appears more prominently in the UI.

@arosboro just dropping a quick note that after applying the patch my non-admin users just get a WSOD with the error logged as:

Error: Call to a member function isPending() on null in /modules/contrib/group/src/GroupMembership.php on line 99 
#0 /modules/contrib/group/src/Entity/Storage/GroupRoleStorage.php(103): Drupal\group\GroupMembership->requiresApproval()"

I suspect the module needs a reinstall as mentioned above, but it doesn't appear I can do that without deleting all my groups and content :/

it doesn't appear I can do that without deleting all my groups and content :/

Yes, you can - revert to the previous commit / db-dump ;)

@scott.whittaker no need to rever, this just means existing memberships don't have a state, and the db update didn't run. In general though, it is a good idea to have backups.

<?php
 
/**
 * Run this to ensure existing group memberships are pre-populated with an active state.
 */
$config_factory = \Drupal::configFactory();
foreach ($config_factory->listAll('group.content_type.') as $group_content_type_config_name) {
    $group_content_type = $config_factory->getEditable($group_content_type_config_name);
    $plugin_id = $group_content_type->get('content_plugin');
    $group_type_id = $group_content_type->get('group_type');
    // Only group_membership types need the group_membership_state field.
    if ($plugin_id == 'group_membership') {
        $group_content_type_id = $group_content_type->get('id');
        // Update all of the existing memberships with a default active state.
        $language = \Drupal::service('language.default')->get();
        $langcode = $language->getId();
        $query = \Drupal::database()->select('group_content', 'gc');
        $query->fields('gc', ['id']);
        $query->condition('type', $group_content_type_id);
        $result = $query->execute()->fetchAll();
        $values = array_map(create_function('$row', "return ['bundle' => '$group_content_type_id', 'deleted' => 0, 'entity_id' => " . '$row->id' . ", 'revision_id' => " . '$row->id' . ", 'langcode' => '$langcode', 'delta' => 0, 'group_membership_state_target_id' => '$group_type_id' . '-active'];"), $result);
        $batch_size = 200;
        for ($i = 0; $i < count($values); $i += $batch_size) {
            $batch = array_slice($values, $i, $batch_size);
            //$query = db_insert('group_content__group_membership_state')->fields(['bundle', 'deleted', 'entity_id', 'revision_id', 'langcode', 'delta', 'group_membership_state_target_id']);
            //foreach ($batch as $record) {
            //    $query->values($record);
            //}
            //$query->execute();
            foreach ($batch as $record) {
              $query = db_merge('group_content__group_membership_state')->fields($record);
              $query->key(['entity_id' => $record['entity_id']]);
              $query->execute();
            }
        }
    }
}

Yeah I have backups thanks, what I meant was that I can't reinstall the module while I have content created by that module. The module refuses to reinstall or uninstall without deleting all groups.

Fortunately I only had a few groups and did delete them all, reinstalled the module and re-created the groups. It seems to be working now, although I can't find any way to get a link to request membership anywhere, and if you click the Related Entities tab I get another WSOD error:

Uncaught PHP Exception InvalidArgumentException: "Unable to get a value with a non-numeric delta in a list." at /var/www/html/web/core/lib/Drupal/Core/TypedData/Plugin/DataType/ItemList.php line 98

I'll probably want to hide that tab anyway, but the ability to relate entities with a UI might be useful for the superuser to have.

Just dropping a note here that the above error appears to be due to membership states. Whatever is building the list of content in the "Related entities" tab is using an ItemList class to store data, but is trying to access a group_membership_state object using a non-numeric index of "state" which throws an InvalidArgumentException.

If I prevent the exception from being thrown in ItemList.php the contents appear to list OK (though I don't see any GroupMembershipState entities in the list.

In any case I don't really want that tab showing up for site users anyway, but I can't figure out how to hide it either. It doesn't appear to be a Views view, so I can't change the access there. There is a Group permission called "Access related entities overview" which sounds like exactly what I need, but group owners can still see that tab even without the Administrator group role ticked.

It's not even listed in the Group module's group.routing.yml file, so not really sure where to modify it, so in the interest of time I expect I might have to resort to hiding it with CSS.

@scott.whittaker
I have the same issue, when I try to access group entities. We should access the state field like this. I will update the patch

$group_membership_state->entity->get('state')

I have added a fix for the problem with access to state field

Uncaught PHP Exception InvalidArgumentException: "Unable to get a value with a non-numeric delta in a list." at /var/www/html/web/core/lib/Drupal/Core/TypedData/Plugin/DataType/ItemList.php line 98

I'm sorry messed up with the previous patch and interdiff. I added a correct one

@LOBsTerr I can confirm that #56 does indeed work for me.

I can confirm that #56 works too... I'm in the process of adding it to the Open Social distribution, however it depends a bit on if and when kristiaanvandeneynde will add this to the module officially.

Any thoughts on that @kristiaanvandeneynde?

+1

#56 works as one would expect. It would be great to see this added to the module.

For those looking to add this feature to a site which has existing group content, you'll need to make sure the module's schema on your site is 8014 or prior and that the database updates run. This was alluded to in prior comments, but the 8015 and after are the essential ones that must run in order to add the membership states.

Adds the correct internal state value for the banned membership state.

Note to anyone already using this patch: you'll have to reinstall the membership states to get this change.

Fixes failing schema validation.

The remaining test failures here are due to #2984750: Tests failing due to new system.site requirement.

Additional use of this patch reveals that it doesn't incorporate the membership states into the module's access logic.

The most visible place this is an issue is when accessing a group permission restricted route. In this scenario, both banned and pending members are given access because they are technically group members. What still needs to be written here is access logic that ensures only active group members are treated as group members.

Updated patch does the following:
- Adds membership state logic to permissions logic with some test coverage.
- Fixes a couple misnamed variables.
- Fixes internal banned state value during GroupType::postSave.
- Addresses some minor code sniffing violations introduced by the patch.

Updates patch with fix from #2984750: Tests failing due to new system.site requirement so tests can pass here.

Explicitly treats a user as an outsider if they are not an active member of a group.

Great work!

Tried the patch in Open Social, there is a lot of request for this and out of the box it already works pretty well functional wise.
Some minor nitpicks in functionality:
- After approving a membership it doesnt make a lot of sense to redirect to the group content, rather go to group canonical maybe (or the place where the user came from) can imagine he wants to do this action for multiple users.
- Already existing members of a group still have the approve membership operation see image (could also be our implementation but worth to check)

Didn't do a code review just yet but this is what I've seen at first glance. Would be great to get this in.

I'm trying to get to grips with this module and this issue in particular as it seems like an obvious feature that group admins should be able to choose what level of access they want for their group.

But from what i've seen so far even this patch doesn't quite do that, it seems like the group type needs to be fixed as an access level based on the permissions that are set for 'Join group' vs 'Request group membership' is that correct?

My assumptions:
If there is only 1 type of group then group creators cannot choose access level
- So you might suggest create 2 different group types, one that requires a request and one that does not. However this isn't ideal because then you need to ensure the group configurations are kept in sync all the times except for the permissions i mentioned above. It's also not ideal as a group admin may create a group wanting it to be open but then at a later date decide they want to close access to requests only and this wouldn't be possible.

Is what i've described correct or am I just missing something obvious?

Thanks

Am interested in this feature for my new site.

I keep getting the same error as @scott.whittaker

https://www.drupal.org/project/group/issues/2752603#comment-12502258

@socialnicheguru

Running the following should resolve that issue by ensuring the membership states are correctly installed:

drush ev "drupal_set_installed_schema_version('group', '8014');"
drush updb

@bobbygryzynger. Thanks. Already tried it. I had the fields already installed which means that the drush updatedb command gave me an error. So after the updatedb I still get the pending error.

@socialnicheguru in that case, it's likely the only remedy is to uninstall the membership states entirely. In reality, that means re-applying the patch and running the database updates on a site where the membership states have not yet been installed.

Great job so far but I'm getting the same kind of error on the isActive() method on null.

If you add a user to the group without picking a membership state, the related entities page of the group and the added user account are broken.

Membership state should be at least required or a default value set on form submit.

I was getting the following error on group/gid/membership:

Error: Call to a member function get() on null in group_entity_operation_alter() (line 317 of drupal-8.6/html/modules/contrib/group/group.module)

The status of the group manager is not set at all. The status for other members is set.

I do not know if the group admin is set.

Once I set the group_manager to active or any status really, the members tab no longer has a WSOD

This is a feature we could definitely use, so will circle back when I have some more time.

I am thinking of simplifying the D7 vs D8 functionality, by introducing a GroupMembershipRequest and GroupInvite plugin that would also allow you to add User entities to a group. If a request or invite is accepted, it would allow the user to create a membership. This way, we can almost 100% re-use functionality that is already there without prematurely having to introduce a membership state system.

As mentioned in #30 and #33 I am still on the fence whether we want to copy it from D7 or give it some more thought. With the above suggestion of using plugins, we wouldn't need a state for "invited" or "requested". So the only ones that I can currently see remaining are "active" vs "blocked" and I'm not sure yet whether we want a block/ban system instead of outright kicking someone from a group.

Many thanks for continuing to work on this!

"In Group 7 we had the notion of membership states; these could be anything ranging from Active to Pending or even Blocked and behind the scenes it would tell Group whether the membership was active or not. I'm contemplating whether we should rebuild that system on Group 8."

It's not obvious that there's a compelling reason to have states as well as roles. If the logic was arranged such that being a member did not itself grant privileges, but instead those all depended on which group role one had, then "Requesting membership" etc. could be a group role.

For those who have the error about group_membership_state
You can try drush entity-updates -y.

The patch is too old and is not applying anymore .
I did a new one, but due to changes, I've drop this part of the code:

// If the user is a member of the group but is not active, treat the user
// as an outsider.
$member = $this->getMember($account);
if ($member && !$member->isActive()) {
      return $this->getGroupType()->getOutsiderRole()->hasPermission($permission);
}

I've tried to keep it, but I am a little lost in the evolution of the permission system in the module.
This commit move the logic to GroupPermissionChecker.php: https://cgit.drupalcode.org/group/commit/?id=7eaa5f73a4eec320284205ce597...
But then this commit remove the code: https://cgit.drupalcode.org/group/commit/src/Access/GroupPermissionCheck...

The last commit confuses me.

Sorry wrong file.

Just have figure out that if the owner leaves the thread it creates an error with an empty status. So I just added a check that the status is not null before calling any method on it.

As @kristiaanvandeneynde suggested, I have dropped the state and created a GroupMembershipRequest plugin instead.

The patch contains a part of the old one but has been considerably reduced since I started again from scratch by writing the plugin.
When you approve someone, there is now a redirection to the members page because redirecting to the group was not really logical as it has been said above.

Feel free to test it and say what problems you encounter. The patch needs tests since the one for the states have been also dropped.

A new view has been added like before, we need to add the hook_update. If someone has time to do it.

Just changed a test now that the group types have 2 installed plugin.
And I added the hook_update to install the new view "pendeing requests" on websites with the module already.

But I can't figure out this error that comes back everywhere in the tests, if someone has an idea it would be really helpful.

TypeError: Argument 1 passed to Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::getTableMapping() must implement interface Drupal\Core\Entity\EntityTypeInterface, null given, called in /var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php on line 1477

@phjou, I have tested. So, far it works. I didn't find any issues.
I have question in the previous patch we had custom membership, which can be added per group. Why we dropped it? It was decided or we just miss it from the current patch?

I also found this case. Anonymous user has rights to request membership, but doesn't have rights to view the group. May we should redirect this users to membership request page in this case ?

Test failures here are probably almost entirely related to #3040047: Kernel tests fail against Drupal 8.7.x and above.

@LOBsTerr I am not sure to understand. Do you speak about the custom state field? As kristiaanvandeneynde said, I thought we wanted to drop that field and use a plugin instead but maybe I have not understood correctly. We can go back to the old patch anyway.

Indeed a redirect to the request page is a good idea if you don't have access to the group.

@bobbygryzynger Thank you for the information, that's good to know.

@phjou previously, there was GroupMembershipState class and we could create custom MembershipStates. I checked your code, it is not a case anymore. It looks like you have decided to simplify things

@LOBsTerr Yes I removed the state because of @kristiaanvandeneynde comment that tells that we can use a plugin instead of a state system. "This way, we can almost 100% re-use functionality that is already there without prematurely having to introduce a membership state system." But maybe I have not understood well.
We can go back to patch #86 which was almost just a reroll of the old patch and go back with the membership state system.

@phjou I believe you have understood the changes that were suggested correctly.

Also, the test failures are no longer occurring against the latest development version.

Thanks for the Patch! #88 works for me. It would be nice though, to be able to choose a role. I could imagine, that the field is shown on both request and approve page or only on the approve page. I don't know if @kristiaanvandeneynde was referencing to this in #6-1

Edit: There seems to be no check if the user is already a member. I can approve multiple requests for a user who is already member.

Hey,

Just fixed the multiple membership requests thing.

But there is still a new problem I have noticed:
With the permission system, you can give or not access to the group page. Consequently, we can't do a redirection to the group page after asking for a membership. Maybe we should stay on the same page by default and make it configurable to redirect to something else?

Just have seen that the group content entity has a method to get the current user, so I improved the syntax by using it.

There is a use statement missing in #98 patch (Drupal\Core\Access\AccessResult). It's in your interdiff though.
Also, I've got a php notice. Notice: Only variables should be passed by reference in Drupal\group\Form\GroupRequestMembershipForm->save() (line 39 of modules/contrib/group/src/Form/GroupRequestMembershipForm.php).

Here is a patch. Pretty much the same as #98.

Is there a chance you'd keep the request around with a different status? It would be nice to have a membership point to the request it was created for so that you can track who approved the member.

I also think that the membership status/state should be easily extendable so anyone can easily add additional ones.
So that the we can easily implement different access conditions and be able to differentiate users in one state or in another.
In the case of our distribution (Opigno lms) we need to differentiate the case when the membership requires an approval from an admin vs when the group has pre requisites that need to be finished before the user becomes an active member of it.
Another example i can think of, the user would get a different state if for example he bought access to it and would need to renew it.

BR

I also think that the membership status/state should be easily extendable so anyone can easily add additional ones.

Yeah, this reinforces my point in #80: what does this feature do that roles don't already do? You're effectively talking about different roles for non-members.

What does this patch accomplish that can't be done more flexibly in by configuring things so that no special privileges on a site are given to members, instead giving the privileges to a member with a particular role like 'approved'?

Please don't use roles to flag things. Not in stock Drupal, not in Group.

Roles have one purpose and one purpose only: To grant permissions to users. Anything else should be flags / boolean fields on the account / whatever. If you don't respect the very thing roles were created for, you're muddying the waters and any code that has to deal with roles will now need to be aware of the undocumented extra behavior that they might imply.

Was thinking about this issue, here is a short diagram which I come up with to clarify some things, which can be obvious for some of us.

So, user have 4 ways to become a member of a group:
1. Group admin can create membership and add user to group directly (part of Group core)
2. User can Join the group if allowed (part of Group core)
3. Group admin can invite user to the group (it implemented here in Ginvite module port https://www.drupal.org/project/group/issues/2801603 )
4. User can request membership (which we are trying to implement here)

What i think missing form latest patch is "Request status" field (pending/approved/declined) and a link to the user who approved/declined request.
If we will have this Status field we will be able to create membership after request is accepted and keep all requests for history (as suggested in #102).
Also I would rather move this functionality to separate module in the Group package, we can call it like 'grequest'. Because really not everyone needs this.

As described in previous comment, adding a patch with a bit different approach.
It uses about 50% of code from patch in #100 but everything moved to a different module, so no interdiff file.

This patch provides working functionality but very basic (just as a previous one).
List of improvements for the future:
- Do not show "Request membership" button if there is pending request already
- Remove calls to \Drupal (dependency injection)
- Notify user somehow when his request accepted/rejected
- Add separate view with requests history
- Add tests

I will try to find some time and work on it further but before that, would be good to hear any feedback from module's maintainer if we are going in right direction here.

I tried the patch in #107 but it broke my install. Can't run entity updates or rebuild the cache due to the error:

In EntityTypeManager.php line 150:
                                                            
  The "group_membership_state" entity type does not exist.  

I managed to run the pending database updates, but even after they had run the error persists and the site is broken.

I managed to get the site running again by hacking the database and manually deleting field.storage.group_content.group_membership_state and field.field.group_content.project-group_membership.group_membership_state from the config table and rebuilding the cache, but entity updates (which is required to uninstall group_content.group_membership_state) fail to run because of the above error.

Been trying to get this working all day, anyone have any success?

Update: rolled back my database and reinstalled the module with the patch in #100. Same error.

I encounter the same kind of error with the patch in #107. I uninstalled and reinstalled the module
+ grequest and I have this error when trying to add an organization that doesn't have the "Group membership request" plugin installed.

Drupal\Core\Entity\EntityStorageException: Plugin ID 'group_membership_request' was not found. in Drupal\Core\Entity\Sql\SqlContentEntityStorage->save()

Ok I just have found the bug, we should first check if the group type have the plugin installed before executing code from the grequest module.

Patch attached with interdiff.

Just did a quick review of the feature:
- The "Membership Requests" tab should not be there for groups where the Plugin for request is not installed
- The "Join Group" operation seems to stay in the block even if the permissions allow only Request Membership for Outsiders

The feature works pretty well otherwise. It is way better to have that in a distinct module :)

I have the earlier patch from #56 running on a production site for several months. This old version prevents me from running updates because the patch adds an update hook group_update_8015 to group.install which conflicts with the same hook added in updates to the group.module.

I cannot update to a newer patch or leave the group module unpatched and abandon the group membership feature because of the "The "group_membership_state" entity type does not exist." error mentioned in #108.

It seems like the only way to fix this is to uninstall group.module and start again, but I cannot do that without losing all my content and views.

Is my only option to freeze group at 8.x1.0-rc2 and never update it again?

@scott.whittaker That's the downside of using patches before they are merged.

First if you want to rerun hook_updates you can still use drush to do something like this
drush ev "drupal_set_installed_schema_version('group', 8014)"
It will allow you to rerun the hook_update 8105 from the module.
Then you have the solution to write your own migration or hook_update in order to copy all your data in the new model. But you will have to write custom code for your project and have a temporary custom version of group in order to have both entity models. It will probably take some time to do.

And the other way is indeed to freeze group, but it is definitely not the best solution because you will probably encounter bugs in the future and the compatibility with Drupal 9 will be compromised.

PS: After the recommendation of @kristiaanvandeneynde, we switched to a GroupMembershipRequest plugin instead of the entity in comment #87.

I see. Well manipulating the code side is easy, since I am using Composer to manage the dependencies and apply the patches, all I have to do is remove the patch or update it to a newer one. I don't mind losing the current functionality or settings provided by the old membership request feature and membership states, but something is clearly expecting the "Group entity state", not finding it, and throwing an exception.

Since the code has been reverted or updated, the reference to the "Group entity state" which is choking it must be in the database. So it ought to be possible to do some database surgery to remove the references to it and return to a state equivalent to not having installed the patch in the first place.

I'm also using exported config, so I could probably make some necessary changes by deleting the associated config and syncing. I just need to figure out exactly what to remove. Unless there's stuff stored in serialized configuration blobs, in which case I'm probably screwed.

Here's a re-roll of #100 that moves the hook_update_N() to a post update - this is both the right place for it since it's only installing config, but also gets around the hook_update_N() conflict. We've been using #100 in production for a couple of months without any major issues.

I haven't had a chance to review/test the #107/#110 patches yet yet. Does it require a data migration from the #100 approach?

It would be good to get some directional feedback from @kristiaanvandeneynde again.

Missing a use statement.

Sorry, and another one.

@catch For me, the patch from tarasich #107/#110 is way better because the feature is separated in a different module. Coming back to the old patch is not a good step.
But if you are using the patch in production and want to stay uptodate with the module, you will probably have to create some data migration.

Hi, there is a bug in patch #100 when you update to 1.0.0-rc4. I upload a reroll patch just modifying the version of function group_update_8016 by group_update_8017 because the 1.0.0-rc4 release come with group_update_8016

It appears as though patches 118 & 120 conflict with the two patches to add subgroup functionality (ggroup_role_mapper-3084153-12.patch and patches/subgroup_core-3084140-6.patch.

I installed each separately on my site already already patched for subgroups and the site became unusable. The error message included

The "subgroup:region" plugin does not exist.

but I know it does because I've been using it for weeks. Some sort of plugin overwriting going on?

I installed 120 on an unpatched site and it's a beautiful thing!! I look forward to using the membership request feature.

I'm pretty new to patches and am not qualified to debug and fix but am willing to do some work. Any ideas on where to start?

Thanks in advance

This issue start becoming unclear. There are two different approaches:
- One integrating the membership feature in a separate module (#110)
- One integrating the feature directly in the main module (#120)

Is there a maintainer that could tell which approach should be used in order to focus our effort on the right one?

@SoTadmin For the approach in a separate module, I made a comment with improvement to do in #111. And if you are new in making patches, you can read that article that will help you to create correct patches: Making a Drupal patch with Git

A number of comments have completely clobbered the issue summary here. When I started reading this issue today, all I saw at the top was:

Just hiding patches using the old way at the top of the conversation to avoid new people to patch and have the same migration problem.

Huh? ;) Stuff like that belongs in the Comment field when adding a comment on issue.

The Issue summary is the summary of the issue, the proposed resolution, remaining steps, etc. It's visible at the top, and folks should try to keep it accurate so people trying to dive in and contribute to this know what's going on and where to focus efforts.

I tried to start repairing this, using the Issue summary template, but there's still a lot TBD, so tagging this for needing further summary cleanup. In particular, I added stubs for the two main approaches proposed in here (separate sub module vs. core group module) but I haven't looked closely enough to fill out pros/cons of each, why folks are advocating for one or the other, etc. If the next step to moving this forward is to decide on an approach (per #123) then we need criteria to make that decision.

Going forward, please don't destroy the summary with your comments. ;) Put them in the Comment field where they belong. And, please update the summary as we get clear on what we're doing here, e.g. to answer pros/cons for each approach, flesh out remaining tasks, etc.

Thanks!
-Derek

p.s. Given that #120 is the latest patch for one of the viable approaches to this issue, I'm restoring it to be visible in the file table in the summary.

The latest commit breaks the patch in #120 because of the hook_update number.

Now with a proper re-roll.

#120 is missing that in #118, the hook_update_N was converted to a post_update. Conversion to post_update seems appropriate. Working to re-roll again from #118.

Here's a re-roll based on #118. It doesn't include the use/import statements to .install file that are no longer needed. But otherwise, its a straight and simple re-roll.

Thanks for moving this forward.
Not sure why the bot doesn't change the status when tests fail here.

We started to implement this feature in open_social project (https://www.drupal.org/project/social) and will use patch from #110 as a basis to make it ready for merging. Any objections/comments?

So the latest patch is *kind of* working now. The Request Membership button works and creates a *Group membership request* entity, but there is no notification sent to the group owner and there is no list of pending membership requests. I can't find any configuration for this.

Do we need to complete the functionality manually by using *Rules* to send emails and making a View of pending membership requests as a block to the Group Members list?

If I manage to do that, how do group admins approve the requests and remove the pending request and send a confirmation email?

[edit] I managed to create a new View of Membership Requests so a group admin can go to that tab and see the requests, but they can't actually approve or deny the membership request from there or do anything at all really. I was also unable to set up a membership request using Rules triggers. Are the workflow features required to make this usable still in development?

Had some discussions w/ @catch, who recently heard from @kristiaanvandeneynde that the patch in #110 was the direction to go with things at this point. To that end, I'm testing things out (I'll report back if it doesn't work). But the following type of post update hook should be all that is needed to migrate the data from the alternative approach. Still to test out the setting of the default values for pending status and what I need to do to the views.

/**
 * Request Membership feature. See #2752603-132.
 */
function example_post_update_request_membership_migration() {
  // Get these ids from your config export for the group_membership_request content_plugin's.
  $group_content_type_ids = [
    'group_content_type_015cc4a34dc19',
    'group_content_type_18aae53e1f391',
    'group_content_type_c7a4e1632ad82',
    'group_content_type_e578fc238bc85',
  ];
  foreach ($group_content_type_ids as $group_content_type_id) {
    // Add Status field.
    FieldConfig::create([
      'field_storage' => FieldStorageConfig::loadByName('group_content', 'grequest_status'),
      'bundle' => $group_content_type_id,
      'label' => t('Request status'),
      'required' => TRUE,
      'default_value' => GroupMembershipRequest::REQUEST_PENDING,
    ])->save();
    // Add "Updated by" field, to save reference to
    // user who approved/denied request.
    FieldConfig::create([
      'field_storage' => FieldStorageConfig::loadByName('group_content', 'grequest_updated_by'),
      'bundle' => $group_content_type_id,
      'label' => t('Approved/Rejected by'),
      'settings' => [
        'handler' => 'default',
        'target_bundles' => NULL,
      ],
    ])->save();

    // Build the 'default' display ID for both the entity form and view mode.
    $default_display_id = "group_content.$group_content_type_id.default";
    // Build or retrieve the 'default' view mode.
    if (!$view_display = EntityViewDisplay::load($default_display_id)) {
      $view_display = EntityViewDisplay::create([
        'targetEntityType' => 'group_content',
        'bundle' => $group_content_type_id,
        'mode' => 'default',
        'status' => TRUE,
      ]);
    }

    // Assign display settings for the 'default' view mode.
    $view_display
      ->setComponent('grequest_status', [
        'type' => 'number_integer',
      ])
      ->setComponent('grequest_updated_by', [
        'label' => 'above',
        'type' => 'entity_reference_label',
        'settings' => [
          'link' => 1,
        ],
      ])
      ->save();
  }
}

The update hook needed to set the default value of "pending" for the newly created field:

    $storage = \Drupal::entityTypeManager()->getStorage('group_content');
    $requests = $storage->loadByContentPluginId('group_membership_request');
    foreach ($requests as $content) {
      $content->set('grequest_status', GroupMembershipRequest::REQUEST_PENDING)
        ->save();
    }

OK here's a new patch using #110 as a base and fixing the membership reject form which was broken, and making a couple of minor grammar fixes.

The one thing that still needs fixing is to enforce the cardinality of 1:1 for membership requests between a user and a group. Currently it does not prevent a user making multiple request entities. On the other hand if an administrator attempts to approve multiple requests from the same user you get an error "User: %username has reached the maximum amount of times it can be added to %groupname" which is coming from GroupContentCardinality.php, so I assume the cardinality enforcement should be possible on the request entity as well.

Every other bit of functionality I've implemented with custom code, mostly using hook_form_alter and views:

• A block on the Group page with a button to request membership.
• A Membership Requests view which shows up as a tab on the Group node which lists pending requests and provides actions to Approve, Deny or Delete the request.
• Alter the membership request form to explain purpose and provide a textarea for a custom message from the requester and redirect to the group page so the user isn't returned back to the request form again. Send an email to the group owner to notify them of the pending request along with the custom request message and a link to the pending requests view.
• Alter the Approve request form to make the user reference readonly and add a textarea for the administrator to modify the default approval message and redirect back to the pending requests page. An email is sent to the user informing them of their approval with the custom message and a link to the group. The request entity is then deleted.
• Alter the Deny request form to add a textarea for the administrator to modify the default denial message and redirect back to the pending requests page. An email is sent to the user informing them of their denial with the custom message. The request entity is then deleted.

I'm happy to share any of this code if anyone wants it.

Interdiff between #110 and #134.

+++ b/modules/grequest/src/Form/GroupRequestMembershipRejectForm.php
@@ -63,20 +63,6 @@
-    $this->groupContent
-      ->set('grequest_status', GroupMembershipRequest::REQUEST_REJECTED)
-      // Who created request will become an 'approver' for Membership request.
-      ->set('grequest_updated_by', $this->currentUser()->id());
-    $result = $this->groupContent->save();

Shouldn't this still exist? Was this an intended removal?

Yes that was removed because it was generating errors and preventing rejections from completing. I don't think that grequest_status exists anymore - or if it does it's broken in my install with the patch from #110.

Correct me if I'm wrong here but it seems the new approach doesn't track request status. Instead an approval simply creates a regular Group Membership entity. Rejecting a membership request doesn't really do anything except notify the user and delete the request entity. In my case both approve and deny notify the user and delete the request entity which I'm doing with form alters.

Even if notification isn't considered core functionality of the module, the automatic cleanup of request entities by deleting them should be, but I kept as much as I could in my module because I don't want to presume too much on behalf of the module developers. I tried my best not to touch the patch code but like I said this functionality was actively breaking and didn't seem to actually do anything useful, so I removed it.

The error message I get with that code included is:

InvalidArgumentException: Field grequest_status is unknown. in Drupal\Core\Entity\ContentEntityBase->getTranslatedField() (line 587 of core/lib/Drupal/Core/Entity/ContentEntityBase.php).

Mind you it could be that my installation is still in a partially broken state. My installation has never fully recovered from the debacle from #112. I seem to have a functional site again but I still can't run Drush entity updates anymore because of the "The group_content.group_membership_state field needs to be uninstalled." item which results in "The "group_membership_state" entity type does not exist. " error. So maybe it's just me?

Sorry @heddn it seems to be my broken grequest installation. I've lost several days battling this module and I've given up all hope of ever getting it to work again with the current module code. I think I will have to freeze it here and stop updating with further patches.

I will have to jump into it again, I wrote the original patch a long time ago already. I am currently busy with lots of other stuff and I have other issues to look first that are more important.

But from what I remember grequest_status is supposed to be the field that stores the status of the request on the groupContent entity, like grequest_updated_by is supposed to store who approved or rejected the request.
So I think the code that has been removed is supposed to be important in the original patch because we keep track of "Rejected by". So it was not supposed to delete the requests that have been rejected. The logic can be changed though, but the patch needs way more changes than just removing that part if you want to stop keeping track of the history of the requests.

Can we define the road map here ? Which approach we are going to use?
I am a little bit lost. I am trying to test existing solutions and contribute, but I see the big differences between

https://www.drupal.org/files/issues/2019-11-21/group-request_membership_...

https://www.drupal.org/files/issues/2020-04-30/group-request_membership_...

I also not a big fan of such big changes without a common agreement.
Let's make it work.

@LOBsTerr yes I'm sorry, please ignore my patch. It was based on changes I needed to make to make it work on my installation which apparently is completely broken due to using an earlier version of this functionality in production. I have no hope of being able to use any future updates here. I expect if you are able to start with a fresh install and use https://www.drupal.org/files/issues/2019-12-20/2752603-128.patch it will probably work properly.

@scott.whittaker Thank you for the answer. I then focus to make #128 work, because it is broken for me.

If you get the same error I mentioned in #137 then maybe it's not just my site? Now I'm even more confused. I'm happy to share my code with you if it helps. The functionality I've built works fine for my purposes as detailed in #134.

I have fixed the issue with the missing method "getCurrentUserId". Owner trait replaces this functionality. Now everything works as before.
Can we now collect missing parts? What should be done next? What we are missing?

What do you think if we move code to a separate module?

@scott.whittaker I've just tested briefly your solution. I need sometime to check, what have been done on your side to adapt to the common solution.

Add a small fix for the approval link in the view. In some rare cases a user will have rights to manage users and approve requests, but will not have permissions to view the group. In this case the link for an approval will be broken.

@LOBsTerr I think that @scott.whittaker told you the wrong thing.
According to what has been decided, the approach on #110 is the way to go according to the maintainer, please look at his comment #132
All the feature is contained in a new submodule called grequest.

The only update made on #110 so far is #134 I think but apparently @scott.whittaker said to ignore it.

re #146: You are absolutely correct. Also see #132. The approach utilized in #110 is the intended direction.

Updated IS.

@phjou and @heddn Thanks for your feedbacks. I will move the code to a separate module based on #2752603-110: Request Membership feature and continue what was done by @phjou taking into account the latest fixes.

I put back the issues that I noticed with the patch #110 since it is very far up in the conversation:

- The "Membership Requests" tab should not be there for groups where the Plugin for request is not installed
- The "Join Group" operation seems to stay in the block even if the permissions allow only Request Membership for Outsiders

@phjou Thanks for information I will check it. why did you decide to switch from entity form routes (like entity.group.request_membership), to custom one (like grequest.request_membership) ?

Update: Found the reason. The ContenEntityForm was replaced with ConfirmForm.

Replaciong ContentEntityBaseForm to ConfirmForm is not ok neithor UX (It sais: This action cannot be undone - which is not true) nor DX prespective (As a developer, I want to add field_accept_terms_and_conditions and field_why_do_you_want_to_join to the membership request entity).

Change entity route is also not ok. As a submodule, it needs to adjust the entity links like this:

/**
 * Implements hook_entity_type_alter().
 */
function grequest_entity_type_alter(array &$entity_types) {
  $entity_types['group']->setLinkTemplate('group-request-membership', '/group/{group}/request-membership');
}

It won't work without entity route.

@szantog Yes, it is a good point. Also, another point to use entity form to use cardinality validation, which is not executed when we save the entity. We need explicitly execute validate method.

I'm still in progress, but very soon I will present an update version of grequest module.

@LOBsTerr: I'm aware that you are working on it, so I don' want to cross you, here is the views integration, if you didn't set it up yet.

Maybe now..

This patch is based on #110, but there are a lot of improvements
1) I used ContentBaseForm instead of basic forms
2) I replaced routes with entity routes (links)
3) I added a view field to request membership
4) I moved a logic from grequest_group_content_insert to forms
5) I added validation for request membership form. Unfortunately default "GroupContentCardinality" validation is applied to entity_id field, but it is prefilled and I hide it. So, default validation will not be executed.
6) I added checks for local tabs and view field, if plugin is enabled or not
7) Set entity_cardinality to 1.

1. +++ b/modules/grequest/grequest.info.yml
   @@ -1,7 +1,9 @@
   +core_version_requirement: ^8 || ^9
   +php: 7.1
   

   This should be: core_version_requirement: ^8.8 || ^9
   to mimic https://git.drupalcode.org/project/group/-/blob/8.x-1.x/group.info.yml#L5.

   Then the php version constrain isn't needed.

2. +++ b/modules/grequest/src/Entity/Form/GroupMembershipApproveForm.php
   @@ -0,0 +1,83 @@
   +    return $this->t('Are you sure you want to Approve this request?');
   ...
   +      $this->messenger()->addStatus($this->t('Membership Request approved'));
   
   +++ b/modules/grequest/src/Entity/Form/GroupMembershipRejectForm.php
   @@ -1,45 +1,39 @@
   +    return $this->t('Are you sure you want to Reject this request?');
   +++ b/modules/grequest/grequest.views.inc
   @@ -0,0 +1,19 @@
   +    'help' => t('Provides an Request Membership link to the Group.'),
   

   Let's be consistent in our capitalization. Can we use a capital first letter of the sentence and lower case for the remaining words? The words "approve", "reject" etc are not proper nouns, so they should be lower case.

3. +++ b/modules/grequest/src/Entity/Form/GroupMembershipRequestForm.php
   @@ -0,0 +1,107 @@
   +  public function validateForm(array &$form, FormStateInterface $form_state) {
   

   Form level validation is only so helpful. Much better if we did it properly. If we are going to all the work of validation, then let's use this method: https://www.drupal.org/docs/8/api/entity-api/entity-validation-api/provi...

   Alternatively, I'd be in favor with stripping it out of the MVP and adding it via a follow-up to keep things smaller and easier to review.

4. +++ b/modules/grequest/src/Plugin/views/field/RequestMembership.php
   @@ -0,0 +1,70 @@
   +  protected $currentDisplay;
   ...
   +    $this->currentDisplay = $view->current_display;
   ...
   +    return FALSE;
   

   Is this ever used? Can it and this entire init method be removed?

5. +++ b/modules/grequest/src/Plugin/views/field/RequestMembership.php
   @@ -0,0 +1,70 @@
   +  public function init(
   +    ViewExecutable $view,
   +    DisplayPluginBase $display,
   +    array &$options = NULL
   +  ) {
   

   Nit: This could all go on a single line instead of wrap onto 5.

6. +++ b/modules/grequest/src/Plugin/views/field/RequestMembership.php
   @@ -0,0 +1,70 @@
   +    // Do nothing -- to override the parent query.
   

   Maybe it would be more clear to say, "Intentionally override query to do nothing."

7. +++ b/modules/grequest/src/Plugin/views/field/RequestMembership.php
   @@ -0,0 +1,70 @@
   +    if ($group->getGroupType()->hasContentPlugin('group_membership_request') && empty($group->getMember(\Drupal::currentUser()))) {
   

   This plugin's parent implements ContainerFactoryPluginInterface, so we could easily just get this via DI instead of calling \Drupal::currentUser().