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.
Comment | File | Size | Author |
---|---|---|---|
#64 | og-features-integration-1349754-64.patch | 12.05 KB | jgraham |
#62 | og-features-integration-1349754-62.patch | 11.13 KB | jhedstrom |
#46 | og-features-integration-1349754-46.patch | 11.12 KB | jgraham |
#44 | og-features-integration-1349754-44.patch | 10.71 KB | jhedstrom |
#42 | og-1349754-42a.patch | 10.69 KB | jgraham |
Comments
Comment #1
jgraham CreditAttribution: jgraham commentedAttached 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
Comment #2
amitaibuThank 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.
Comment #3
BarisW CreditAttribution: BarisW commentedAny updates yet Amitaibu? We'd like to have this as well ;)
Comment #5
jgraham CreditAttribution: jgraham commentedre-roll of patch, git apply won't create missing directories.
Comment #6
BassistJimmyJam CreditAttribution: BassistJimmyJam commentedThis 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:
Comment #7
jgraham CreditAttribution: jgraham commentedFollow up with whitespace fix from 6.
Comment #8
Grayside CreditAttribution: Grayside commentedOn a quick scan it looks good.
Comment #9
amitaibuHaven'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.
Comment #10
BarisW CreditAttribution: BarisW commentedLooks 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.
Comment #11
jgraham CreditAttribution: jgraham commentedAmitai, 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.
Comment #12
ressa CreditAttribution: ressa commentedIt would be great to have Features integration in Organic Groups, much appreciated :-)
Comment #13
amitaibu>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 ;)
Comment #14
jgraham CreditAttribution: jgraham commentedOptions 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 ;)
Comment #15
amitaibu> 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.
Comment #16
jgraham CreditAttribution: jgraham commentedSo 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.
Comment #17
jgraham CreditAttribution: jgraham commentedUpdated 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.
Comment #19
jgraham CreditAttribution: jgraham commentedre-roll of last patch with proper adding of new files.
Comment #21
jgraham CreditAttribution: jgraham commentedTest 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?
Comment #22
amitaibu@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.
Comment #23
jgraham CreditAttribution: jgraham commentedI'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.
Comment #24
Grayside CreditAttribution: Grayside commentedMost 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.
Comment #25
scottrigby@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.
Comment #26
jgraham CreditAttribution: jgraham commentedYeah, 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.
Comment #27
jgraham CreditAttribution: jgraham commentedI 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
Comment #28
amitaibuSorry for the late review. Haven't tested yet, just from reading code:
Move comment above line.
Comment should start in capital letter and end with dot. (Here and other comments).
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 :)
- "all specified" => "all"
- Missing dot.
This comment isn't very clear.
(array( => array(
2 space needed.
What do you mean by translated?
Comment #29
Morten Najbjerg CreditAttribution: Morten Najbjerg commentedNice 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.
Comment #30
dtarc CreditAttribution: dtarc commentedRerolling 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
Comment #31
dtarc CreditAttribution: dtarc commentedComment #32
jgraham CreditAttribution: jgraham commentedFinally 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
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?
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.
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.
Comment #33
jgraham CreditAttribution: jgraham commentedSetting 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.
Comment #34
dtarc CreditAttribution: dtarc commentedThe 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:
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.
Comment #35
dtarc CreditAttribution: dtarc commentedOops, here's the patch
Comment #36
jgraham CreditAttribution: jgraham commentedRe-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.
Comment #38
jgraham CreditAttribution: jgraham commentedFixed patch headers for new files. Re-roll of 36.
Comment #39
dtarc CreditAttribution: dtarc commentedThanks jgraham.
Have we lost featurized roles along the way?
Comment #40
amitaibuIndeed, there was an API/ concept change -- we no longer have the idea of "global roles/ permissions". we now have:
Comment #41
jgraham CreditAttribution: jgraham commented@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.
Comment #42
jgraham CreditAttribution: jgraham commentedrewrite 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.
Comment #43
jhedstromPatch 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.
Comment #44
jhedstromThis 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.
Comment #45
jhedstromActually, 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()).
Comment #46
jgraham CreditAttribution: jgraham commentedReworked 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.
Comment #47
lund.mikkel CreditAttribution: lund.mikkel commentedI'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?
Comment #48
arosboro CreditAttribution: arosboro commentedI'm interested in this becoming core to og or as a contrib module, I would love to package permissions into my features
Comment #49
Taxoman CreditAttribution: Taxoman commentedComment #50
amitaibuI'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)
Comment #51
arosboro CreditAttribution: arosboro commentedHi 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?
Comment #52
arosboro CreditAttribution: arosboro commentedCleared 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.
Comment #53
arosboro CreditAttribution: arosboro commentedNevermind, #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.
Comment #54
arosboro CreditAttribution: arosboro commentedLooks 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.
Comment #55
arosboro CreditAttribution: arosboro commentedjgraham, how are you using features in organic groups? spaces_og hasn't been ported to support 2.x yet
Comment #56
jgraham CreditAttribution: jgraham commented@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.
Comment #57
arosboro CreditAttribution: arosboro commentedI'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.
Comment #58
arosboro CreditAttribution: arosboro commented@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.
Comment #59
arosboro CreditAttribution: arosboro commented#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
Comment #60
arosboro CreditAttribution: arosboro commented@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.
Comment #61
arosboro CreditAttribution: arosboro commentedI 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.
Comment #62
jhedstromSame patch as #46, but replaced
__DIR__
withdirname(__FILE__)
for php 5.2 compatibility.Comment #63
amitaibuI'd like to commit it, but some minor code style fixes are needed
Lets call it piping, it's not really DI I think.
Is this still valid?
Code style (Captial + end dot).
Coding style.
No need for else{} just return FALSE.
Comment #64
jgraham CreditAttribution: jgraham commentedI'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.
Comment #65
amitaibuAnd... its in :) Thank you all!
Comment #66
jgraham CreditAttribution: jgraham commentedThanks Amitai and everyone else!