-
Notifications
You must be signed in to change notification settings - Fork 16
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
Let PermissionManager handle permission filtering #238
Conversation
// ->willReturn([]) | ||
// ->shouldBeCalled(); | ||
// return $role_name; | ||
// }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the mess. It took a bit more time than expected to get the tests green yesterday evening and I had to rush out to catch my train before I was ready with the clean up.
I'll clean this up now and rebase the branch so this unholy mess disappears.
What is interesting here is that this refactoring uncovers a circular dependency that currently exists between In the current implementation, This is obviously a very bad idea. The solution is simple though, This increases the size of this PR by quite a bit, but the overall complexity is now be reduced. |
…ty arrays around. This method that provides properties to construct a default role has no real value any more, it was only used by GroupManager and its test. Let's move it there.
This is not something that GroupManager should be concerned with.
66cb591
to
e7ce88a
Compare
This has been cleaned up and rebased. Next step is to merge in the latest changes from 8.x-1.x and #237. |
…ring # Conflicts: # og.services.yml # src/GroupManager.php # src/PermissionManager.php # tests/src/Unit/GroupManagerTest.php
use Drupal\og\Event\DefaultRoleEvent; | ||
use Drupal\og\Event\DefaultRoleEventInterface; | ||
use Drupal\og\Event\PermissionEvent; | ||
use Drupal\og\Event\PermissionEventInterface; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay reduced dependencies!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -58,6 +58,13 @@ public static function getSubscribedEvents() { | |||
* The OG permission event. | |||
*/ | |||
public function provideDefaultOgPermissions(PermissionEventInterface $event) { | |||
// @todo This currently sets both the group level and group content level |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will be addressed in #240 right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this is already reworked in #240. In that issue two separate calls are done to $event->setPermissions()
: one to set the group-level permissions and one for the generic entity operation permissions.
In a future PR this will get a third call even: to provide the entity operation permissions for the core entity types like node, block etc.
* The entity type ID of the group for which to return permissions. | ||
* @param string $group_bundle_id | ||
* The bundle ID of the group for which to return permissions. | ||
* @param array $group_content_bundle_ids |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not clear about $group_content_bundle_ids
- why do we need this in this context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getDefaultPermissions()
returns all permissions that are enable by default for a given role. This is used when a new group type is created: we need to populate a default set of permissions initially, mainly to give non-members the right to subscribe, members the right to leave, and administrators the right to do everything.
For the moment this covers both the group-level permissions and the entity operation permissions. I agree that the entity operation permissions don't make sense when creating a new group type, since the group won't have any group content types yet. I don't remember why I put this in initially. I'll see if I can remove this parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I remember why I put this in. Currently getDefaultPermissions()
is the only 'entry point' to the PermissionEvent
event subscriber. So this was the only place where I could pass the $group_content_bundle_ids
to the event subscriber in the current code base.
This indeed doesn't make sense in this context. We only need to pass the group content bundle IDs when we create a new group content type. This is something we can do after #187 is in. We will then probably add a new method getDefaultEntityOperationPermissions()
which we can use to populate the initial permissions for the new group content type.
I will remove it for now.
…actor-permission-filtering
It doesn't make sense in this context. This is called when a new group type is created, at which point there are no group content bundles yet.
… tests to fail." This reverts commit 57aa02c.
Addressed remarks, ready for review! |
Thanks! |
This is a follow-up of #237.
In #217 (comment) @amitaibu proposed to improve the way default roles for a new group type are created.
Currently
GroupManager::createPerBundleRoles()
fires thePermissionEvent
for each of the default roles and filters the permissions, and then proceeds to create and saveOgRole
entities using the properties returned by the event.This results in a lot of actions related to roles and permissions being handled by
GroupManager
which is not really in scope of its responsibility, which is managing groups:In #237 a first step is taken to solve this. In that issue the creation of the
OgRole
objects is delegated to theDefaultRoleEvent
, so thatcreatePerBundleRoles()
no longer needs to compile the role properties and create the objects, reducing its complexity:In this PR a second iteration will be done to reduce the responsibility of this method: the permission handling will be factored out and moved to the
PermissionManager
where it belongs. The goal of this PR is to end up with a method that is only concerned with actually applying its arguments and the default permissions to the roles, looking similar to this: