Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Allow creating per bundle/ per group roles #209

Open
wants to merge 5 commits into
base: default-roles
Choose a base branch
from

Conversation

amitaibu
Copy link
Owner

@amitaibu
Copy link
Owner Author

@pfrenssen this is just a beggining, but maybe can already give you a hint on my direction

*/
protected function createRoles($entity_type_id, $bundle_id) {
protected function createRoles($entity_type_id, $bundle_id = NULL, $group_id = NULL) {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pfrenssen I guess it is a bit ugly having to pass either bundle ID or group ID. Which made me thing that maybe we should have two Og Roles bundles: per_bundle and per_group so the schema will just have those two keys: group_type and reference_id (bad name) . reference_id will be populated either with the bundle ID or the group ID.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OgRole is a config entity, I don't think this supports bundles. If you look at the inherited method OgRole::bundle() you see it returns the entity type instead of the bundle. Only content entities support custom bundles out of the box.

But I actually think we can hack in bundle support easily. We just need to ensure that the bundle is stored along with the entity, that we have a $this->bundle property that we populate in the constructor, and then we can use it to handle per_bundle and per_group specific logic.

@amitaibu
Copy link
Owner Author

Here are some thoughts:

  1. Have two classes OgRolePerBundle (= the current OgRole) and OgRolePerGroup
  2. As proposed here change the schema, so we have target_id instead of bundle
  3. Rename role_type to required, and make it a boolean.
  4. OgRolePerBundle is now a Config entity which allows it to being exported. However it would make more sense to make OgRolePerGroup a Content entity, as it is non-exportable, and their could be thousands instances of it.

@pfrenssen what do you think about 4? Not sure how to tackle it, and still be able to share code between the classes.

@pfrenssen pfrenssen self-assigned this Aug 6, 2016
@pfrenssen
Copy link
Collaborator

Assigning to me so I won't forget to review this and answer the questions.

@amitaibu
Copy link
Owner Author

amitaibu commented Aug 23, 2016

A possible solution to having to reference both Config and Content is simply to have two different references fields on OgMembership.

So OgMembership::getRoles() will be responsible on looking at the right field according to the group's settings.

This means we can break it to smaller PRs:

  1. Change the schema to be $entity_type_id and $identifier instead of the $entity_type/$bundle/$group_id
  2. Add the OgRolePerGroup and a matching reference (base) field on OgMembership
  3. Bake the logic into OgMembership::getRoles() and family

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants