This is a feature request for organic group roles and permissions features integration.

Similar to how core roles and user permissions work with features we want the same functionality in og.

I'm willing to take the lead on implementation of this. I am mainly putting this issue here so others may find it and to minimize duplication of work in case this already exists or someone else has started on it.

I'm thinking this should live as a separate module, perhaps og_features which provides the implementation details for features. If it makes sense I could also open up a standalone project page if the maintainers would prefer.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jgraham’s picture

Status: Active » Needs review
FileSize
7.56 KB

Attached is a patch which adds an additional module og_features to og after enabling this module you should be able to manage og roles and og permissions via features.

Please advise if this is likely to make it into og_core and if not I will move it to its own module space.

edit: patch is against 7.x-1.x

amitaibu’s picture

Thank you for working on this

> Please advise if this is likely to make it into og_core and if not I will move it to its own module space.

Yes, it will be in Og core. I'm currently a bit stressed with time, so won't have time this week to review.

BarisW’s picture

Any updates yet Amitaibu? We'd like to have this as well ;)

Status: Needs review » Needs work

The last submitted patch, og-features-integration-1349754-1.patch, failed testing.

jgraham’s picture

Status: Needs work » Needs review
FileSize
7.45 KB

re-roll of patch, git apply won't create missing directories.

BassistJimmyJam’s picture

This is exactly what I was looking for! The last patch worked great, I was able to export my permissions and properly revert the feature after making changes. I was looking it over and the only thing that jumped out at me was an extra line of whitespace:

/**
 * Implements hook_features_export().
 */

function og_features_role_features_export($data, &$export, $module_name = '') {
jgraham’s picture

Follow up with whitespace fix from 6.

Grayside’s picture

On a quick scan it looks good.

amitaibu’s picture

Haven't tested, but from quick look it looks good. I think its best if this started as a contrib module outside of the OG package.

BarisW’s picture

Status: Needs review » Reviewed & tested by the community

Looks good here as well.

But please not another module.. Every other module with features integration does it in the module itself, why should OG differ from this? It would be awesome if OG settings were exportable out-of-the-box.

jgraham’s picture

I think its best if this started as a contrib module outside of the OG package.

Amitai, just to clarify you want me to create a contrib module? Your comment in 9 is counter to what you indicated in comment 2, I'm fine either way, I just want to make sure there's no confusion before I create a new project for this.

ressa’s picture

It would be great to have Features integration in Organic Groups, much appreciated :-)

amitaibu’s picture

>But please not another module.. Every other module with features integration does it in the module itself,
> Amitai, just to clarify you want me to create a contrib module? Your comment in 9 is counter to what you indicated in comment 2,

Indeed, the reasoning is this (but I'm open to hear what you guys think). With #1342632: Deprecate OG group entity the feature integration will need to change, as we have the global roles and permissions, but also per-bundle permissions. This means the features integration might need to change.

So what I'm thinking is:

a) Work on features integration for #1342632, make sure it will work there, and only after that commit this patch
b) Instead of letting this great work rot in the issue queue until #1342632 is out, we can have it in contrib, and later on if needed, move into OG core itself.
c) commit this patch, after a code review -- and nag jgraham to make sure it's ported to #1342632

I have a feeling most will say (c), but Jeff's thought is probably the important one, as he's doing all the hard work ;)

jgraham’s picture

Options a or c sounds good to me.

I'm hoping to get some free time in the next week to review #1342632: Deprecate OG group entity anyway so that will be a good opportunity to bring this current with that branch. That should hopefully mean minimal nagging necessary ;)

amitaibu’s picture

> I'm hoping to get some free time in the next week to review

Great! so I think it's best if we take a decision after you had a chance to look at it.

jgraham’s picture

Version: 7.x-1.x-dev » 7.x-2.x-dev
Status: Reviewed & tested by the community » Needs work

So the existing patch works for the global permissions.

Now we need to account for entity type and bundle type permissions.

I'm leaning towards a group type and bundle type aware integration so that if you export a "clubs" node type that is an organic group you can also export its group permissions as standalone and hopefully avoid dependency soup. So in addition to the exportables in the existing patch I will add an additional permissions export chain that will handle the entity type and bundle specific permissions.

I'm open to input if anyone has any thoughts on this issue, but in the meantime I will be jumping in on an implementation for the 2.x branch that handles the entity type and bundle defaults.

jgraham’s picture

