amitaibu / og

A fork to work on OG8
https://github.com/Gizra/og
29 stars 16 forks source link

Let PermissionManager handle permission filtering #238

Closed pfrenssen closed 8 years ago

pfrenssen commented 8 years ago

This is a follow-up of #237.

In https://github.com/amitaibu/og/pull/217#issuecomment-225639354 @amitaibu proposed to improve the way default roles for a new group type are created.

Currently GroupManager::createPerBundleRoles() fires the PermissionEvent for each of the default roles and filters the permissions, and then proceeds to create and save OgRole 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:

  protected function createPerBundleRoles($entity_type_id, $bundle_id) {
    foreach ($this->getDefaultRoles() as $role_name => $default_properties) {
      $properties = [
        'group_type' => $entity_type_id,
        'group_bundle' => $bundle_id,
        'id' => $role_name,
        'role_type' => OgRole::getRoleTypeByName($role_name),
      ];

      // Populate the default permissions.
      $event = new PermissionEvent($entity_type_id, $bundle_id);
      /** @var \Drupal\og\Event\PermissionEventInterface $permissions */
      $permissions = $this->eventDispatcher->dispatch(PermissionEventInterface::EVENT_NAME, $event);
      $properties['permissions'] = array_keys($permissions->filterByDefaultRole($role_name));

      $role = $this->ogRoleStorage->create($properties + $default_properties);
      $role->save();
    }
  }

In #237 a first step is taken to solve this. In that issue the creation of the OgRole objects is delegated to the DefaultRoleEvent, so that createPerBundleRoles() no longer needs to compile the role properties and create the objects, reducing its complexity:

  protected function createPerBundleRoles($entity_type_id, $bundle_id) {
    foreach ($this->getDefaultRoles() as $role) {
      $role->setGroupType($entity_type_id);
      $role->setGroupBundle($bundle_id);

      // Populate the default permissions.
      $event = new PermissionEvent($entity_type_id, $bundle_id);
      /** @var \Drupal\og\Event\PermissionEventInterface $permissions */
      $permissions = $this->eventDispatcher->dispatch(PermissionEventInterface::EVENT_NAME, $event);
      foreach (array_keys($permissions->filterByDefaultRole($role->getName())) as $permission) {
        $role->grantPermission($permission);
      }
      $role->save();
    }
  }

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:

  protected function createPerBundleRoles($entity_type_id, $bundle_id) {
    foreach ($this->getDefaultRoles() as $role) {
      $role->setGroupType($entity_type_id);
      $role->setGroupBundle($bundle_id);

      foreach (array_keys($this->permissionManager->getDefaultPermissions($role)) as $permission) {
        $role->grantPermission($permission);
      }

      $role->save();
    }
  }
pfrenssen commented 8 years ago

What is interesting here is that this refactoring uncovers a circular dependency that currently exists between GroupManager and PermissionManager.

In the current implementation, GroupManager::createPerBundleRoles() invokes the PermissionEvent and passes it the group entity type and bundle. However, to generate the permissions for the group content the PermissionManager needs to know which group content bundles are associated with the group, so at the moment PermissionManager::getPermissionList() calls GroupManager::getGroupContentBundleIdsByGroupBundle(). So we have a circular dependency chain GroupManager -> PermissionEvent -> PermissionManager -> GroupManager.

This is obviously a very bad idea. The solution is simple though, GroupManager should supply PermissionEvent with the data it needs to gather its permissions, so it should simply pass the group content bundle array to it.

This increases the size of this PR by quite a bit, but the overall complexity is now be reduced.

pfrenssen commented 8 years ago

This has been cleaned up and rebased. Next step is to merge in the latest changes from 8.x-1.x and #237.

pfrenssen commented 8 years ago

Addressed remarks, ready for review!

amitaibu commented 8 years ago

Thanks!