Allow more than one parent by node / subgroup

Hi there and thanks for using Group, I'm glad you like it :)

While I myself have found use cases where a node could benefit from multiple parents, I chose to start off with the single parent story. This choice was made to get a working alpha out the door sooner.

While I do intend to look into the multiple parent story again after polishing the rest of the module, I would need to make sure that I've got all bases covered. A possible hiccup could be the removal of nodes:

• User X who has Page permissions in both Group A and Group B creates a single Page in Group A and B (shared node)
• User Y is a regular member of group A and can view the page
• User Z who only has permissions in Group B to delete Page nodes, deletes the page
• All of the sudden, User Y can no longer view the page

This can be resolved by making sure deletion only happens when there is just one group left (otherwise group unlinking is the action of choice). But as you can see, there is a lot more to keep in mind with multiple parent entities.

In the case I described, you want to be consistent to the end user: The button should always say "Delete page".

However, if the node is still part of other groups, you'd just want to break the link between the group the node is being deleted from and the node itself. In case the node only had one link remaining, it is safe to actually delete the node.

As you can see, there is a lot more to keep in mind when working with multi-parent nodes.

I will try to understand all of your code and maybe I can help a bit more.

Please do :) A code review is always a nice thing to have!

Hi - as with juagarc4 just like to start off by saying say great module!

I have a similar need - which I am doing by overriding some of your functions in my own module with a few tweaks to gnode_node_form_alter and gnode_node_submit.

In my case it is a bit simpler, as only admin users will be creating content and assigning permissions to groups for access control. Nonetheless I would be very interested if you were to release a version that supported multiple parents - and once again - great work.

What I would suggest for now is that people clone the gnode submodule and of course rename it. Then you can use that module as a sandbox for setting up multi-parent nodes. The hardest part will be the access checks (lots of code in there) on the node_access queries.

Hi,

We needed this functionality for a client, so we have prepared the following MVP alteration to gnode (patched against the latest beta5 release.)

If you can administer all groups, you get the autocomplete and can add and remove from all groups
If you can administer some groups, you get checkboxes for those groups. On save we preserve group access they don’t know about
If you have access to delete the content based on your permissions in any applicable group, it gets deleted. Even if it’s in a group you don’t know about. (We extensively discussed how this should work, and for our MVP we matched OG's default behaviour.)

Known issues:

The autocomplete could use some more love.
If you aren't a group admin and you remove it from all of your groups, you have no way of knowing whether it is still in other groups or has gone public. This one needs a bit more thought.

This is currently with our client for testing. We think it serves our needs nicely, so we thought we would share!

Thanks,
Pete

This is going to need some attention when the node access patch gets in

#2559135: Not using node grants creates a general incompatibility with other Drupal funcionality

Please find an initial patch for this assuming patch in #17 of the other issue has already been applied. All existing tests currently pass.

@todo Add some new tests for checking access rules when a node is attached to more than 1 group.

Realised that inaccessible nodes still appear on listing pages in Safe mode with the patch above. We therefore need:

1. To change gnode_query_node_access_alter to account for multiple parent groups for a node
2. Test coverage for listing pages
3. General test coverage for multiple group parents for a node

Attached patch includes the multiple parent groups per node access test.

This patch assumes patch in the following issue has already been applied. That patch fixes the issue with access to nodes on listing pages and adds tests for listing pages.

#2559135: Not using node grants creates a general incompatibility with other Drupal funcionality

I think we do need a better UI tool for selecting groups. Perhaps a dropdown select rather than autocomplete then site builders can apply chosen to it to make it look nice?

I've rewritten the form alter code to simplify it. There needs to be some tests for the form to ensure the access control works as expected.

However, I think there are some important questions to resolve first:

1. Should nodes in multiple groups be configurable? This could easily be controlled with a single setting enforcing one group per node.
2. Confirm the access control logic for nodes in multiple groups:

a. Bypass group access users can set any groups for any node.
b. If it is a new node, the user can add the node to any group they have create access to (via either the create type node permission or administer group permission)
c. If it is an existing node, the user can change the groups the node is in only for groups they are an administrator of (via the administer group permission)
d. If the user cannot globally create nodes of this type then they must select at least one group for the node to be a member.
e. If the user cannot globally create nodes of this type and there is only one option available for the group then it should preselected for them and the control disabled.
f. If the user is unable to select any groups (for example, they are able to edit nodes in a group but not being an administrator could not change the group it is in) then the group control should not be on the node edit form at all.

3. Standardise the control interface - the autocomplete is clunky for nodes in multiple groups. I'm in favour of just using a multi select control for everyone, this would cut out a chunk of code in gnode_form_node_form_alter(). The user interface for sites with lots of groups can be improved by enabling the chosen module. Again, this could be configurable if there are strong reasons for multiple routes.

Please find a new patch which allows:

1. Nodes in multiple groups is configurable via a setting at /admin/group/settings/gnode - default is node is only allowed in a single group by default
2. The form widget the user gets to choose which groups a node is in is configurable at /admin/group/settings/gnode - the choices are Select dropdown, auto complete or checkboxes (or radios when in single node to group mode).
3. Different form widgets can be chosen for users with or without bypass group access permission
4. There are separate tests for multiple nodes in groups

This patch also includes the latest changes on this thread as well:
https://www.drupal.org/node/2559135#comment-10469543

The interdiff shows the difference between this work and the work on the other thread - so just the work introduced by this thread.

Including an updated patch which also deals with another issues identified in #2460927: Hide "Group settings" from Node creation.

If the user only has the ability to create nodes within their groups and they are only an admin of a single group then preselect that group on the node/add form

Reroll for the new 7.x-1.x branch changes now #2559135: Not using node grants creates a general incompatibility with other Drupal funcionality is in.

Can we get a reroll for this?

Rerolled against dev. Looks like everything is still working like it should.

Thanks! I am trying to find the time to get a few outstanding feature requests in group 7 so I can tag a 1.0 release. This being one of them :)

