Port Subgroup (ggroup) to the D8 version

Although, this could be easily enabled by writing a small plugin, the Group 7 version of this did a lot more. I.e.: membership inheritance and synchronization. This is on the to-do-list, but only as an addition after a 1.0 release.

Attached is a patch to get things rolling here. This will allow you to add existing/new subgroups and view an overview off all the subgroups associated with a group.

Not this much code was needed to actually allow subgrouping functionality (the minimum would be basically just the content enabler plugin), but this was created by basically copying the gnode module and changing it to deal with groups instead of nodes, so implements all of the same functionality that gnode does except... (get this)...

...Access control!

So this is put as needs work, because it needs:

- Stronger access control
- Features from the D7 version (membership inheritance, synchronization, etc)
- It should probably be treated more uniquely. Not everything in the gnode module makes sense in the module and vice-versa.

This is still, however, very usable if all you need to do is add groups to groups.

Cheers!
-Justin

This looks more thorough than my contribution at https://www.drupal.org/node/2699287
I'm keen to get this committed because I have a lot to build on top of it.

Hey matslats,

I didn't see that base module that you had committed in that other issue. Sorry! that would have been a good place to start from and it looks almost exactly like the plugin I had written before fleshing it out.

kristiaanvandeneynde, make sure to give matslats some credit if this goes in because his contribution is pretty much exactly what we talked about at DrupalCon.

Right closing the other issue then in favor of this one.

Just to emphasize the instructions I wrote there:

• Start off with a GroupContentEnabler plugin that does nothing more than allow you to add groups to other groups.
• Use a deriver, just like the group_node plugin does to create a plugin derivative per group type.
• In your plugin's configuration form, read the roles from the group type you're on AND from the group type you're targeting.
• Then build a UI that says "People with role X will receive role Y in child groups", perhaps a list of checkboxes per role or a matrix. (table of checkboxes)
• When a person's permissions are checked, see if the group has a parent group and try to read the roles from there. Perhaps this should be recursive so that you can inherit from grandparents and other ancestors too.

That would be a good start!

Adding matslats to the fixed credit receivers.

The conventional way to create new entities here and in gnode in a group is to provide a new entity form to create the entity, then redirect to another form which creates the GroupContent.
This is a weird user experience though. What if we made a CreateEntityForm for every contentEntity?
group/{group}/node/add/story
which inherits everything but in the submit method, it just creates the groupContent

#9 Gets my vote. My users don't care about the relation. I care a lot as the site builder but they just want to create content. It's me that has divided them into groups.
In the current two-step process, they say "Didn't I just do that?"

Well here's how I'm doing it

/**
 * Implements hook_ENTITY_TYPE_create().
 * Take the group from the request and add it as a temp property to the new entity.
 * (Doesn't work for scripted entities e.g. devel_generate)
 */
function hook_myentitytype_create($entity) {
  $params = \Drupal::routeMatch()->getParameters();
  if ($params->has('group')) {
    $entity->group = $params->get('group');
  }
}

/**
 * Implements hook_smallad_insert().
 * read the temp property and create the GroupContent
 */
function hook_myentitytype_insert($entity) {
  if (isset($entity->group)) {
    $props = [
      'type' => $entity->group->getGroupType()-id().'-myentitytype-'.$entity->bundle(),
      'gid' => $entity->group->id(),
      'entity_id' => $entity->id(),
    ];
    GroupContent::create($props)->save();
  }
}
/**
 * Implements hook_smallad_insert().
 * Override the entity's add-form link with a new path which includes the group
 */
function hook_entity_type_alter(&$entity_types) {
  //this new path must be declared in the mymodule.routing.yml
  $entity_types['myentitytype']->setLinkTemplate('add-form', '/group/{group}/myentitytype/add');
}

The need to hide the relationship form is a strong sentiment I want to cater to. The first iteration of that crashed and burned as I realized we would potentially have a security issue, incomplete data and a performance hit on our hands.

Maybe we should open a ticket to discuss the options regarding the workflow in the Group Node module and then copy the solution we found there into Subgroup?

>security issue, incomplete data and a performance hit
Does this apply to my solution in #11?
How does organic groups module do it?

When I tried a version where you could have the form auto-submit itself, I came to realize that links such as /group/X/join would also auto-submit the form, meaning we are open to click-jacking attacks.

So ideally, there needs to be a setting on the plugin that allows you to choose whether you want to use the wizard (2-step) or just the node creation form (1-step). That way, we do not risk having auto-submitting links that could potentially be click-jacked.

I see the problem but you didn't comment on either of my two ideas.
1) Dynamically create a new entityForm for every GroupContent plugin installed with a new path prefix (which informs the route context) then use the submit handler of that extended entityForm to create the GroupContent entity.
2) Create new routes for every GroupContent plugin installed and use the entity create hook to get the group from context, and create the GroupContent with the hook_entity_insert
Both of these approaches involve making a new route with the group in, which means new content can go into a maximum of one group.

Still hoping for answers for #15. But this is a question about the new subgroup module.
There is a global group permission (appears in every group) called Access subgroup overview.
Seems to me it should only appear in groups with subgroups, no?
In which case the permission should be declared in
Drupal\ggroup\Plugin\GroupContentEnabler\SubGroup::getPermissions
and referenced in
Drupal\ggroup\Plugin\GroupContentEnabler\SubGroup::getGroupOperations

I'll have a thorough look at #15 shortly. Probably tomorrow.

Re #15:
1) I would not like to add or generate individual forms per plugin. I'd prefer to have a solid base for every scenario that other modules could then extend or alter. E.g.: CreateContentInGroupForm and GroupContentCreateWizardForm.

2) We just abandoned the path of generating routes for every single group content type (plugin) because it was too annoying to maintain. I'd prefer to see paths such as:

• group/1/content/relate/{entity_type_id} to add existing entities
• group/1/content/add/{entity_type_id} to add new entities to the group

Where the latter path would either use the simple for or the wizard as outlined in my reply to your first suggestion.

The key here is simplicity and reusability. We want solutions that offer good UX in the front end, without sacrificing developer sanity in the process. Having code that's easy to extend or re-use is the key to make the module loved by developers as well.

Re #16, that permission could perhaps be used for a view at group/X/groups. Much like the group node and members view.

I can develop this, but I need clearer instructions.
I don't see in your last comment which way you want to avoid a second stage form.

I would use a plugin config with radio buttons:

When people want to create content in this group they:

• Are presented with just the content form.
  Warning: If you configured fields on the relationship, these will not be filled out!
• Are presented with a wizard to both create the content and fill out the relationship's fields.

Then based on that setting, we could fire up the wizard or the single creation form. Relating content to groups can keep using the form we already have. I.e.: With the entity reference field and the relation's configured fields.

Plugins could then choose which option they want to default to and call it a day. They could even choose to lock the setting and not show the radio buttons if it would break when another method is selected; e.g.: group_membership

OK that's clear
How will 'my_entity's creation form be built for the second option?
Will the original creation form be altered by the ContentEnabler plugin?
Will it add a submit handler which creates the groupcontent?

Please see #2796197-24: Group or Group Node generates some massive queries, we will have to remove hook_group_user_roles_alter(). If you want to move this issue ahead, look at the approach in Group 7. It involves saving memberships and linking those back to the parent that allowed them to exist.

This one just got more complicated sadly, but it's to allow us to have consistent cache contexts and performant grants, so it has to be done.

I don't see the connection, and I'm not going to get involved in that other issue.
While I'm waiting however, and building my own distro on groups, and my own contentEnabler plugins, any directions you can give about how this issue is going to work out would be appreciated.

The connection is that the hook we could have used to make Subgroup a really simple and streamlined module, will have to be removed.

I'm interested in contributing some development of the subgroups feature. We need it for a project we're working on. Has anyone made any additional progress toward kristiaanvandeneynde's outlined approach in comment #7 starting with jludwig's module in comment #3? I'd like to start from whatever has the most progress toward the functionality.