Status: Needs work » Needs review
FileSize
14.27 KB

Updated for the 2.x branch.

This has an additional bundle exporter that allows you to export configuration for bundle (aka nodetype) specific default permissions.

This also now does dependency injection to add roles when including permissions with non-standard og roles.

Status: Needs review » Needs work

The last submitted patch, og-features-1349754-17.patch, failed testing.

jgraham’s picture

Status: Needs work » Needs review
FileSize
14.3 KB

re-roll of last patch with proper adding of new files.

Status: Needs review » Needs work

The last submitted patch, og-features-1349754-19.patch, failed testing.

jgraham’s picture

Test failed at - Organic groups field access (OgFieldAccessTestCase) I'm pretty sure that has to be unrelated to this patch.

Is there a way to review testbot output on an unmodified branch/release?

amitaibu’s picture

@jgraham,
maybe you can try the failing test locally? (If not I can try it in a couple of days in my local).

Also, what do you think, should it be a separate module in OG package, or part of OG core? I'm a bit on the fence here.

jgraham’s picture

I'll fire up a local test when I get a chance.

Re: separate module.

I don't have a strong opinion on either approach, but am in favor of a separate module. The only argument against a separate module, that I can come up with, is it's a separate module. That is, as a separate module it means another item at admin/modules, another dependency, but these all seem trivial. While we use features on nearly all of the sites we build there are numerous others who do not. As such it's nice to not force the extra code bloat on somebody who doesn't need it. I'm sure we can mitigate a lot of this with conditional loading, but depending on the implementation it seems that a separate module is the simpler easier to maintain solution. Also features inject their dependencies when building the feature so end-users should be minimally impacted by an additional module. If they need it, it will be enabled indirectly.

Grayside’s picture

Most modules don't have a separate export module. I'd follow that lead. The helper functions should be retained in some easily accessible space, the rest, include file. If Features is installed, they're likely to need it.

scottrigby’s picture

@amitai @jgraham nice work!
I'm with Grayside though - adding the feature hooks to og.features.inc would lead to less confusion i think.
I can see why something like og_access should be in a separate module, but features hooks themselves are only called as needed - not very heavy overall.

jgraham’s picture

Yeah, makes sense and I forgot about hook_features_api() having a file parameter.

This mitigates my only real issue with the same module so I'll rework the patch to include hook_features_api() in og.module and then have the remaining code in corresponding component files (eg. og_features_role.features.inc, og_features_permission.features.inc, og_features_bundle_permission.features.inc)
which will be dynamically included by features as needed.

This approach seems consistent with other implementations.

jgraham’s picture

Status: Needs work » Needs review
FileSize
15.02 KB

I re-ran the failing test locally and it completed successfully. I'm assuming this was something that has since been fixed, but I believe the previous failure is still unrelated to this patch.

Attached patch is reworked to reflect comment 26

amitaibu’s picture

Status: Needs review » Needs work

Sorry for the late review. Haven't tested yet, just from reading code:

+++ b/includes/og_features_bundle_permissions.features.incundefined
@@ -0,0 +1,181 @@
+    list($entity_type, $bundle_type, $perm) = explode(':', $key);  // key is enity_type:bundle:permission

Move comment above line.

+++ b/includes/og_features_bundle_permissions.features.incundefined
@@ -0,0 +1,181 @@
+      // dependency injection on og_roles

Comment should start in capital letter and end with dot. (Here and other comments).

+++ b/includes/og_features_bundle_permissions.features.incundefined
@@ -0,0 +1,181 @@
+  // prune default roles from dependencies
+  $nondefault_roles = _og_features_get_roles(FALSE);

Can you better explain what we are doing here.
Using our jargon, we have 3 level of roles:

- Global roles
- per-bundle
- per-group

^^unless you have better name for them :)

+++ b/includes/og_features_bundle_permissions.features.incundefined
@@ -0,0 +1,181 @@
+ * Helper function to get all specified roles by group bundle and group type

- "all specified" => "all"
- Missing dot.

+++ b/includes/og_features_bundle_permissions.features.incundefined
@@ -0,0 +1,181 @@
+    // don't export code if no roles have this permission

This comment isn't very clear.

+++ b/includes/og_features_bundle_permissions.features.incundefined
@@ -0,0 +1,181 @@
+  return (array('og_default_bundle_permissions' => $code));