Thank you very much for the fix in #16 that includes #2460927: Hide "Group settings" from Node creation because I struggled with that old one for a while, like a doofus, and kept getting errors. Very much appreciated.

I've reviewed what was done and am happy this is still working as it should.

I ran all the tests again which takes about 20 minutes and they are all green (1882 passes, 0 fails, 0 exceptions)

Looks like we have at least 3 people using this now so setting this to RTBC ...

I've reviewed the entire patch aside from the tests and made a few minor changes and code cleanups left and right. However, I've discovered something that seems off in gnode_node_submit().

The code that's supposed to add the groups someone couldn't change back to the list to save doesn't seem to work properly. I've already made sure it now takes single-value form elements into account properly and it runs the check before the array filter removes the unselected elements (see interdiff). But it will still fail when using an autocomplete, seeing as that doesn't contain a full list of groups the user could change.

If I read the code correctly, the idea is that the selects/checkboxes submit an array that unselected IDs are still part of, but have a zero value in. This allows us to both tell which options were available to the user and which he decided to select. Judging by the available options, we then re-add the groups he could not see and thus edit.

We're close, but I need to be sure this will work for all scenarios before I can commit this.

Just to update - I'm in the middle of writing some tests to show this problem.

This probably isn't the issues to do it in but now that we are starting to abstract the group logic out of node's form_alter what are the opinions on taking it a step further and fully generalizing the functions within form_alter,validate, and submit to work for most entities?

I'm of the opinion that if we generalize the functions to interact with standard entities then integrating group with other entities becomes a copy, paste, change entity type affair. Where as right now to add multiple parents to other entities will require copying over all these new functions and modifying them for that entity.

@ceng: Great!
@ctrlADel: Excellent idea, definitely +1. But perhaps we should work towards this for 1.1?

I believe I've solved the issue but in writing the more comprehensive tests I've come up with another issue.

When a node can exist in multiple Groups, should it be possible for a Group administrator to remove a node from all their Groups?

Currently with this patch the behaviour is like this:

1. If the Group selection widget is auto complete, the Group administrator can always remove a node from all their Groups (though now not Groups they are not administrator of)

2. If the Group selection is either checkboxes or select, the form element is disabled if the Group administrator is only an admin of one Group so the node has to be in that Group.

3. If the Group selection is either checkboxes or select and the Group administrator is an administrator of multiple Groups then the form element is not disabled and the Group administrator can choose to put the node in all their Groups or some of their Groups or none of their Groups.

I believe that only those with the global 'bypass group access' permissions should be able to put a node into none of their Groups. I am going to rework the patch and provide the tests for this.

Let me know if you think otherwise.

Updated patch attached which I believe addresses the problems raised in #20 and #24.

There is another test file included for the gnode module which checks the following situations for all 3 methods of manipulating node to group relationships on the node edit form (checkboxes / select and auto complete):

1. Group administrators are only able to affect the group a node is in for their own groups
2. Group administrators cannot take a node out of all their groups
3. Site administrators with 'bypass group access' can take a node out of all their groups.

I uploaded the wrong patch file last time, what an idiot.

Real patch file attached, interdiff is in the previous comment.

Please find updated patch after last nights mini sprint with the code which decides what group a node should be in after save moved from the gnode_node_submit to its own function so it can be called from both the validate and submit functions. This allows us to have a form validation error in the case that the group admin has taken the node out of all their groups but does not have the ability to create nodes of that type globally on the site.

Keep up the good work, @ceng. We will certainly benefit from this enhancement when it's finished.

I ended up needing to add multiple parent support to file entities so I took the time and generalized the existing patch from 27. In an effort to keep things at least kinda clear I've split the patch into a group changes patch and a gnode changes but both are required to make node's have multiple parents.

Not sure I chose the best naming for the functions or the files so opinions are welcome.

I feel the generalizing turned out pretty well. If you guys agree it's workable and will require minimal changes then I think we should continue patching off of the generalized functions. If it looks like there's still quite a bit of work to be done to make the generalized approach workable then let's not derail this issue and this can be left as a proof of concept for the future implementation.

Thanks for the patches ceng and ctrlADel! Having talked to ceng, his patch should be quite complete now but I'm always open to improvements.

@ctrlADel: Could you bundle your work into one patch and provide an interdiff compared to the one in #27 please? That makes it a lot easier to review the code and changes.

Yep, not sure how useful the interdiff will be because a lot of it moved to different files but here it is.

Attached patch and interdiff are for the patches from 29 combined.

Here are observations I made at a cursory look at patch #31:

Case 1:
Authenticated can view published content
Authenticated on Article can do nothing
Group 1 Role on Article node can CRUD own, edit any, but not delete any
Group 2 Role on Article node can CRUD own, edit any, and delete any
User 1 has Group 1 Role: on Article can create content, but cannot edit own, edit any, or delete own
User 2 has Group 2 Role: on Article can create content, but cannot edit own, edit any, delete own, or delete any

Case 2:
Authenticated can view published content
Authenticated on Article can CRUD own, but not edit any or delete any
Group 1 Role on Article node can CRUD own, edit any, but not delete any
Group 2 Role on Article node can CRUD own, edit any, and delete any
User 1 has Group 1 Role: on Article can create content and edit own, but cannot edit any or delete own
User 2 has Group 2 Role: on Article can create content and edit own, but cannot edit any, delete own, or delete any

Summary: In Case 1 I would expect User 1 to be able to edit own and delete own, but it cannot. And, I would expect User 2 to be able to edit own, delete own, edit any, and delete any, but it cannot. In Case 2 I would have expected the global permissions to not have affected the behavior.

Of particular note, it appears when a node is created it is not getting assigned to a group. This may be the cause of the unexpected behavior. Or, the cause could be specific to my setup--as such, @ctrlADel can you verify you do not experience the same behavior?

After applying two minor changes ctrlAdel suggested (see fixes-for-32.txt), here is what I observe:

Case 1.A. - Node CRUD
Authenticated can view published content
Authenticated on Article can do nothing
Group 1 Role on Article node can CRUD own, but not edit any or delete any
Group 2 Role on Article node can CRUD own, edit any, and delete any
User 1 has Group 1 Role in Group 1: on Article can view any in group, view any that has no group, create content in group, edit own, delete own, but cannot edit any or delete any
User 2 has Group 2 Role in Group 2: on Article can view any in group, view any that has no group, create content in group, edit own, delete own, edit any in group, delete any in group
User 3 has Group 1 Role in Group 1 and Group 2: on Article can view any in either group, view any that has no group, create content in group, edit own, delete own, but cannot edit any or delete any; can set group on creation, but cannot modify group after creation
User 4 has Group 2 Role in Group 1 and Group 2: on Article can view any in either group, view any that has no group, create content in group, edit own, delete own, edit any, delete any; can set group on creation, but cannot modify group after creation
Administrator in no groups: on Article can view any in any group, vie wany that has no group, create content in any group, edit own, delete own, edit any, delete any; can set group on creation and modify group after creation

Case 1.B. - Node CRUD w/o View published
If you remove "View published content" from Authenticated, the user cannot see any content

Case 2.A. - Changing Node's Parent Group
Authenticated can view published content
Authenticated on Article can do nothing
Group 3 Role on Article node can CRUD own, edit any, and delete any
Group 4 Role administer group, on Article node can CRUD own, edit any, and delete any
User 5 has Group 3 Role in Group 1 and Group 4 role in Group 2:

• Node in Group 1 and 2: cannot edit group of node; field shows Group 2 (see screenshot "node-in-group-1.jpg")
• Node in Group 1: cannot edit group of node; field shows Group 2 (see screenshot "node-in-group-2.jpg")
• Node in Group 2: cannot edit group of node; field shows Group 2 (see screenshot "node-in-group-1-and-group-2.jpg")

Case 2.B. - Changing Node's Parent Group
Authenticated can view published content
Authenticated on Article can do nothing
Group 4 Role administer group, on Article node can CRUD own, edit any, and delete any
User 5 has Group 4 Role in Group 1 and Group 2:

• Node in Group 1 and 2: can edit group of node; field shows Group 1 and Group 2
• Node in Group 1: can edit group of node; field shows Group 1
• Node in Group 2: can edit group of node; field shows Group 2

Summary: I think it all looks good except for the case where a node is part of two groups and the user editing it is only administer of one of the groups, i.e. Case 2.B. The behavior appears to solely be aesthetic: it just displays to the user the wrong group.

Next steps:

1. Include fixes-for-32.txt in the patch (or something similar)
2. Address the aesthetic issues noted in Case 2.B.
3. Do a thorough code review (perhaps @kristiaanvandeneynde or @ceng could?)

Rolled the fixes from 33 into the patch.

These changes in #34 look good.

Next Steps

1. Address the aesthetic issues noted in Case 2.B., which is when a user is part of a group that it is not also an administrator of the default value in the select and in the left sidebar do not show correctly
2. Do a thorough code review (perhaps @kristiaanvandeneynde or @ceng could?)

Found a minor annoyance that was simple to fix. Checkbox and select elements return associative arrays keyed by gid and the autocomplete element was just returning an array of gids. For consistencies sake I made it so the autocomplete returns an associative array.

To clarify the problems, which are minor, imo:

Problem 1: Hybrid admin ownership results in unexpected behavior

1. Add a user to Group 1 as an administrator of Group 1
2. Also add the user to Group 2, but not as an administrator of Group 2
3. As the user, add a node in both Group 1 and Group 2
4. Edit the file as the user and you will notice the "Group" select box displays "Group 1" but is disabled

I would expect the select to not be disabled so that it could remove the node from Group 1.

Problem 2: Hybrid admin ownership results in incorrect parent group shown

1. Add a user to Group 1 as an administrator of Group 1
2. Also add the user to Group 2, but not as an administrator of Group 2
3. As the user, add a node to only Group 1
4. Edit the file as the user and you will notice the "Group" select box displays "Group 2" but is disabled

I would expect the select box to be shown so that the user could add the node to Group 2.

Hi there,

I'm trying to apply the patch in #36 but getting errors. Which version of the module should I be applying it against?

UPDATE - I managed to update patch #36 successfully against 7.x-1.x-dev version, rather than the betas.

@griffphilips how did you apply the patch? What command did you use? I tried and it said that certain files already existed and a few hunks failed.

UPDATE - I managed to update patch #36 successfully against 7.x-1.x-dev version, rather than the betas.

Yeah, patches are typically written against the latest dev branch so sometimes they'll apply cleanly to a tagged release but not always.

@griffphilips how did you apply the patch? What command did you use? I tried and it said that certain files already existed and a few hunks failed.

I didn't have a problem applying the patch in #36 to the dev branch using git apply group-2317195-multiple_parents-36.patch

I got "warning: 1 line adds whitespace errors." when applying the patch via git. Afterward, I could not find an option to enable multiple group selection.

I got "warning: 1 line adds whitespace errors." when applying the patch via git. Afterward, I could not find an option to enable multiple group selection.

The warnings not a big deal, just means I missed a whitespace somewhere. To enable multiple group selection you'll need both group and gnode enabled then when you go to /admin/group/settings/entity you should see a checkbox to enable multiple parents for nodes.

Thanks for responding @ctrlADel. Sorry, I've looked through every subbranch of admin/group I'm aware of but I still can't seem to find the checkbox in question. Would you be able to share the exact url? Is it /admin/group/type/manage/entity?

Would you be able to share the exact url? Is it /admin/group/type/manage/entity?

/admin/group/settings/entity should be the exact path, have you tried clearing the cache?

@ctrlADel I get a 500 when I ping that url. I can get to admin/group/settings/gnode. But I only see an option for group node access mode there. I have flushed cache multiple times. I've also attached a screen of the options (tabs) I have under /group.

I'd like to help with this if any more help is needed so we can merge it and move on to working on the option to have selective node types for groups. Can someone help me with what needs done next? I don't actually need this functionality but I have a site I can test on if needed. So far I applied the patch and I see the new tabs: Group Entity Settings and Group Node Settings.

I have confirmed this patch applies to the latest 7.x-1.x branch, commit fc55988f7f070a4b60ad59b9d0ba049687d280f9. I have thoroughly tested this only two minor issues in #37 which I believe can be follow ups.

The only thing that I believe should happen next is for @kristiaanvandeneynde or @ceng to give it a final review. Setting this to RTBC.

I finally got this working. I experienced a permission issue which I've yet to identify exactly as I had to revert back. Essentially, the node form group settings disappeared for some users and others lost the ability to see certain group posts in views.

@pribeh Thanks for checking this out--to make sure I understood your previous comment, are you saying that after getting it working you experienced the permission issue?

@josephdpurcell, correct. I'm not entirely clear if it was caused by this patch or the following since it arose while we were testing both:

https://www.drupal.org/node/2702133#comment-11500099

@pribeh do you have steps I could reproduce? Any info to help reproduce it will be useful.

Hello, I am unable to use #36 to patch the current 7.x-1.x-dev dated 2016-Sept 19, as it fails with file not found and hunk errors.

I have no idea how to re-roll a patch.

The patch from https://www.drupal.org/node/2702133 also failed.

Can anyone provide assistance or instruction on how to correct.

Thanks

With the most recent dev version, as others already stated, patch is not applicable. Managed to manually apply to all files "resisting" to patching except gnode.compliance.inc - but to no avail.

Could please somebody either make the patch compliant to a designated version or - better - create a new dev build including that patch. Multiple parents is desperately needed...

I believe this is an accurate patch re-roll on the latest 7.x-1.x of group.

@ctrladel do you mind giving this a review to ensure it's accurate? If it is, it can be set to RTBC. The two conflicting files were modules/gnode/gnode.module and modules/gnode/modes/gnode.compliance.inc. Refer to the "unresolved-conflicts.txt" for the list of conflicts before I resolved them.

This should also be re-tested if anyone has time to contribute. My notes from #31 and #32 are relevant.

Don't fully understand: To which commit exactly does this patch apply? I'd love to test but need a little advice...

@thomas.wardin The patch from #55 should apply to the latest 7.x-1.x branch of the group module. I believe that commit is 5914267f52907ff761f3a46f49fe26d4f0f67dfc. Let me know if there's anything I can help with, also you can find me in #drupal in IRC.

As I'm not on IRC, let me get back this way: Neither the latest group-7.x-1.x-dev nor group-7.x-1.0-beta6 can be successfully patched. This also applies to commit 5914267f52907ff761f3a46f49fe26d4f0f67dfc. In all cases, group.child.entities.inc and group.entity.inc fail.

@thomas.wardin I have attached a console printout of the steps I'm taking to apply the patch. It appears to apply cleanly for me. I hope this helps clarify, if it doesn't would you mind attaching a console printout of the errors you are seeing?

Worked now - using git apply instead of patch did the trick!

Sorry for the silly question, but how can I update the installed module with this patched version - as I have already defined group type(s) and group(s), would not want to start over - if possible.

No worries! This is exactly why the issue queue is here :)

If you're applying this patch on an existing site, with existing groups, you should be OK to apply this patch without needing to any update steps.

That said, if you do run into any issues, consider re-saving each group config--there may need to be values that need populated.

Instead of applying the patch on the site, I copied the patched module contents to the group folder on the site. Cleared caches before and after administering the new group entity settings - and it just works fine. Thank you so much!

I applied this patch successfully, but it now seems to have broken #2530000: Bulk assign of content to groups. The Rules Component that that issue told me to import worked fine before the patch, but now fails integrity check. When attempting to execute the VBO, I get this in an error box near the top:

The utilized component rules_assign_a_node_to_a_group fails the integrity check.

When attempting to edit the Component, I see this in red underneath it in the Components listing, and again in the list of Action Sets when I try to Edit the Component:

Error: The data type of the configured argument does not match the parameter's value requirement.

And finally, when I try to Edit the Action Set, I see this above the Data Selectors collapsed fieldset:

Data types: Select data of the type List of group items.

Am I correct in assuming that the original field type for the “node:group” was simply “Group item” as opposed to “List of group items” and that this change is what enabled multiple parent groups per node? If so, how do I go about getting a Rules Component to provide a list of Groups to assign Nodes to?

I also note that there are “Data selectors” with names like “node:group:0:” through “node:group:3:” (note colon at end in both) in addition to “node:group” and that all five of those have the Label “Parent Groups” and Description “The group(s) the node belongs to.” Are those then individual Integer-type entries to allow individually setting the first, second, third, and fourth (0–3) Parents? Does this mean that this patch still limits Nodes to belonging to a maximum of four Groups each?

(The Rules Edit Action Set “Data selectors” really needs to add a column for “Data Type” so that we can see which selectors are which types, but that’s a Feature Request for another Module.)

Where are we at with this? I was successfully able to patch and have been using this for a number of months now. Are we ready to commit this?

@rositis@gmail.com #64
Where are we at with this? I was successfully able to patch and have been using this for a number of months now. Are we ready to commit this?