I'm interested in contributing some development of the subgroups feature. We need it for a project we're working on. Has anyone made any additional progress toward kristiaanvandeneynde's outlined approach in comment #7 starting with jludwig's module in comment #3? I'd like to start from whatever has the most progress toward the functionality.

The code I added in #3 did the following out of #7:

• Start off with a GroupContentEnabler plugin that does nothing more than allow you to add groups to other groups.
• Use a deriver, just like the group_node plugin does to create a plugin derivative per group type.

It does some more, but does not approach these:

• In your plugin's configuration form, read the roles from the group type you're on AND from the group type you're targeting.
• Then build a UI that says "People with role X will receive role Y in child groups", perhaps a list of checkboxes per role or a matrix. (table of checkboxes)
• When a person's permissions are checked, see if the group has a parent group and try to read the roles from there. Perhaps this should be recursive so that you can inherit from grandparents and other ancestors too.

Great to see other people working on this. Thanks guys!

I just wanted to say that I do plan on starting in on this effort either tomorrow or Monday. We had to focus on getting our Group setup configured first.

Ggroup module can not be enabled if there are no groups on the site. Basically form installation profile.

It returns:

Exception: No entity type for field type on view subgroups in                                                            [error]
/var/www/dv/docroot/core/modules/views/src/Plugin/views/HandlerBase.php:697

Ggroup module can not be enabled if there are no groups on the site. Basically form installation profile.

It returns:

Exception: No entity type for field type on view subgroups in [error]
/var/www/dv/docroot/core/modules/views/src/Plugin/views/HandlerBase.php:697

Aye. gnode has the same issue if no content types have been defined.

Has anyone submitted a bug report for that?

Kristiaan, wondering if you have any additional thoughts on how this should work? I wrote some ideas below. Interested in what you agree or disagree with.

Here is a thought experiment concerning groups, subgroups, and user membership. If Group B is added as a subgroup of Group A, then all members of Group B can be said to be indirect members of Group A (agreed?). Should $group_a->getMembers(); return a flattened list of all members of Group A and Group B (and descendant groups, recursively)? What about the other way around? I am thinking $group_b->getMembers(); shouldn't return Group A members because membership is recursive descent? Roles could be recursively mapped into the roles of Group A.

I looked at the Drupal 7 version of ggroup and it seems to have an inheritance concept whereby group_membership entities are created based on inheritance configuration. I'd like to avoid anything like that and make the behaviors of subgroups as seamless and intuitive as possible. There seems to be a lot of fuzziness in how certain things could be handled.

Basically, I can come up with some behaviors based on how we'd like it to work for our use case, but if we're going to contribute back I'd like to build something that the community could use.

@CalebD IMO, that's a tough one. I, for one, would not want the children to inherit the memberships because I would want to know exactly who is in that specific child. No more, no less.

A good example is the university setting. You wouldn't want every single person in a department to be a part of the classrooms associated with that department, right? Otherwise, that department member might be in dozens of groups, receive emails for dozens of groups, etc, and you wouldn't know if they were really a part of that group or a part of the staff overseeing the classroom.

Permissions, on the other hand, make sense. The members of the department, with the right permissions, should be able to see basically anything in any classroom that is associated with the department.

If we cannot comet to an agreement, I would at least argue that this should be configurable. Both functionalities make sense in certain circumstances.

@jludwig, in that scenario if the department is a group and the classes are each a group, and the class groups are part of the department group, then $department->getMembers(); would return a list of everyone directly in the department group as well as all the class members, while $some_class->getMembers(); would return only the members of the class because what I was proposing is that membership is only inherited by descent. Or am I misunderstanding your example? Either way, I do agree that direct inheritance may not be desired, or there needs to be a way to disambiguate between direct and indirect members. Maybe it is a configuration option on the plugin?

My use case is using the gnode to control access to content. We have a lot of different groups of users. To grant access to some content, instead of adding a node to all those groups, we'd prefer to add the node to a "grouping" group, one that has all the user groups added as subgroups. Then in the future if we have another group of users who need access to that content, we just add their group to the grouping group as a subgroup and those users would be given the same node access permissions.

Parent "grouping" group ← node added to this group, permissions grant members view access
    Child subgroup ← users added to this group

Given that you created the initial D8 version of ggroup, what was the use case?

One approach I am thinking about is having ggroup maintain the hierarchy data of the subgroups. GroupMembershipLoader would be modified to fire events when loading memberships giving other modules a chance to react. ggroup would react by looking at the given group, finding the proper content plugin, checking if an "inherit membership" option is enabled, and if so pulling group membership for all descendant groups in the hierarchy to add to the event data.

Group hierarchy would be stored by having ggroup create a text field on the group content type that would store hierarchy data using the "path enumeration" technique (similar to how Drupal stores comment thread structures). Querying against that field should allow for efficient joins to grab the membership of all descendant groups given a starting group.

Interested in feedback. Thanks!

I realized my use case is more complex than I originally realized. Because a group can have any number of child groups and a group can have any number of parent groups, the result is a graph structure. Unfortunately relational databases don't excel at dealing with graphs. I'm currently researching approach ideas.

As an aside, I recognize my use case is probably much more complex than others. We're wanting to build a flexible access control system and groups within groups is a core component of that flexibility. I don't want to derail anyone else's vision of how this could be implemented, so I'm still interested in input from others.

I'd appreciate if we could have some code committed here, just to be going on with. I've built a lot on jludwig's patch and I've things to add.
- a block showing subgroups of the current group
- a devel_generate base plugin

One approach I am thinking about is having ggroup maintain the hierarchy data of the subgroups. GroupMembershipLoader would be modified to fire events when loading memberships giving other modules a chance to react. ggroup would react by looking at the given group, finding the proper content plugin, checking if an "inherit membership" option is enabled, and if so pulling group membership for all descendant groups in the hierarchy to add to the event data.

This would be a good way to deal with things. We should avoid having code in the Group class that knows there may be subgroups. Firing events from within the GroupMembershipLoader would be great.

That said, I do not think we should have an unconfigurable inheritance system. We now have the benefit of configurable plugins, so let's use those to their best abilities. The UI we have in place in D7 could use some UX love, but it's a good start.

Like in D7, synchronizing memberships so that we actually have them in the database is a good solution too. So far, relations have always been top-down but do we also want to cater to bottom-up?

A teacher in a faculty should get privileges in all class rooms there (top-down). But do we also want to let students from those class rooms see a faculty-only message (bottom-up)? In that case, we need to be careful that inherited memberships do not trigger an endless loop.

Sound to me like the subgroup Enabler plugin needs to have an option to inherit membership from the parent or to create memberships in the parent or do nothing.
I would be happy to add that feature if you give me the go-ahead.

Please look at the way the D7 ggroup module does it to get an idea of what that inheritance option may look like. But yeah, feel free to work on something already :)

I've installed d7 and looked at it. I see how the roles inherit.
What I was offering to do was create the mechanism for passing members up and down the hierarchy when groups are created, which is what CalebD was talking about.
Shouldn't that be done first, then it can be extended to handle roles other than 'member'?

matslats, I'm not sure if you've started any work on what you mentioned, but I've made progress on my own approach for subgroups. The code needs a bit more work to support basic functionality. I ended up figuring out my own approach based on our needs rather than look at D7 ggroup. I wanted a solution that could quickly determine group membership via a hierarchy without having to synchronize membership content entities into each group. It also needs to prevent circular references in the hierarchy (Group A has Group B has a subgroup, Group B has Group A as a subgroup, and any other extended derivative).

Once I have the rest of the basics working I'll post a patch for others to look at. Either later today or tomorrow.

@CalebD - Waiting for the patch :) . In our project we need subgroup module.

Here is my work in progress on subgroups. THIS PATCH IS A PROTOTYPE AND IS NOT READY FOR PRODUCTION USE! This patch is built on the work started by jludwig. What follows is an explanation of what it currently does and how.

First I'll explain the functionality I'm solving for in my approach. My need is around group hierarchies and memberships. Members of subgroups should be considered as indirect members of the supergroup (parent group). For example, given group A and group B, such that group B is a subgroup of group A, then all direct members of group B are indirect members of group A. Now extend that relationship and behavior out to arbitrarily complex group hierarchieries, like this one. Upon doing so we realize that such a hierarchy can be described as a graph.

Other things to consider:

• How can we quickly know a group's hierarchy without walking the graph each time?
• What if group A has a subgroup of group B and group B also has a subgroup of group A? That's a circular reference that makes it impossible to resolve the hierarchy. So we need to prevent that from happening.

With more thought, we realize that a group hierarchy is a specific type of graph, called a directed acyclic graph (a DAG). To store the group hierarchy effectively, we need to store and traverse a DAG quickly. Unfortunately hierarchieries are not something that relational databases excel at storing and traversing. There are a variety of approaches for storing trees, but a DAG is more complex. Research brought me to this great article, which proposes a way to store the graph structure in a single table and query relationships quickly at the expense of storage. This is because the approach uses the graph structure to create a relationship between every connected entry in the graph, e.g. if group A has subgroup B, which has subgroup C, then a row will be added connecting A to C. Simple queries can be used to answer questions like "What are all the subgroups of this group?" or "What are all the supergroups of this group?". This is good for performance.

In studying the code for the group module, my assesment was that the best way to implement my needed functionality would be to modify the \Drupal\group\GroupMembershipLoader so that memberships could be influenced by other providers. Drupal core uses events in a few areas for this purpose, for example with building and altering routes. The need to "build and alter" group memberships is similar. This approach allows modifying group membership transparently so that other modules, like gnode, do not need any modification.

So, in this patch the ggroup module defines a table, group_graph, that is used to store the group hierarchy graph. This information is in addition to the group_content subgroup entities that are created. When a subgroup entity is inserted, the group_graph table is updated with the group hierarchy. There is a service class, \Drupal\ggroup\Graph\SqlGroupGraphStorage which handles all the database work, and a more general service class, \Drupal\ggroup\GroupHierarchyManager which is a wrapper around the storage class and is what other classes should use to interact with the group hierarchy. A constraint is added to group_content subgroup entity type so that circular group hierarchieries cannot be created.

The patch also modifies GroupMembershipLoader to dispatch events when loading memberships, as mentioned above. The ggroup module implements an event subscriber which uses the GroupHierarchyManager service to pull group hierarchy. The event subscriber creates transient group_content membership entities which are added to the membership collection provided by the events. The transient group_content membership entities represent indirect memberships in a group and are not saved (though they should probably be cached but aren't currently).

With the patch, this is what you can do:

• Thanks to jludwig, you can enable plugins that allow groups to be added to groups.
• When a group is added as a subgroup, the group_graph table is updated with the proper hierarchical representation.
• When requesting group membership, the hierarchy is used to automatically create indirect memberships based on the group hierarchy. This affects anything that looks at group membership, like gnode.
• A service class, GroupHierarchyManager, can be used to easily query for subgroups and supergroups.

Here is what is missing from this patch:

• TESTS - I haven't written any additional tests. My testing has been manual so far and focused on the core concept and implementation.
• Optimization - There isn't any caching of indirect memberships. There could also be various slow blocks of code.
• Roles - Anything to do with roles and role mapping is not implemented.
• Probably a bunch of other stuff I'm not thinking of.

Some other thoughts:

• Need general code cleanup and reivew. There are some places that are straight up copy and paste of code to get something working.
• GroupMembershipLoader dispatches three different events. I wonder if this could be simplified?
• I don't like how many of the classes I added are named. For example one of the event classes is GroupMembershipLoaderByUserGroupEvent, which is really long. Any of the names of classes, methods, could and should be changed.
• I'm not sure of the licensing implications of using the approach outlined in the CodeProject article. The approach in this patch does not use any of the stored procedures but does use the same logic implemented in PHP. The table structure is the same.

I invite others in this thread to take a look and provide feedback. Thanks!

Here is a very small update to my previous patch to fix a bug I noticed with the event subscribers. I haven't had time for further development unfortunately.

I'm working with a much earlier version and it is functional.
The module mostly declares group as a groupcontent type.
There's a lot of stuff about inheriting permissions not done yet.

I installed the latest patch but sub-group not inheriting any content and permission.

I installed the latest patch but sub-group not inheriting any content and permission.

@kaizerking PFA complete group module with sub group.

I tried the patch in #46, but I'm getting the following error when attempting to add a subgroup to a group:

Recoverable fatal error: Argument 1 passed to Drupal\ggroup\GroupHierarchyManager::groupHasSubgroup() must implement interface Drupal\group\Entity\GroupInterface, null given, called in ../modules/contrib/group/modules/ggroup/src/Plugin/Validation/Constraint/GroupSubgroupConstraintValidator.php on line 71 and defined in Drupal\ggroup\GroupHierarchyManager->groupHasSubgroup() (line 46 of ../modules/contrib/group/modules/ggroup/src/GroupHierarchyManager.php) #0 

As far as I can see, it has to do with $child_group being null when passed to

if ($this->groupHierarchyManager->groupHasSubgroup($child_group, $parent_group)) {
      $this->context->buildViolation($constraint->message)
        ->setParameter('%parent_group_label', $parent_group->label())
        ->setParameter('%child_group_label', $child_group->label())
        ->addViolation();
    }

but I'm still too new at Drupal 8 to figure out if this is supposed to be a valid use case or if I've done something wrong. Things seem to work if I add a check for !is_null($child_group) into the if statement.

oo0shiny, I'm not sure how you would have triggered that error. Did you install and configure the subgroup group content type by going to the "Set available content" option for your group type(s)? Sounds like there is a bug in here somewhere that will need to be hunted down.

Updated patch to work with 1.x-dev. Still needs tests and general architectural discussion.

Here some draft version of integration ggroup with views.
Added filter to select which content to show in depends of subgroup configuration.
https://www.dropbox.com/s/annnjje56pl8y60/%D0%A1%D0%BA%D1%80%D0%B8%D0%BD...

Little fixes.

Added support for inheriting permissions from supergroups. Patch is attached. Also fixed the issues below. Please review!

1. +++ b/modules/ggroup/src/Form/SubgroupFormStep1.php
   @@ -0,0 +1,102 @@
   +  public function __construct(EntityManagerInterface $entity_manager) {
   +    parent::__construct($entity_manager);
   +    // @todo use proper dependency injection for this.
   +    $temp_store_factory = \Drupal::getContainer()->get('user.private_tempstore');
   +    $this->privateTempStore = $temp_store_factory->get('ggroup_add_temp');
   +  }
   

   Fixed this.

2. +++ b/modules/ggroup/src/Form/SubgroupFormStep2.php
   @@ -0,0 +1,112 @@
   +    parent::__construct($entity_manager);
   

   The parent constructor needs $temp_store_factory as well. Fixed this.

Fixed small mistake.

Ignore my above patches. Supergroup permission inheritance will now take place in the group entity hasPermission() function.

Also in future we should think about permission propagation.
Like upward/downward/both.

For some cases we need logic to see region content if we have country membership or vice versa if we have region membership see country content.

May be we can have this option in subgroup content enabler plugin.

Any thoughts ?

There is a valid case for upward/downward permission propagation. Having this as optional makes sense (in both cases). Not sure yet on the best place to put this, I will need to dive in the code a bit more. When checking access we have a group and a account. We should be able to fetch these settings from a group.

Sidenote: This could make the process of checking where the permissions are coming from pretty complex. It is easier to make a mistake so we should add some kind of warning to make sure people don't activate this without thinking about it.

Hi,

I apply patch #64, but plugin GroupIdDepthwrong work.

I set option in filter:

But SQL query is wrong:

Fix apply patch

If we remove group which is subgroup the relation is not delete. In method removeSubgroup $group_content->getEntity() return null but $group_content->entity_id is not empty.

Fix apply patch

Add test

+++ b/modules/ggroup/config/optional/views.view.subgroups.yml
@@ -0,0 +1,818 @@
+dependencies:

+++ b/modules/ggroup/src/Plugin/GroupContentEnabler/Subgroup.php
@@ -0,0 +1,136 @@
+    $config['info_text']['value'] = '<p>By submitting this form you will add this subgroup to the group.<br />It will then be subject to the access control settings that were configured for the group.<br/>Please fill out any available fields to describe the relation between the subgroup and the group.</p>';

Option info_text was removed in issue.

Just a minor fix, ensure user does not belong to a parent group before assigning it.

I don't have time to focus on this so I am unassigning myself. However, thank you to everyone who has contributed to improve the patch!

I decided to take the first step towards compatibility the group_graph table with the views module.

Is the latest patch (#85) based on the code (and associated logic) added by CalebD in #45, or something else?

A quick look through the comments makes it look like multiple people were submitting multiple patches based on different starting points.

FYI - I've installed #85 against the latest dev release w/out errors. I'm testing now to see if everything works as it should, but I can't tell exactly what features to expect / test...

Thanks to everyone who has worked on this so far! I'm working on multiple project that could use subgroups and this is all looking very promising.

Hi @dang42.

Yes, this patch is based on the patch #45, you could verify this following the interdiffs chain.

Merge changes from #85 to our global patch (@spheresh please take a look here https://www.drupal.org/patch).

@dang42 expect/test:
1) CRUD for subgroups like content enabler plugins for other groups.
2) Permission inheritance: Now only from top to bottom.
3) Views integration: contextual argument to show selected levels of subgroup
...

Other contributors feel free to update this list.

We currently have a use case where there are different role that you want to propogate:

• Member: Inherit membership and permissions (up)
• Admin: Propogate membership and permissions (down)

To fix this we could:

• When adding a group as a subgroup, we should see a list with all roles defined in the parent. For each role we want to be able to map a role in the parent group to a role in the subgroup and also map roles in the subgroup to roles in the parent group.
• The membershiploader should recursively check all subgroups/supergroups to check if the membership for that group should be added based on the mapping.
• In hasPermission() for a group, we should recursively check all subgroups/supergroups to check which roles the user has for each group. If the role is mapped, we should check if the role has the permission we are looking for.

This leads me to believe that when you create enable a group as a subgroup, you should be able to define how to propogate each role down, and how you propogate each role up. As an example:

• We have a school group type with roles member/admin.
• We have a cluster group type with roles member/admin.
• We have a study group type with roles member/admin/teacher.
• When adding cluster as a subgroup of school:
  • we can make sure the admin role of school is mapped to the admin role in cluster
  • we can make sure the member role of cluster is mapped to the member role in school
• When adding study as a subgroup of cluster:
  • we can make sure the admin role of cluster is mapped to the admin role in study
  • we can make sure the member role of cluster is mapped to the member role in study
  • we can make sure the member role of study is mapped to the member role in cluster.

This way:

• When you are added as a member to a study you are automatically a member of the cluster and the school.
• When you are added as a member to a cluster you are automatically a member of all studies and member of the the school.
• When you are added as a admin to a cluster you are automatically a admin of all studies.
• When you are added as a admin to a school you are automatically a admin of all clusters and of all studies.

I will work on this next week.

Just letting you know that this issue is a goldmine for different points of view. When I get to a 1.0 and want to look into making subgroups part of the main module again, all of this will be really helpful. Thanks all!

Just finished implementing inheritance as mentioned in #93. You can now specify inheritance/propagation for each installation of a subgroup. This allows a lot of flexibility (as already mentioned in a bunch of comments).

In doing this also fixed the following:

• Improved the alters for the membership loader. Since the group roles are now properly inherited, we no longer need to change the hasPermission in \Drupal\group\Entity\Group. Much better!
• Now using the proper database API's in SqlGroupGraphStorage
• A whole bunch of code style/documentation fixes.

Things still todo:

• Fix new line issues in the patch
• Write tests. The whole graph storage could probably use some. The inheritance needs a bunch of tests. The alter events as well.
• Fix cache invalidation. When adding/removing subgroups or members, this could change access or other things in parent/children. This probably means we have to add cache tags for all parents/children to a group, and could mean that cache is invalidated a lot.
• Use inheritance in views. Not sure what the best way is to fix this, since the inherited group membership are not actually stored in the database. Maybe a default argument to provide all group IDs for a user?
• The new events for the membershiploader are nice, but we probably need a way to fetch only the direct memberships too.

Please test/review, feedback would be very much appreciated :)

*edit: Apparently I missed a bunch of missing new lines in the end of files. Adding this to the list.

One important issue comes up: You can't add a user to a group if it already inherits roles for that group. For instance, when a user is an admin, but also a regular intranet user, you want admin permissions on all groups, but member permissions on only some.

As mentioned in my last comment. The new events for the membershiploader are nice, but we probably need a way to fetch only the direct memberships too.

I see several ways to do this, but all have some limits:

• We add a context to the membershiploader methods and pass this to the event from places where it might be logical to conditionally alter the memberships (for instance in Group::getMember(), Group::getMembers() and GroupRoleStorage::loadByUserAndGroup()). Not sure I'm a fan of this, but it's the best I can come up with for now.
• We add new events to places like Group::getMember(). It would do the trick, but I'm not sure if we want modules to alter values in all those places.
• We add a separate method to fetch unaltered memberships. The means for Group::getMember() we decide if the inherited members are included or not, and there is no way to change this. I think we should be a bit more flexible though. We could also provide a setting, but I could imagine situation where you want to influance this.

While the patch is probably easy, it's probably best to discuss what would be the best solution first. If anyone has any other ideas, I'm very open to suggestions.

Ok last one. It seems like the inherited memberships could potentially cause issues since they are not in the database. You can't leave a group for example. The membership also can't be removed. This leads me to believe having a separate method to load the unchanged membership is probably best (after looking through the code we probably only need this for Group::getMember()). We could mark it as internal so we don't expand the API and discourage people to use this.

Patch is attached, also fixed the newlines I missed before.

After testing this in an application with thousands of groups it was obvious that there were huge performance implications. The main reasons were doing queries for each path/relation. To fix this, I added static caches with the data we need to find the roles we want to inherited. This means a bit more memory is used, but is still a lot faster than all queries (5431 groups and a user being member of all of them, the request time went from 11732ms to 950ms on my local machine).

Getting the group type ID from the group content is now solved in a very hacky way :(. We need this to fetch the implied roles. This should still be fixed. Maybe it's even better to not inherit implied roles at all and remove the code?

Hey all,

I ended up using OG for rather than the Group module because of a specific problem that needed Nodes as the group type.

I still love the Group module, though, and would like to help out when possible.

For this particular problem on the OG side, we are _not_ going to attempt to solve the role inheritance problem in the module itself.

Too many people have too many opinions and there are too many use cases, that it ended up being a better use of time to give an example of how to add some custom business logic to fulfill specific inheritance requirements rather than trying to provide some generic functionality that will 100% satisfy no one's needs.

My suggestion would be:

- Make subgroups only involve posting groups into groups
- Provide some helpful APIs for subgroups
- Create a separate subgroups role inheritance module that is semi-generic; essentially just choose one of the solutions here, knowing it won't solve everyone's use case
- Label that module as an example of how to implement role inheritance, acknowledging that it is a difficult problem and that one may need to develop their own business logic for their specific need

@kristiaanvandeneynde you looked like you were flirting with this conclusion in #94. What do you say?

@jludwig The way I currently implemented inheritance is as flexible as it comes. You can specify inheritance for every single relation between group types (up and down). This basically allows people to configure whatever they need.

The added hooks for the membership loader provide extra options to:

• Add extra memberships for whatever situation might comes up
• Stop the events we provide for any situation by implementing your own custom event

This leads me to believe that we have very powerful options in the current state of the patch, and enough flexibility to change the current behaviour and/or extend it. Besides that, configuring inheritance is optional.

I do agree that we can't implement every possible use case, but I think basic up/down inheritance and the custom event should cover it.

@seanB

Looking at what you provided, it does seem like the the up/down inheritance is flexible enough to fit most use cases. Great work!

I would still lean towards keeping this separated from the module that simply allows adding groups to groups to allow people wishing to implement their own business logic to do so without needing too much "duct tape" code needed to override certain parts that might be disruptive in some way for their specific use case.

We had a BoF on group role/permission inheritance at DrupalCon Baltimore, and your code looks like it can solve over half of the problems that were in that room, but if it is too closely coupled to the simple ability to add groups to groups, it may make the remaining 40% need re-implement that feature before getting to their own inheritance business logic.

On a side note, I may snag some of your code for the group inheritance stuff that BioRAFT is doing. :)