(array( => array(

+++ b/og.moduleundefined
@@ -3180,3 +3180,69 @@ function og_operations_load_action_includes() {
+  ->fields('r')
+  ->condition('name', $name)
+  ->condition('gid', 0)
+  ->condition('group_type', $group_type)
+  ->condition('group_bundle', $bundle)
+  ->execute()

2 space needed.

+++ b/og.moduleundefined
@@ -3180,3 +3180,69 @@ function og_operations_load_action_includes() {
+ * Generate $rid => $role with role names untranslated

What do you mean by translated?

Morten Najbjerg’s picture

Nice work on getting permissions and roles into features! Much appreciated.

It seems as if the patch in #27 adds a dependency for og_features module - even though this module is not created.
The paths for .inc files also seems to be wrong. They are placed in og's root folder and not the includes directory.

dtarc’s picture

FileSize
14.84 KB

Rerolling the patch from #27 with the following changes:

- Fixed a typo in og_features_permission_features_rebuild() that was preventing the permissions from reverting

- Removed the dependency on og_features mentioned in #29.

- Syntax cleanup of comments mentioned in #28

dtarc’s picture

Status: Needs work » Needs review
jgraham’s picture

Finally getting back to this.

dtarc, thanks for the re-roll and catching that typo I must have messed that up when I saved it because I did have it working before.

@amitaibu

$nondefault_roles = _og_features_get_roles(FALSE);

Can you better explain what we are doing here.
Using our jargon, we have 3 level of roles:

- Global roles
- per-bundle
- per-group

Here we want all of the roles that are *not* provided by OG by default. These roles should always be injected as dependencies since they are not provided by default. Does that make sense? Perhaps there is an existing og method that we should call instead to accomplish this?

+ * Generate $rid => $role with role names untranslated

What do you mean by translated?

Here, I mean the role names without any localization or translation, we want a machine name. We need some non-serial way to refer to these roles outside the context of a particular site install the untranslated name is the most reliable way to accomplish this.

// don't export code if no roles have this permission

This comment isn't very clear

Essentially we are checking to see if any roles have the particular permission assigned to them. If no roles have a given permission then we don't bother exporting the corresponding permissions code as it would just be an empty array() and totally superfluous. I'm not sure how to keep this brief but still informative.

Let me know if more clarification is needed on these, any other points or if you have language suggestions for these remaining details.

jgraham’s picture

Status: Needs review » Needs work

Setting to needs work I just ran into an issue where bundle level features permissions are not exported.

The file is not being created when attempting to export bundle level permissions, however the appropriate entries are created in the .info file.

dtarc’s picture

The patch in #30 was still the closest we've had and we've been using it for a site that we have launched in beta (not using bundle permissions).

Recently I updated from the alpha2 to a dev snapshot and found that the patch is calling functions that no longer exist, due to API changes.

This is a straight reroll of #30 with changes to two function calls:

og_get_global_permissions() becomes og_get_default_permissions

og_get_global_roles() becomes og_get_default_roles()

I can see already that there will be things to change in this patch to get the default & bundle roles & permissions featurized. Again this is just a straight reroll of 30 to fix the deprecated function names. This patch does not have complete functionality.

dtarc’s picture

FileSize
14.85 KB

Oops, here's the patch

jgraham’s picture

Status: Needs work » Needs review
FileSize
14.92 KB

Re-roll of 35 with one change to fix a typo on line 26 of og_features_bundle_permissions.inc this resolves the issue described in comment 33.

Status: Needs review » Needs work

The last submitted patch, og_features-1349754-36.patch, failed testing.

jgraham’s picture

Status: Needs work » Needs review
FileSize
14.87 KB

Fixed patch headers for new files. Re-roll of 36.

dtarc’s picture

Status: Needs review » Needs work

Thanks jgraham.

Have we lost featurized roles along the way?

amitaibu’s picture

I can see already that there will be things to change in this patch to get the default & bundle roles & permissions featurized

Indeed, there was an API/ concept change -- we no longer have the idea of "global roles/ permissions". we now have:

  • default roles/permissions -- The hook implementations.
  • per bundle -- instead of the global role, each role/ permissions is assigned to an existing group typs
jgraham’s picture

@amitai thanks for clarifying I will rework this integration to reflect the API design change as there are is some superfluous code due to this change.

jgraham’s picture

Status: Needs work » Needs review
FileSize
10.69 KB

rewrite of the patch to reflect the recent changes in the API around deprecation of global roles.

This will need some additional work once #1616950: og_get_default_roles() does not return values as documented instead returns roles keyed by *arbitrary* integer is complete we will either need to discover our own module implementors or og will provide the additional role to module details. This is needed for proper dependency injections in og_features_role_features_export() for modules implementing hook_og_default_roles().

I've only done a minimal level of testing, I plan on doing more, but wanted to get this out in the wild so more eyes are on it.

jhedstrom’s picture

Status: Needs review » Needs work

Patch in #42 has a stray call to dpm() left over.

When installing from an install profile, none of the exported components (OG roles or permissions) appear to be getting picked up. I'm digging into this now.

When exporting into 2 features, and roles are exported into feature A, then a few permissions are exported to feature B, the code additionally exports the already exported roles, causing a conflict between the 2 features.

jhedstrom’s picture

This patch removes the dpm(), uncomments a call to og_role_save, and changes the two render hooks to use the default hook specified in hook_features_api().

With this patch, I'm seeing roles come through, but assigned permissions are still not working.

jhedstrom’s picture

Status: Needs work » Needs review

Actually, with the patch in #44, permissions are exporting properly (I hadn't updated my feature to use the hook names defined in og_features_api()).

jgraham’s picture

Reworked the patch in 44:

- Always export permission code even when no roles have a given permission. This prevents potential duplicate options and ensures we can discover all overrides.
- Removed xdebug_break() call.
- Omit roles provided by hook_default_roles() from showing up as options to export. Instead these will be added as dependencies to the module providing the role via hook_og_default_roles. This implementation is dependant on the resolution of #1616950: og_get_default_roles() does not return values as documented instead returns roles keyed by *arbitrary* integer. If that issue is committed then this patch needs to adjust all code where og_get_default_permissions() is called, if not it may need to resolve its own module dependency tree.

lund.mikkel’s picture

Version: 7.x-2.x-dev » 7.x-2.0-alpha3
Assigned: jgraham » Unassigned
Category: feature » bug

I've apply the latest patch (46), but I now get a lot(!) of notices, when I visit admin/structure/features:

Notice: Undefined offset: 2 in og_features_permission_features_export_render() (line 78 of sites/all/modules/contrib/og/includes/og_features_permission.features.inc)

I have little insight into what happens, so I don't know what to do. Anybody knows how to fix this issue?

arosboro’s picture

I'm interested in this becoming core to og or as a contrib module, I would love to package permissions into my features

Taxoman’s picture

Version: 7.x-2.0-alpha3 » 7.x-2.x-dev
Category: bug » feature
amitaibu’s picture

I'd really like the community to test it out properly, as I myself don't have a use case for exporting OG-permissions. (I'm going to commit it of course when it's ready, but want it tested)

arosboro’s picture

Hi Amitaibu, I'm going to test this in the features I produce for a production organic groups site, and I'll keep this thread updated.

I'm having a little bit of trouble getting my features to act appropriately in group spaces (they remain enabled even if disabled in the group, and the create content links are always visible even if disabled. I'll post an issue in the appropriate issue queue... most likely the features queue. Have you experienced this when rolling your own features for OG 7?

arosboro’s picture

Cleared the cache after applying patch and got this error site-wide:

Error messagePDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'ogr.group_type' in 'field list': SELECT ogr.group_type AS group_type, ogr.group_bundle AS group_bundle, ogr.name AS name, ogrp.permission AS permission FROM {og_role} ogr INNER JOIN {og_role_permission} ogrp ON ogr.rid = ogrp.rid WHERE (ogr.gid = :db_condition_placeholder_0) ORDER BY ogr.group_type ASC, ogr.group_bundle ASC, ogrp.permission ASC; Array ( [:db_condition_placeholder_0] => 0 ) in _og_features_get_permissions() (line 172 of public_html/sites/all/modules/contrib/og/includes/og_features_permission.features.inc).

Will post back with info when I figure out why group_type column is unknown.

arosboro’s picture

Nevermind, #52 I was using og-7.x-1.4. It only has rid, gid, and name columns in the og_role table.

I'll backup my db and upgrade to the dev branch, so I can help out in testing.

arosboro’s picture

Looks like I'll have to test in a separate sandbox, I didn't realize that other contrib modules are still relying on og_get_group() which was replaced in 2.x.

arosboro’s picture

jgraham, how are you using features in organic groups? spaces_og hasn't been ported to support 2.x yet

jgraham’s picture

@arosboro: We aren't currently using spaces in our projects. For an example distribution using organic groups take a look at julio where we use organic groups to control access to various portions of the site build. Additional documentation is available with specific og details covered in Creating and Managing User Groups in Julio. Keep in mind that julio is still a development release.

arosboro’s picture

I'd like my features to be group specific, so I patched spaces to work with the latest og beta released yesterday. I'll start testing out your patch now. See http://drupal.org/node/1668636#comment-6187616 for the patch.

arosboro’s picture

@jgraham, I've applied your patch, but when enabling a feature that uses og permissions I no longer can access any groups as user 1. I've tried adding administer groups to the permissions list but it didn't let me into the group.

arosboro’s picture

#58 was related to a spaces issue, not this patch... I've got julio set up on my laptop, and I'm looking at how it uses og_features integration

arosboro’s picture

@jgraham, I see how julio is using this patch to export permissions on a site wide level. I understand that it's not trivial to export permissions as group overrides, but that is what I'm after. What would be the best method of achieving this? I've considered exporting permissions from the bundle with an overridden option in features that would turn the field_data_og_roles_permissions value to 1, and possibly require that this field be packaged with the feature.

Here is some pseudo code:

Group override roles

Determine whether field_data_og_roles_permissions has a row for entity_id of group_bundle with og_roles_permissions_value set to 1:

Use entity object in group space to check if $entity->og_roles_permissions[LANGUAGE_NONE][0][value] == 1
else set to 1

Retrieve rid, gid, name, group_type, group_bundle from og_role

og_roles($group_type, $bundle, $gid);

Set permissions using rid, permissions array arguments for og_role_change_permissions();

We'd have to get the group from spaces to determine how to apply the override. I can make modifications required, but would like to have your thoughts on where to copy permissions from for the export, and how to apply the override.

arosboro’s picture

I solved the issue I was attempting to resolve by using og_permissions. Instead I found that I can use spaces_menu_access() in place of node_access to determine which links appear in the create content menu.

I'm not going to be overriding my group permissions, so this patch seems fine the way it is. I'll update if I discover any issues.

jhedstrom’s picture

Same patch as #46, but replaced __DIR__ with dirname(__FILE__) for php 5.2 compatibility.

amitaibu’s picture

Status: Needs review » Needs work

I'd like to commit it, but some minor code style fixes are needed

+++ b/includes/og_features_permission.features.incundefined
@@ -0,0 +1,197 @@
+        // Dependency injection on og_roles.

Lets call it piping, it's not really DI I think.

+++ b/includes/og_features_role.features.incundefined
@@ -0,0 +1,125 @@
+        // provided by some other module add as a dependency
+        // waiting on resolution of http://drupal.org/node/1616950

Is this still valid?

+++ b/includes/og_features_role.features.incundefined
@@ -0,0 +1,125 @@
+      // add the role to the export since its not provided by a module

Code style (Captial + end dot).

+++ b/includes/og_features_role.features.incundefined
@@ -0,0 +1,125 @@
+ * @param $name string the role name
+ * @param $group_type the entity type
+ * @param $bundle the bundle
+ * @return mixed integer indicating rid if the role exists FALSE otherwise

Coding style.

+++ b/includes/og_features_role.features.incundefined
@@ -0,0 +1,125 @@
+  else {

No need for else{} just return FALSE.

jgraham’s picture

Status: Needs work » Needs review
FileSize
12.05 KB

I've fixed all of the aforementioned coding style issues as well as quite a few others mostly related to whitespace.

Fixed extraneous else.

I think that #1616950: og_get_default_roles() does not return values as documented instead returns roles keyed by *arbitrary* integer can be closed as "closed (won't fix)" unless the rework that returns the module implementing the og role would be useful elsewhere, in which case we should resolve 1616950 and then update this patch to use the solution from that issue. As it stands now, it looks like this patch would be the only code, at least that I'm aware of, that would benefit from og_get_default_roles() returning the implementing module along with the roles. As such in this latest iteration I have assumed that #1616950: og_get_default_roles() does not return values as documented instead returns roles keyed by *arbitrary* integer will not be implemented and added _og_features_role_dependencies() to discover any additional module dependencies via implementers of hook_og_default_roles().

Edit: this is a re-roll of the patch in 62.

amitaibu’s picture

Status: Needs review » Fixed

And... its in :) Thank you all!

jgraham’s picture

Thanks Amitai and everyone else!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.