I disagree. I’ve already posted several other issues with this one as either Related or Parent Issue about things that this patch seems to break, including the Rules integration (#2530000: Bulk assign of content to groups), hard errors on viewing gnodes under certain circumstances (#2858188: "Undefined group in gnode_node_access() line 306" after Patch #55 of Parent Issue when viewing gnode if logged in without “Bypass group access control” permission), and more (see the gray Block listing Child, Related, and Referencing Issues in the right sidebar [when viewing on desktop]).

These problems range from moderate to pretty serious, and need to be addressed before this patch is committed.

@ctrlADel and @josephdpurcell, as the authors of the patch, could you please take a look at the Child Issue #2858188: "Undefined group in gnode_node_access() line 306" after Patch #55 of Parent Issue when viewing gnode if logged in without “Bypass group access control” permission and in particular how, when editing a node, the “Group settings” vtab continues to say “Not a group node” even when the note being edited had previously been manually assigned to one or more Parent Groups? I suspect that some key flag may not be set when manually assigning Parent Groups to an existing node, and that this may be the underlying cause of the errors. There’s a screenshot attached to that Issue.

Also, the patch appears to break Rules integration (Bulk Assign Action) as I commented upthread (#63) and in the now-Related thread #2530000: Bulk assign of content to groups because the data type no longer matches (Array or List of Groups rather than a single Group) and I can’t figure out how to work around that.

I think that these should be fixed before y’all consider committing this patch even to a -dev release, let alone a new recommended version.

@ctrlADel and @josephdpurcell, as the authors of the patch, could you please take a look at Comment #10 in the Child Issue #2858188-10: "Undefined group in gnode_node_access() line 306" after Patch #55 of Parent Issue when viewing gnode if logged in without “Bypass group access control” permission? I think I fixed that problem, but I don’t know how to roll and submit a patch. I have now marked it “Needs Review.”

The Rules Integration remains broken. If that can be fixed, then I’d agree that this patch is finally ready to be committed to at least a -dev release.

I re-rolled the patch to incorporate @Joel MMCC's suggestion for fixing Child Issue #2858188. While I posted it in that issue, I'm posting is here as well. Feel free to give it a try.

Re: @rositis@gmail.com's patch in #68. I tested it and found that it fixed the problem and appeared to cause no side-effects.

I was going to re-upload it to activate auto-testing, but the forum here would only allow me to select “8.x-1.x-dev” to test against instead of “7.x-1.x-dev” even though this Issue is clearly marked as being about Version 7.x-1.x-dev. Not sure why that is.

Changing Status from NW to NR.

While this fixes a critical bug and may justify this finally being rolled into a new -dev release at least, there are still some Related Issues that need to be resolved before making a new stable release (IMHO), especially #2530000-18: Bulk assign of content to groups (Rules Integration), plus the minor Admin UI issue I pointed out in #2858188-4: "Undefined group in gnode_node_access() line 306" after Patch #55 of Parent Issue when viewing gnode if logged in without “Bypass group access control” permission (and #66 in this issue, though the other one has a screenshot image of the problem) regarding how gnodes (nodes assigned to one or more Parent Groups) are confusingly showing up as “Not a group node” in the “Group settings” vtab (the vtab itself, not its contents & form) when Editing them.

Attention @kristiaanvandeneynde!

The more I think about it, the more I think that such a major feature as this one that introduces a significant change to the underlying GNode data structure (changing the Parent Group entry from a scalar Group [ID?] to a List of Groups [IDs?]) and thus inflicts significant backwards incompatibilities (such as with the #2530000-18: Bulk assign of content to groups Rules Integration) justifies a whole new 7.x-2.x major version release, not a mere new -dev and then stable point-increment release of 7.x-1.x. That way, existing code depending on GNodes having one and only one Parent Group and that scalar Group parameter won’t have to be changed so long as they specify Groups 7.x-1.x as a dependency, and likewise the existing Rules Action code in that Issue would be said to work only with 7.x-1.x, and a different Rules Action would need to be created for 7.x-2.x.

With this in mind, I think that once the RTBC process is complete, that Group 7.x-1,x-dev as patched by @rositis@gmail.com’s #68 improved version of @josephdpurcell’s #55 improvement of @CtrlAltDel’s #36 & #34 improvements of @johnennew’s #s 8–13 improvements of @Pete B’s #7 should be rolled into a new 7.x-2.x-dev major version release, with multiple Parent Groups per GNode being the key new feature. With further testing, this could become a new major version stable release that would be designated the new Recommended version and home for subsequent new features that don’t inflict backwards incompatibility like this one does, with 7.x-1.x remaining available with only bug-fix development for those who have code that depends on the scalar Group property of GNodes or otherwise requires one and only one Parent Group per GNode. Doing this removes such backwards incompatibility Issues as blockers for such a release, and I know of no remaining blockers (the UX thing with the vtab saying “Not a group node” is comparatively minor and should be easily fixable).

My earlier Plan suggestion of #2847520: Proposal: New 2.x branch rewrite based on Relation module would be incremented to being a proposal for a 7.x-3.x based on the Relation module. I’ll Edit that Issue accordingly.

Just to let you guys know, I'm currently reworking the D7 gnode access layer to match the one in D8 for a client. "Safety mode" will be buried in favor of "compliance mode" and all of the performance boosts recently introduced in D8 will be backported. This issue can then (finally) move forward and perhaps get committed.

This might need a reroll now that #2880603: Remove the "safety mode" and default to using the node access layer. + follow-ups have landed.

@kristiaanvandeneynde #71
Just to let you guys know, I'm currently reworking the D7 gnode access layer to match the one in D8 for a client. "Safety mode" will be buried in favor of "compliance mode" and all of the performance boosts recently introduced in D8 will be backported. Both this issue and the multiple parents issue can then (finally) move forward and perhaps get committed.

Thanks!! If I understand aright from something you said in #2847520-3: Proposal: New 2.x branch rewrite based on Relation module, the D8 gnode basically treats group-gnode-node relationships as fieldable “GroupContent” entities in their own right? Are you backporting that aspect as well? If so, this is going to be awesome!

I stand by what I said above: this really needs to be a new 7.x-2.x branch. I know that even the 7.x-1.x branch is still in beta and has never had a full stable release, but still people have likely being doing Rules and Views and maybe custom code and such that would depend on “Parent group” being a single scalar Group value rather than a List of Groups, and this would all be broken by such a major change to the underlying schema.

I noticed that a new 7.x-1.x-dev release came out the day of your comment. The Release Notes for it are uninformative (“…kittens may catch fire…”). What issues were actually addressed in it, and in particular, does it incorporate the #68 Patch in this issue or its functionality (from my quick perusal of the code in “…/group/modules/gnode/gnode.module” on a VirtualBox Debian 8 test installation, this does appear to be the case), and, if not, will that patch still successfully apply to it? Or does it fully implement the Fieldable Entities implementation of the D8 version? I need to know this before updating the live site to it. Thanks.

Okay, in my testing, it turns out that the new 2017-05-23 7.x-1.0-beta6+20-dev does not incorporate Multiple Parent Group functionality, and the patches in this Issue and my Child Issue #2858188-11: "Undefined group in gnode_node_access() line 306" after Patch #55 of Parent Issue when viewing gnode if logged in without “Bypass group access control” permission will not apply to it.

Anyone using this functionality, do not update to 7.x-1.0-beta6+20-dev! Not until the latest patch in #68 has been rerolled against it. If you do, you’ll lose Multiple Parent Group functionality! For now, I highly recommend those of you using Drush to execute: “drush up --lock=group --lock-message='LOCKED: Not compatible with Multiple Parent Groups yet!'” to keep from accidentally updating it until a new Multiple Parents Group patch has been rolled.

Is there any way I can get my text VirtualBox back to using the previous -dev version (I think it was 7.x-1.0-beta6+13-dev)? It’s no longer available for download from the Releases page nor via drush dl group --select --all. I suppose I can copy it over from my live site. But, there was some sort of database update ( Gnode 7001 Backport the Drupal 8 node access code for consistency.) as well which may be difficult to revert.

Telling people not to update is kind of counter-productive. They really should update, but this patch needs a reroll. Shouldn't be that hard. If anything, the access records and grants code is more easy to read now.

@kristiaanvandeneynde #75: Telling people not to update is kind of counter-productive. They really should update, but this patch needs a reroll. Shouldn't be that hard. If anything, the access records and grants code is more easy to read now.

After further playing with the new 7.x-1.0-beta6+20-dev in a VM, it appears to remove the Multiple Parent Groups admin UI, but does not break the assignments already made (for instance, a “List Nodes by Parent Group” View using a GID as its Contextual Argument will still list gnodes that were assigned to that parent, even if it wasn’t the first Parent Group assigned to that gnode). So it’s not as dangerous as I feared and apparently won’t cause actual data loss if you upgrade, which was what I was afraid would happen. You will lose the ability to do further assigns to multiple parent groups, though, until the patch is rerolled.

When I first saw your announcement in #71, I thought that you were backporting the D8 functionality to D7, not just the node access code. In particular, the GroupContent entities that connect Groups to Content as a many-to-many relationship linkage entity in D8 and which are themselves fieldable entities so that fields can be created on the particular relationship of a given Content node or other entity with a given Parent Group entity) would be highly desirable (see #2847520: Proposal: New 2.x branch rewrite based on Relation module for a scenario on how useful that could be) and would IMO be a better way to implement Multiple Parent Groups than this patch anyway. It’d also mean feature and internal structure parity between the two versions. Is there any reason that couldn’t be feasibly backported? Does it rely on some D8-only core stuff?

For one it uses the plugin system introduced in D8. Then there's the much more powerful Entity API, Symfony constraints and generally the fact that I do not have time to spend days on rewriting a D7 release :)

D8 is where it's at right now :P

If this issue gets a proper reroll, I might actually commit it and tag a 7.x-1.0 which will be final except for major bugs or security issues.

Oh, I agree that D8 is where it’s at. Now I only wish that we could convince the maintainers of certain other modules our project absolutely depends on of that fact. And the dependency modules that those modules would need. And so on.

Doesn't the Entity API module for D7 backport much of the D8 Entity functionality? You already require that as your sole dependency module for the D7 version. What does D8 Entities have that D7 + Entity API doesn’t that your D8 Group module needs?

On a reroll for this issue, whoever does it should also take a look into the Admin UX vertical tab on GNodes where it says “Not a group node.” Without this patch, the standard module just says what the one-and-only parent group is when there is one.

I propose that the tab should, in any case for a gnode, say “# parent group(s)” where “#” is the current count of parent groups assigned to that gnode, and which can be any non-negative integer, even “0” in the case of a former gnode whose last remaining parent group(s) were removed from it yet the gnode record remains. “Not a group node” should be used to distinguish nodes that do not have a gnode data structure at all, such as would be the case for a node which has never had a parent group.

IMO, this should be done before any final commit. Should I open another Child Issue for that aspect?

Doesn't the Entity API module for D7 backport much of the D8 Entity functionality?

Not even close. It's the other way around: D7 Entity API was a starting point for D8.

IMO, this should be done before any final commit. Should I open another Child Issue for that aspect?

Don't see why we can't add it to the patch here. It's part of the same goal: Make nodes support multiple parents and adapt the UI.

Then would it be a separate issue to provide a better user interface for assigning subgroups to multiple parents? This issue is specifically about nodes, but the basics should be the same, and as I recall the patch does implement a configuration user interface (at “…/admin/group/settings/entity” in the “Entities in multiple groups” subheading [last of three]) for allowing multiple parents for (sub)Groups as well as for (g)Nodes (that whole page, including the other subheadings, appears to be missing entirely in the new 7.x-1.0-beta6+20-dev).

I posted an important comment in the Child Issue #2858188-14: "Undefined group in gnode_node_access() line 306" after Patch #55 of Parent Issue when viewing gnode if logged in without “Bypass group access control” permission that anyone attempting a reroll should read.

To expand a bit on my #82: as I noted in #2858188-14: "Undefined group in gnode_node_access() line 306" after Patch #55 of Parent Issue when viewing gnode if logged in without “Bypass group access control” permission, this patch not only implements Multiple Parent Groups for Nodes, but also for Subgroups. The patch implements a “Group entity settings” admin tab at “…/admin/group/settings/entity”. The bottom section of this is as follows:

Entities in multiple groups

🗹 Group
🗹 Node
^{Choose if an entity can be in multiple groups. Entities in multiple groups can be updated and deleted by users with the necessary permissions in any of the groups it is in. After changing this session you must clear the site cache. It is not recommended that you go from multiple to single on a site which has entities in multiple groups without testing.}

(First off, in the help text, the word “session” (which I boldfaced above) should probably be “setting.”)

Most of what has been discussed here has been about the Nodes, but what about the Subgroups? At the present time, other than by creating a new Subgroup using the “Add <grouptype>” on the desired Parent Group’s homepage / landing page (“/group/#” where “#” is the desired Parent Group’s GID), I see no way to even see what the Parent Group(s) of a Subgroup is/are, let alone be able to add multiple Parent Groups to an existing Subgroup. There is no “Group settings” vertical tab when Editing a (Sub)Group like there is when Editing a (G)Node.

How does one go about assigning an already existing Group to be a Subgroup of another Group, even without having multiple such Parent Groups for a given SubGroup? And what does that “🗹 Group” checkbox on the “Group entity settings” admin tab actually do?

This, too, needs to be addressed before a final reroll.

(Changed Issue Title to reflect that this patch at least ostensibly adds Multiple Parent Group functionality to Subgroups as well as to Nodes.)

Is there ever going to be a version of this patch for the new -devs? Another one (7.x-1.0-beta6+21-dev) came out on June 6, and so far as I know, we weren’t ever told what it did differently from 7.x-1.0-beta6+20-dev, and this patch was never even rerolled for that!

Since the -dev releases other than the most recent one aren’t available even from the Releases page (or via drush dl group --select --all), that means that the one-and-only version that the most recent patch works against, namely, 7.x-1.0-beta6+13-dev, is no longer available to new users who may need this functionality, or who want to test it out. It’s been almost 1½ months since kristiaanvandeneynde posted #71ff announcing the 7.x-1.0-beta6+20-dev and my followups including #74 warning people not to upgrade to it if they use this patch, and there has been no activity on this issue or any child / related ones.

What can I do to help? My development experience isn’t with PHP, let alone Drupal (I did some pretty advanced stuff in ASP.NET VB.NET [very little in C#] back in the day), but I’m willing to learn. I’ve downloaded Aquia Desktop 2 and NetBeans IDE 8.2 (PHP/HTML5 edition) on a Win10 machine and have made two local copies of my site (one of which I intend to apply the latest -dev to). How would I go about re-rolling the patch for the current -dev? Where can I learn enough to be helpful here as quickly as possible?

My employer is willing to pay to help get this project moving. If we can’t get something here reasonably quickly, we may have to abandon Drupal entirely and go with something else (maybe ASP.NET) for this project. A D7 backport of the D8 functionality (which looks a lot closer to completion, having a Release Candidate as recommended version as I type this) would be best, and we’re willing to pay or otherwise chip in to help make that happen. Is there core stuff in D8 that your D8 version absolutely requires that isn’t available in D7 even with Entity API and likely won’t ever be?

I just posted the following in the first Child Issue #2858188-15: "Undefined group in gnode_node_access() line 306" after Patch #55 of Parent Issue when viewing gnode if logged in without “Bypass group access control” permission:

In reading up on the differences between Drupal 7 and 8 Entity APIs, D8 resolves this sort of thing by making everything a list. If I read and understood it right, all properties of all Entities are lists, even if it’s something like a Node’s “Title” that can logically only have one entry. D8 has constraints to enforce validations at more than just the form level, including enforcing minimum and maximum counts in the list.

I think that the D7 Group module should be written like the D8 one as much as possible, and Parent Group should be a list even if the checkbox to allow multiple Parent Groups is unchecked. That way all code to access parameters would access them as lists so there wouldn’t have to be constant tests of the configuration every time that parameter is referenced to see if it should be a list or a scalar, thus preventing such errors.

I’ll post more details in the Parent Issue thread.

In thinking more about this, I think the Parent Groups settings should be per-Group Type and per-GNode-Content-Type instead of system-wide. So, for instance, if you have Group Types of (for the USA) State, County, and City, plus a GNode Content Type of Shops (for, say, the purpose of calculating sales tax), then Shop and City could each be set to allow multiple Parent Groups (since a Shop would have to pay taxes to City, County, and State, whereas a large City can extend into more than one County), while County and State would not (no County is in more than one State). State would not be enabled to be a Subgroup at all.

This would enforce proper limits (through standard constraints in the D8 version, manually coded in the D7 version). In the Parent Groups tab for a County, the Parent Group would be shown as a single-entry drop-down or auto-complete (could also be radio buttons, but impractical in this case with fifty States), while the ones for City and Shop would be multiple-entry (either with an “Add Another Parent” button or using multi-select auto-complete widgets such as Chosen or AutoComplete Deluxe).

Without any background I have rerolled the patch.

It was pretty heavy as the last patch is 6 months old.

I tested it manually but have not configured my environment to run the automated tests yet.

Let me upload the actual patch as well..

No interdiff as there were file deletions and it would make no sense.

I found one issue when you are submitting a node from the group context (group/gid/node/add)

It can result in a duplicate entry.

Attached is a proposal to solve this issue.

@harings_rob, if I read that interdiff correctly (I’m still pretty green at PHP and even greener at Drupal coding — my background is in ASP.NET), the duplication would only happen if a default group were specified?

Thanks very much for these patches! I’ve been playing with the #86 one and it seems to work pretty good. I’ve not encountered the issue that #88 fixes, but I have already applied that on my development and test installations as well.

To all: I’ve added a feature basically swiped from Node View Permissions to implement its functionality for GNodes (now that Compatibility Mode is gone from the past two 7.x-1.x-dev versions and presumably all 8.x versions, any node access features that would be applied to GNodes would have to be implemented directly in Group or as some sort of contact access plugins for Group, so such additional modules won’t work with the latest Group 7.x-1.x-dev, with or without this patch).

My question: since I did it against the Multiple Parent Groups patched version, should I post it here? Or start a new Feature Request issue with this as Parent Issue? But it could also be applicable to the straight-up unpatched 7.x-1.x-beta6+21-dev, though I wouldn't be able to easily roll that as a feature patch since I don’t keep that version around nor would have ability to easily test it since my use case absolutely requires Multiple Parent Groups. So, should I create a new Issue that does not have this as Parent Issue? What’s the proper procedure here?

The basic idea is to relabel “[node_type]: View any content” to “[node_type]: View any published content” to more properly indicate what it does (since the new -dev also added a separate permission not found in -beta6+13-dev nor prior versions for “[node_type]: View any unpublished content”), then added two more duplicating those and replacing “any” with “own.” I modified gnode.module to implement the permissions in the admin forms (also made a tweak to display a “☓” in place of the checkbox for all “own” permissions on anonymous users since it’s a logical impossibility for them to be the owner of any content) and gnode.node_access.inc to actually implement those two new permissions using Node Grants (the actual Node View Permissions module, as well as Group 7.x-1.x-beta6+19-dev and previous, used the older deprecated NODE_ACCESS_ALLOW|DENY|IGNORE constants). It works so far with our use case (allowing Outsiders to view [as well as edit] their own GNodes but not anyone else’s), though I’m not familiar yet with the SimpleTest system and so have not crafted automated tests for it.

The Drupal 7 version is no longer supported, cleaning up the issue queue.