I was using the patch in a application with thousands of groups. And while I managed to fix performance for a lot of cases, there seem to be some cases where this whole membership inheritance is just messed up.

For instance:

• All groups are created by an administrator, so the admin is a member of lets say 5000 groups
• We enabled role mapping for related group types (for now a hierarchy with 5 levels)
• When we do a GroupMembershipLoader::loadByUser() we get 5.000 memberships.
• In the event to alter the memberships, for each of the 5.000 memberships we now have to check which roles we inherit from 1 or more supergroups or subgroups because there could be some roles missing that we might inherited for each membership.

The solutions seems very nice, and might work very well for small numbers of groups, or if a user is only a member of a small number of groups. You could say that no user actually has to be a member of all group (since we can inherit), but since we can't actually controll that, we are pretty much screwed.

All issues I encountered were caused by messing with the membershiploader. I think if we use the inheritance only for the permissions (an not actual membership) we save us a lot of trouble.

What did I change:

• Removed loadUnchanged from #97
• Removed membershiploader eventsubscribers.
• This also means we can remove all events on the membershiploader, since they are technically now unrelated changes? I do like the groupMembershipCollection though! We should probably create a seperate issue for the events and the membership collection.
• Add a new event to set a permisison for a user and group. We use this to check for permissions inheritance in subgroups/supergroups.

I think this makes the solution a lot simpler, which might address the feedback from #101

Latest patch gives an exception on installing the submodule, but seems to "work" anyway.

Drupal 8.3.2

Exception: No entity type for field type on view subgroups in Drupal\views\Plugin\views\HandlerBase->getEntityType() (line 697 of /home/dcjf1/www/core/modules/views/src/Plugin/views/HandlerBase.php).

I have found an error with group permissions for creating subgroups. The access check in class SubgroupAddAccessCheck builds the permission string using the old permission name structure:
$access = $group->hasPermission('create ' . $group_type->id() . ' subgroup', $account);
but this should be:
$access = $group->hasPermission('create subgroup:' . $group_type->id() . ' content', $account);
to match the current structure of group permission names.

The attached patch fixed the problem.

Playing with the patch in #104. I didn't set up any mapping when enabling the ggroup module. So it's not inheriting any members of my parent space. But I can't see a way to go back and edit those role mappings. Is there a URL path for that? Or not yet implemented in the patch?

The roles between groups can be mapped on the page where you install / edit subgroups in a group. For example, when having a parent group type named 'department' and a subgroup type 'team', it's the path admin/group/content/manage/department-subgroup-team.

The patch currently only allows the inheritance of permissions (by mapping roles). Actually inheriting membership is another story and can lead to serious performance issues. See #102.

@seanB thanks, found it finally (Group Types -> select Set Available Content -> Configure for the subgroup).

I have a "Top Level" group and a "Subgroup" group. I have Member permissions in both set to Create Article. I have a "testuser" who is a member of "Top Level". I have the role mapping "MAP GROUP ROLES TO SUBGROUP ROLES" setting Member to Member.

When I log in to "testuser", they can create Articles in the Top Level group, but they cannot create anything in the Subgroup.

Are you saying that I still need to manually add "testuser" to *also* be a Member of the Subgroup? If so, that defeats the purpose of subgroups to me. Trying to implement Atrium-like functionality with Group and the whole idea is to have parent groups and putting content-creation users in the top-level parent so they don't have to be manually added to all 16,000 subgroups (stores) on the site.

I understand the issues you raised in #102, but those are exactly the issues that need to be addressed for proper subgroups. Just handling permissions without handling membership is only half the problem.

In og_subgroups we distinguished between "assigned members" and "inherited members" to deal with this (and yes, still had to fix performance issues, but got it working fine with 16,000 subgroups). I'll have to take a closer look at this patch to see if I can understand your architecture better.

@mpotter

Are you saying that I still need to manually add "testuser" to *also* be a Member of the Subgroup?

That shouldn't be needed? I did find I sometimes need to clear the cache? I have the exact same mapping and it seems to work for me?

I understand the issues you raised in #102, but those are exactly the issues that need to be addressed for proper subgroups. Just handling permissions without handling membership is only half the problem.

I totally agree. The issue is complex though since we allow role mapping down/up. You don't just inherit membership but you actually inherit a specific role (or roles). To make it more complex you can not only inherit a role from a parent group, but also from one of your children. If we find a way to solve this I 100% think this should be added.

The biggest problem is that when you are building the list of all the users memberships (inherited or not) you need to check each and every group and fetch the related parent/subgroups for those to see if there are groups to inherit. That is a big problem when you are a member of a lot of groups. If you have some insight on how to deal with this please let me know!

IMO the way it is doing it now is acceptable for going into the core Groups module.

I'd like to reiterate what I said in comment #99, that it may be wise to not provide a specific inheritance implementation because there are so many varying use cases for subgroups, and providing an API flexible enough to deal with even half of them would probably perform terribly.

This is one of the few examples where I think it is better to _not_ provide a complete solution and to allow developers to implement the inheritance business logic needed for their own use case.

If there is a simple example that can be added so that pure sitebuilders at least have _something_, I would suggest that it be a separate submodule and an example of how a developer can implement inheritance.

Hi everyone, I applied the patch in #104 and while I didn't get any errors I am not able to create any subgroups. I understand this is a work in progress -- so any guidance would be greatly appreciated.

I am able to activate the Subgroups module and then configure my group type as "available content". However, attempting to create a subgroup from the group page I am directed to a page (URL: group/1/subgroup/create/mysubgroup) that simply says: "The website encountered an unexpected error. Please try again later."

The error logged is below, and I have been unable to determine where to start in addressing it:

Recoverable fatal error: Argument 1 passed to Drupal\group\Entity\Form\GroupForm::__construct() must be an instance of Drupal\user\PrivateTempStoreFactory, instance of Drupal\Core\Entity\EntityManager given, called in /home/mysite/mydomain.com/drupal8/modules/group/modules/ggroup/src/Form/SubgroupFormStep1.php on line 33 and defined in Drupal\group\Entity\Form\GroupForm->__construct() (line 33 of /home/mysite/mydomain.com/drupal8/modules/group/src/Entity/Form/GroupForm.php) #0 /home/mysite/mydomain.com/drupal8/core/includes/bootstrap.inc(566): _drupal_error_handler_real(4096, 'Argument 1 pass...', '/home/mysite/de...', 33, Array) #1 /home/mysite/mydomain.com/drupal8/modules/group/src/Entity/Form/GroupForm.php(33): _drupal_error_handler(4096, 'Argument 1 pass...', '/home/mysite/de...', 33, Array) #2 /home/mysite/mydomain.com/drupal8/modules/group/modules/ggroup/src/Form/SubgroupFormStep1.php(33): Drupal\group\Entity\Form\GroupForm->__construct(Object(Drupal\Core\Entity\EntityManager)) #3 /home/mysite/mydomain.com/drupal8/modules/group/modules/ggroup/src/Form/SubgroupFormStep1.php(44): Drupal\ggroup\Form\SubgroupFormStep1->__construct(Object(Drupal\Core\Entity\EntityManager), Object(Drupal\user\PrivateTempStoreFactory)) #4 /home/mysite/mydomain.com/drupal8/core/lib/Drupal/Core/DependencyInjection/ClassResolver.php(28): Drupal\ggroup\Form\SubgroupFormStep1::create(Object(Drupal\Core\DependencyInjection\Container)) #5 /home/mysite/mydomain.com/drupal8/core/lib/Drupal/Core/Entity/EntityTypeManager.php(187): Drupal\Core\DependencyInjection\ClassResolver->getInstanceFromDefinition('Drupal\\ggroup\\F...') #6 /home/mysite/mydomain.com/drupal8/core/lib/Drupal/Core/Entity/EntityManager.php(82): Drupal\Core\Entity\EntityTypeManager->getFormObject('group', 'ggroup-form') #7 /home/mysite/mydomain.com/drupal8/core/lib/Drupal/Core/Entity/EntityFormBuilder.php(44): Drupal\Core\Entity\EntityManager->getFormObject('group', 'ggroup-form') #8 /home/mysite/mydomain.com/drupal8/modules/group/modules/ggroup/src/Controller/SubgroupWizardController.php(131): Drupal\Core\Entity\EntityFormBuilder->getForm(Object(Drupal\group\Entity\Group), 'ggroup-form', Array) #9 [internal function]: Drupal\ggroup\Controller\SubgroupWizardController->addForm(Object(Drupal\group\Entity\Group), Object(Drupal\group\Entity\GroupType)) #10 /home/mysite/mydomain.com/drupal8/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(123): call_user_func_array(Array, Array) #11 /home/mysite/mydomain.com/drupal8/core/lib/Drupal/Core/Render/Renderer.php(574): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() #12 /home/mysite/mydomain.com/drupal8/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(124): Drupal\Core\Render\Renderer->executeInRenderContext(Object(Drupal\Core\Render\RenderContext), Object(Closure)) #13 /home/mysite/mydomain.com/drupal8/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(97): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) #14 [internal function]: Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() #15 /home/mysite/mydomain.com/drupal8/vendor/symfony/http-kernel/HttpKernel.php(144): call_user_func_array(Object(Closure), Array) #16 /home/mysite/mydomain.com/drupal8/vendor/symfony/http-kernel/HttpKernel.php(64): Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request), 1) #17 /home/mysite/mydomain.com/drupal8/core/lib/Drupal/Core/StackMiddleware/Session.php(57): Symfony\Component\HttpKernel\HttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #18 /home/mysite/mydomain.com/drupal8/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(47): Drupal\Core\StackMiddleware\Session->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #19 /home/mysite/mydomain.com/drupal8/core/modules/page_cache/src/StackMiddleware/PageCache.php(99): Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #20 /home/mysite/mydomain.com/drupal8/core/modules/page_cache/src/StackMiddleware/PageCache.php(78): Drupal\page_cache\StackMiddleware\PageCache->pass(Object(Symfony\Component\HttpFoundation\Request), 1, true) #21 /home/mysite/mydomain.com/drupal8/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(47): Drupal\page_cache\StackMiddleware\PageCache->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #22 /home/mysite/mydomain.com/drupal8/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(50): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #23 /home/mysite/mydomain.com/drupal8/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #24 /home/mysite/mydomain.com/drupal8/core/lib/Drupal/Core/DrupalKernel.php(656): Stack\StackedHttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true) #25 /home/mysite/mydomain.com/drupal8/index.php(19): Drupal\Core\DrupalKernel->handle(Object(Symfony\Component\HttpFoundation\Request)) #26 {main}.

Perhaps I have made a simple mistake? I made sure to install a fresh Drupal, version 8.3.4 and encountered the same issue. I made sure to apply it against 8.x-1.x-dev dated 2017-Jun-06.

Here are my web server details in case relevant: Apache/2.2.32 (Unix) mod_ssl/2.2.32 OpenSSL/1.0.1e-fips mod_bwlimited/1.4 PHP/5.6.30

Thank you

Small review:

1. +++ b/modules/ggroup/config/optional/views.view.subgroups.yml
   @@ -0,0 +1,818 @@
   +  page_1:
   

   Can we have a better machine name rather than page_1?

2. +++ b/modules/ggroup/ggroup.views.inc
   @@ -0,0 +1,32 @@
   +   $data = array();
   

   Use []

3. +++ b/modules/ggroup/ggroup.views.inc
   @@ -0,0 +1,32 @@
   +  $data['group_content_field_data']['group_id_depth'] = array(
   

   Use []

4. +++ b/modules/ggroup/ggroup.views.inc
   @@ -0,0 +1,32 @@
   +    'argument' => array(
   

   Use []

5. +++ b/modules/ggroup/src/Form/SubgroupFormStep1.php
   @@ -0,0 +1,111 @@
   +  public function __construct(EntityManagerInterface $entity_manager, PrivateTempStoreFactory $temp_store_factory) {
   

   EntityManagerInterface is deprecated.

6. +++ b/modules/ggroup/src/Form/SubgroupFormStep2.php
   @@ -0,0 +1,112 @@
   +  public function __construct(PrivateTempStoreFactory $temp_store_factory, EntityManagerInterface $entity_manager) {
   

   EntityManagerInterface is deprecated.

7. +++ b/modules/ggroup/src/Graph/SqlGroupGraphStorage.php
   @@ -0,0 +1,416 @@
   +    while ($id = $results->fetchField()) {
   

   Should be able to just use $result->fetchAll(); here instead?

I have the same issue like comment #112, I modified the patch in #104 slightly to fix this issue.

Just fixing problem with last patch.

#112 Fixed 1, 2, 3, 4 ,5

Added some small performance improvements.

Also added missing interdiff between 104-114 for easier review.

Can't reproduce this? The stack trace also doesn't show anything related to group or ggroup modules? Are you sure it's caused by this patch?

Did break something else, it is time to start adding a bunch of tests...

I can reproduce the issue with the view. Will try to fix that asap.

Ok fixed the WSOD. There is another issue though. We can't filter the subgroup view to show only subgroup group content. There is no plugin type available. You can add these after you actually create group types and install subgroups for the group types.

I think there should be a filter for plugin type 'subgroup'. So this still needs some work.

Using migrate to import groups and subgroups, I noticed the graph got messed up after changes in the source data. I'm assuming this is an error on my part, but this got me thinking that the only time the graph is properly created or check is when adding subgroup content. Since the graph is a form of caching for the relations, it would be nice to be able to recalculate the graph.

The code below does exactly this, not sure how to fit it in the module. Maybe a configuration page that adds a button to do this?

$subgroup_types = [];
foreach (\Drupal::entityTypeManager()->getStorage('group_type')->loadMultiple() as $group_type) {
  $plugin_id = 'subgroup:' . $group_type->id();
  /** @var \Drupal\group\Entity\Storage\GroupContentTypeStorageInterface $storage */
  $storage = \Drupal::entityTypeManager()->getStorage('group_content_type');
  $subgroup_content_types = $storage->loadByContentPluginId($plugin_id);
  foreach ($subgroup_content_types as $subgroup_content_type) {
    /** @var \Drupal\group\Entity\GroupContentTypeInterface $subgroup_content_type */
    $subgroup_types[] = $subgroup_content_type->id()
  }
}

$hierarchy_manager = \Drupal::service('ggroup.group_hierarchy_manager');
$group_contents = \Drupal::entityTypeManager()->getStorage('group_content')
  ->loadByProperties([
    'type' => $subgroup_types,
  ]);
foreach($group_contents as $group_content) {
  $hierarchy_manager->addSubgroup($group_content);
}

1. On /group/%/subgroups I'm seeing all of the group's content listed - nodes, users, menus. **EDIT: Is this what #121 is referencing?

2a. In src/Plugin/GroupContentEnabler/Subgroup.php all of the permissions should be for "entity" not "content". They should be like $permissions["create $plugin_id entity"]. "entity" is for new creation and "content" is for relating.

2b. Need to set the 'create_mode' flag that distinguishes creating/relating as is done in gnode. (Also see #2881769: group/{group}/node/create uses the permissions of group/{group}/node/add). In SubgroupRouteProvider.php:

    $routes['entity.group_content.subgroup_add_page']
      ->setDefaults([
        '_title' => 'Create subgroup',
        '_controller' => '\Drupal\ggroup\Controller\SubgroupWizardController::addPage',
        'create_mode' => TRUE,  #### This is the addition 
      ])

Here's an update that addresses point two in the last comment. (I'll once again point out that permissions specificity relating to creating vs relating is broken per #2881769: group/{group}/node/create uses the permissions of group/{group}/node/add)

An update to #125 to fix the Edit permissions machine name: it should be "update" not "edit".

#124.1 Yep, that's what 121 is about. Not sure how to fix?

#126 Nice one. Just ran into this myself. Improved the patch a little to fix the ordering and the description of the relations permissions. Also fixed weird references to a create $name group permission that doesn't seem to exist. Not sure why it was there?

Added support for parent group tokens for Subgroups. Based in https://www.drupal.org/node/2774827.

Unchecking the " 2-step wizard" option in a subgroup content plugin seems to have no effect... the second screen still appears regardless of the value of that option. This patch aims to fix the problem.

Apllied patch #129

While creating subgroups, In the subgroup tab I see myself show up as a subgroup... That I did not expect to happen?

Just created a location group type

Activated gnode (article) and ggroup (location) on it

created group Spanje

in group Spanje created subgroup Alicante

@parijke in #130

The missing filter on the subgroups pages to display only group content of the type 'subgroup' was identified in #121 and #124.1 as well

@idebr you're absolutely right.... punish me for my lazyness (didn't read the whole thread)

Glad it is in vision though

Fixed an issue where inherited group roles from the supergroup were written to the static cache of inherited subgroup roles, causing incorrect permissions.

#129 works fine for me. The only issue I experienced is that no matter if I'm creating new group entity or relating an existing one, the same permissions is used for access check, which doesn't allow us to restrict the user to relate an existing entity in group by using Group permissions table.

Here is a patch that fixes the issue

@nikolabintev #135
Your patch does not include the changes in #134. Can you post a new patch based on #134?

An interdiff would also help for reviewing your changes. You can find info on how to generate an interdiff at https://www.drupal.org/documentation/git/interdiff

@idebr I'll remove patch #135 as it is related to another issue: https://www.drupal.org/node/2881769

Fix issue.

Notice: Undefined offset: 363 in Drupal\ggroup\Graph\SqlGroupGraphStorage->getPath() (line 390 of modules/contrib/group/modules/ggroup/src/Graph/SqlGroupGraphStorage.php).
Drupal\ggroup\Graph\SqlGroupGraphStorage->getPath('446', '325') (Line: 238)
Drupal\ggroup\GroupHierarchyManager->getInheritedSupergroupRoleIds(Object, Object) (Line: 199)
Drupal\ggroup\GroupHierarchyManager->getInheritedGroupRoles(Object, Object) (Line: 63)
Drupal\ggroup\EventSubscriber\GroupEventSubscriber->inheritGroupPermission(Object, 'group.permission', Object) (Line: 111)

Method getPath tried get directAncestors from group which doesn't have those.

I've updated Subgroup.php with a patch that fixes a harmless warning about parent_role_mapping and child_role_mapping indexes being undefined for the default values.

I jumped the gun there, it looks like a sanity check is required after submitting the form and going back to the Group Content Types overview.

Regarding #121 and #124.1, I have updated the view to properly display only subgroup content, and fixed the dropbutton links.

Tested patch #141 and quickly came across 2 issues, but they may have already be mentioned, as I might have missed it in all of the comments above :)

- When navigating to the "subgroups" tab on any group it always says "An illegal choice has been detected. Please contact the site administrator.", because the "- Any -" option is selected, but not working.
- When you press "Create subgroup" you cannot select group type you are automatically placed onto an add form, instead of the group type selection form.

Other than that I think it's in good shape, but didn't do much in depth testing yet, as I am currently looking for options in my use case.

EDIT: I have realized now that I have to install content plugin for each group type, to be able to add it as a subgroup.

Was running into an issue where a user in a parent group could view/edit/delete a subgroup they weren't a member of. Looks like these permissions didn't do anything: "$operation any/own $plugin_id entity" (example: view any subgroup:<subgroup_type> entity).

I basically cloned and tweaked what gnode.module is doing in gnode_node_access(). Tested and this is working for me. (I haven't test the Views work from #141.)

It might be a little late to ask, but:
Might it be actually easier to propagate roles instead of permissions for a first access related solution?

My thinking:
* Create outsider roles for group-types based on configured group-type as content relations in other types
* Go up the group nesting from the corresponding bottom, selecting the first actual membership to be found
* Use that membership to select appropriate outsider permissions in case the use is not a member

Even it does not make sense in this context I would still propose separating the permission-bubbling so something like the above could be used instead.

Found an issue in this patch. From the subgroups page of a group, the edit and delete operations for each row point at the parent group, not the subgroup.

This means that attempting to delete a child entity from this interface results in the deletion of the parent instead.

Added a fix for #2933819: getPermission does not convert plugin label to string properly to the patch.

About #145:

Was running into an issue where a user in a parent group could view/edit/delete a subgroup they weren't a member of. Looks like these permissions didn't do anything: "$operation any/own $plugin_id entity" (example: view any subgroup: entity).

This could be caused by the membership inheritance. If you set your groups up to inherit a role from a parent/child, you could get access to groups you are not actually a member of. This is exactly the point of the current patch and I don't think the ggroup_group_access() should be needed.

When using large amounts of subgroups with inheritance we still noticed a significant decrease in performance. This was even worse after upgrading to RC2.
I think I found a way to prevent calculating the inherited roles for groups on every request by building a cache with all possible direct/indirect relations between groups and the roles that are inherited. The cache is invalidated and rebuilt when subgroups are added or removed.

Please let me know if you find any issues.

On a sidenote, I also used the patch in #2878389: Add static cache to gnode_node_grants. Without that patch, overview are still a bit slow. The function gnode_node_grants() adds a lot of calls to $group->hasPermission().

Also not sure why PHPLint fails on the splat operator?

Fixed a PHP warning and corrected small doc error.

ggroup also has the same issue found at #2793621: Upload files when adding content to group don't work well, which can be solved with the same solution as #12 applied in that thread.

Hi @seanB,

I tried your patch in #150, and have set the role inheritance correctly between group and subgroup, but I'm not seeing the inheritance working correctly.

My group is called "State" and my subgroup is called "District". I have 3 different roles for both groups: Program Manager, Team Member, and Guest. Any role in "State" should have Guest access to the "District".

Here is how my District subgroup content plugin is configured:

Whenever I try to access a given permission for a "District" with a user that has "Program Manager" role in the "State", I don't get the permission inheritance as defined by the configs of the content plugin. But if I manually add that role for the "District" to the user, it works fine.

Am I missing something?

Added the changes from #2793621-12: Upload files when adding content to group don't work well as suggested in #154.

Also fixed another issue with merging group roles in GroupHierarchyManager::getInheritedGroupRoleIdsByUser().

@ #155 Paul Kim Consulting, it seems you are doing everything right. If you are able to debug the issue and provide some more insight that would be really helpful!

Same issue as #155 here. Seems like this line in GroupHierarchyManager is the culprit:

$mapped_role_ids[] = array_intersect_key($role_map[$group_id][$membership_gid], array_flip($this->getMembershipRoles($membership)));

the role map and the final result need to be flipped as well.

+  $entity_types['group']->setFormClass('ggroup-form', 'Drupal\ggroup\Form\SubgroupFormStep1');
+  $entity_types['group_content']->setFormClass('ggroup-form', 'Drupal\ggroup\Form\SubgroupFormStep2');
...
+    return $this->entityFormBuilder()->getForm($entity, 'ggroup-form', $extra);

I think there's an issue here, as the ggroup-form messes up the form id, meaning one can't implement hook_form_FORM_ID_alter.

Patch & interdiff attached.

I have been able to "git apply -v" the .156 patch to the group-8.x-1.x-dev module with only whitespace errors warning. I then package up the new group module as a .tar.gz and use that to install into a clean Drupal 8.5.4 installation.

I then enable (within the administrations 'extend' menu item) the modified group module into a clean Drupal installation which is installed with the latest 64 bit bitnami LAMP stack.

Enabling group and 'group node' causes no problems.

When I try to enable subgroup I get the message:
"The website encountered an unexpected error. Please try again later."
on a White Screen Of Death.

The last lines at: apache2/logs/error_log show
"[Wed Jun 20 13:54:30.453360 2018] [php7:notice] [pid 13439] [client 127.0.0.1:52768] Uncaught PHP Exception Exception: "No entity type for field type on view subgroups" at /opt/lampstack-7.1.18-1/apps/drupal/htdocs/core/modules/views/src/Plugin/views/HandlerBase.php line 711, referer: http://localhost/drupal/admin/modules"
PHP Version:7.1.18 MySQL Version:5.7.22

This appears to be the same error from #119 except that the HandlerBase.php has gone from line 697 to 711. Yet, this was meant to have been fixed at #112

Regardless of the above, when I now check the enabled modules (within the administrations 'extend' menu item) it shows that subgroup has been enabled

I have redone the above
but using group-8.x-1.0-rc2 with the later 157 patch
and group-8.x-1.x-dev with the latest 158 patch
but in each case I got the same result.

Before using this 64 bit computer I tried to implement it on an old 32 bit laptop with older LAMP software but I had the same problems.

Are other people getting the same as this?

Is there anything I can do to not get this?

Many thanks

Applied patch from #159, it installed well and I could link groups and subgroups.
The roles and permissions inheritance and propagation though DID NOT work at all.
After debugging, I reverted the changes from #155 (no additional array flips) and voila - the permissions worked!
Why are those additional array flips of the role_map necessary at all?

Bug: when group is deleted, it still remains listed in "Content" tab, as a Subgroup. Hitting the "Install" button next to the deleted group throws error...ofcourse:) Please see atached screenshot.

I have installed a patch #159.

I wanted to unistall the entire Group suite. When uninstalling Group Node and Subgroup modules, I got error:

The website encountered an unexpected error. Please try again later.

Site is now unaccessible. Drush cr does not help, neither restarting Apache.

Apache logs say:

[Sat Sep 22 14:26:14.923769 2018] [php7:notice] [pid 16637] [client 62.197.243.35:49400] Uncaught PHP Exception Drupal\\Component\\Plugin\\Exception\\PluginNotFoundException: "The "subgroup:technicians" plugin does not exist." at /home/bizhub/public_html/web/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php line 52

Any thoughts please?

This is 156 rerolled to pick up the translation of the parent group in the tokens, similar to:

https://www.drupal.org/project/group/issues/2774827#comment-12835463

With this patch applied one can use a path alias like:

[group:group:title]/[group:title]

And get the parent and the child in the same language.

Sorry attached 156 again instead of new patch 164.

Changed status for testing and added a test for #165

We've got something going like this for our configuration already. Curious how this would compare. It's custom written for a particular set of groups, and menus in groups.

I've been running the patch from #165 on a project with a large number of groups (about 50K). Our structure is really simple with just groups and subgroups. The method of loading the entire groups_graph structure and using that for role inheritance just doesn't work for a project with our type of structure/size. Operations dealing with groups use a debilitating amount of memory (peaking around 900M) and take an extraordinary amount of time (no duh).

I've created an alternate method for loading group hierarchy that loads only one group at a time. I realize that there are potential issues with other projects with a deeper group structure, but at least for our use case, we've seen a dramatic improvement in performance. I also added caching on the hierarchy mapping, so that we're only hitting the database when we have to and only for the data that we need.

Not saying that this is the way to do this, but I think it's a step in the right direction. The existing solution just doesn't scale well enough for large projects. I haven't dug into the role inheritance logic, but I have a feeling that this issue is really just revealing another performance issue further up the chain.

Applying patch #170 worked, but enabling modul I get fatal error and following message in dblog:

Exception: No entity type for field type on view subgroups in Drupal\views\Plugin\views\HandlerBase->getEntityType() (line 712 of /medjxcwh/drupal8test-composer/core/modules/views/src/Plugin/views/HandlerBase.php).

@MartinMa, I had the same issue, until you actually create a group type. I'm testing using Opigno_LMS.

Don't forget to change the issue status when you commit a patch. Otherwise tests will not run :)

Added a test manually.

@Martijn de Wit Ah thanks for that. Totally forgot.

I've kept working on this and made a similar change to the GroupRoleInheritance service, allowing mapping to be built for one group at a time. Also fixed a few bugs in the last patch.

Again to reiterate: I don't think this is the correct solution. I'm just dipping my toes into the inner workings of ggroups. I'm happy to keep pushing this along, but I would super appreciate some feedback from everyone. I'm basically just hacking ggroups so that it isn't a total memory hog for my use case. :/

We're building a distro with subgroups using the patch from #165. We have a demo content module that creates a bunch of test groups, then behat tests that verify our group content and relationships.

Testing either the patch from #170 or #174, I'm getting a lot of failures. In particular it appears to be related to the GroupHierarchyManager. Code like

$subgroups = \Drupal::service('ggroup.group_hierarchy_manager')->getGroupSubgroups($group->id());

gets an empty array back when there are valid subgroups.

I've inserted some cache clearing just in case it was a caching issue, but that doesn't appear to make a difference.

For now we're sticking with #165.

Attached is a reroll of the patch from #164 with a better validation. This is following alex.rutz's suggestion on a similar change on 2774827.

@floydm Awesome, thanks for the feedback.

Holy cow. I had a huge bug in the loadMap function.

-    if (!empty($this->loaded[$gid])) {
+    if (empty($this->loaded[$gid])) {

This works on the SECOND check of any page request, but not the first.
I also found a bunch of places where I was misspelling descendants (decendants).

Anyways, Fixed both of those issues and now you can load the subgroups on the first try :)

@floydm, would love for you to test this out and see if it fixes things for you.

Also I forgot to reroll with your last patch, so if anyone wants to go for it. Otherwise I'll hit that next go around (as I'm sure there will be one).

Also, I lied. It doesn't work on the second try. Honestly, I have no idea how this was working for me....

@lukedekker My tests are still failing with the patch from #179 but failing differently now. Which is progress. :)

With #177 I am able to see the subgroups and supergroups, but user permissions aren't cascading.

Our scenario has:

Parent group with group content (nodes).

Child group which users are members of.

The members of the child group inherit access to view the content in the parent group by virtue of parent/child group role mapping.

This works on #177 but not #179 (Access Denied).

Ok. Here is an updated patch (again). I believe that it addresses the issues you were having @floydm.

I also found yet another area where a permissions check was loading every single group_content entity of a specific plugin. (This time it only took up about 600MB, yay.)

Anyways, fixed those, also fixed a caching issue that I found.

Thanks for helping me test this!

Hey @lukedekker! I'm on holiday but don't want to keep you hanging, so I applied your latest patch and ran my tests. I went from 4 failing previously down to 2 failing now.

The errors seem to be related to role access inheritance still. For example, when I'm accessing the parent group as a member of the child group, I'm still getting "Access Denied" and a screen full of notices like:

Notice: Undefined offset: 1 in Drupal\ggroup\GroupRoleInheritance->getSubgroupRelationConfig() (line 269 of modules/contrib/group/modules/ggroup/src/GroupRoleInheritance.php).
Drupal\ggroup\GroupRoleInheritance->getSubgroupRelationConfig('1', '3') (Line: 142)
Drupal\ggroup\GroupRoleInheritance->build('1') (Line: 90)
Drupal\ggroup\GroupRoleInheritance->getAllInheritedGroupRoleIds('1') (Line: 186)
Drupal\ggroup\GroupHierarchyManager->getInheritedGroupRoleIdsByUser(Object, Object) (Line: 65)
Drupal\ggroup\EventSubscriber\GroupEventSubscriber->inheritGroupPermission(Object, 'group.permission', Object)
call_user_func(Array, Object, 'group.permission', Object) (Line: 111)

I probably won't have a chance to chase this down in any more detail until after the holidays but I hope this helps you out.

@Floydm, can you confirm that there is a valid subgroup for your group id 1?

I believe I've run into that issue before when running patch from #164.

Currently, my inclination from past behavior and looking at the logic in place is that we need better handling for this situation, but there is no serious issue in the logic here.