Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion og.services.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,17 @@ services:
og.access:
class: Drupal\og\OgAccess
arguments: ['@config.factory', '@current_user', '@module_handler']
og.default_role_event:
class: Drupal\og\Event\DefaultRoleEvent
arguments: ['@entity_type.manager']
og.event_subscriber:
class: Drupal\og\EventSubscriber\OgEventSubscriber
arguments: ['@og.permission_manager']
tags:
- { name: 'event_subscriber' }
og.group.manager:
class: Drupal\og\GroupManager
arguments: ['@config.factory', '@entity_type.manager', '@entity_type.bundle.info', '@event_dispatcher', '@state']
arguments: ['@config.factory', '@entity_type.manager', '@entity_type.bundle.info', '@event_dispatcher', '@og.default_role_event', '@state']
og.permissions:
class: Drupal\og\OgPermissionHandler
arguments: ['@module_handler', '@string_translation', '@controller_resolver']
Expand Down
63 changes: 47 additions & 16 deletions src/Entity/OgRole.php
Original file line number Diff line number Diff line change
Expand Up @@ -174,11 +174,42 @@ public function setRoleType($role_type) {
return $this->set('role_type', $role_type);
}

/**
* {@inheritdoc}
*/
public function getName() {
// If the name is not set yet, try to derive it from the ID.
if (empty($this->name) && !empty($this->id())) {
list(, , $name) = explode('-', $this->id());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This seems slightly limited. Maybe it would be better to explode, but pop off the last element and use that?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

There should always be three elements in the current implementation. But I think you're on to something, this is indeed a bug. I don't think there is anything stopping people to use dashes in their entity type IDs and bundle IDs, which means that counting the dashes is not going to cut it.

I'll change it to match exactly "{$this->getGroupEntityTypeId()}-{$this->getGroupBundleId()}-$role_name", then we'll eliminate any dash-related problems.

$this->setName($name);
}
return $this->name;
}

/**
* {@inheritdoc}
*/
public function setName($name) {
$this->name = $name;
return $this;
}

/**
* {@inheritdoc}
*/
public function save() {
if ($this->isNew()) {
// The ID of a new OgRole has to consist of the entity type ID, bundle ID

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Wouldn't it also be good to be able to have more generic roles? rather than always tied to a group? I am currently trying to use something similar. I have an admin type role, and this is available for on on all groups I create.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I have an admin type role, and this is available for on on all groups I create.

Not sure I understand, but to clarify the logic in OG7 that should be ported to OG8:

OgRoles could be either perBundle or perGroupId.

  • per bundle means the group's roles are "read-only" for the group admins, and are controlled by the site admin.
  • per group Id means the group's roles are being handled by the group admins. They can add/ change existing roles - apart of course of the non-member and member.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

ok, thanks for clarifying @amitaibu. I think I am just using per bundle in that case... :)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Basically, ignore me please ;)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes the per-group ID roles are not in yet, these are going to be added in #209. I'm not worrying about them here, that's the responsibility of that ticket.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

👍

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@damiankloip if you want to provide a role that is available for all groups, you can provide an event subscriber that simply ignores the passed in group entity type and bundle and returns the role every time, eg:

class MyEventSubscriber implements EventSubscriberInterface {

  public static function getSubscribedEvents() {
    return [
      DefaultRoleEventInterface::EVENT_NAME => [['provideDefaultRoles']],
    ];
  }

  public function provideDefaultRoles(DefaultRoleEventInterface $event) {
    // Just ignore $event->getEntityTypeId() and $event->getBundleId(),
    // this role applies to all groups.
    $event->addRole([
      'name' => 'moderator',
      'label' => 'Moderator',
      'is_admin' => TRUE,
    ]);
  }

}

// and role name, separated by dashes.
if ($this->isNew() && !empty($this->id())) {
list($entity_type_id, $bundle_id, $name) = explode('-', $this->id());
if ($entity_type_id !== $this->getGroupType() || $bundle_id !== $this->getGroupBundle() || $name !== $this->getName()) {
throw new ConfigValueException('The ID should consist of the group entity type ID, group bundle ID and role name, separated by dashes.');
}
}

// If a new OgRole is saved and the ID is not set, construct the ID from
// the entity type ID, bundle ID and role name.
if ($this->isNew() && empty($this->id())) {
if (empty($this->getGroupType())) {
throw new ConfigValueException('The group type can not be empty.');
}
Expand All @@ -187,6 +218,10 @@ public function save() {
throw new ConfigValueException('The group bundle can not be empty.');
}

if (empty($this->getName())) {
throw new ConfigValueException('The role name can not be empty.');

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

we seem to have a similar check in validate, however that one throws a different exception. Maybe consolidate it, and call validate() from here instead?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I had a look at this but it is not really possible I think. The DefaultRoleEvent::validate() method is used to validate only the data that is necessary for the event listener to gather the default roles. This one is in OgRole::save() and validates everything required to successfully store the entity.

These are different use cases, and calling validate() from here will make OgRole depend on DefaultRoleEvent.

}

// When assigning a role to group we need to add a prefix to the ID in
// order to prevent duplicate IDs.
$prefix = $this->getGroupType() . '-' . $this->getGroupBundle() . '-';
Expand All @@ -195,7 +230,7 @@ public function save() {
$prefix .= $this->getGroupId() . '-';
}

$this->id = $prefix . $this->id();
$this->id = $prefix . $this->getName();
}

parent::save();
Expand Down Expand Up @@ -234,30 +269,26 @@ public function delete() {
}

/**
* Returns default properties for the default OG roles.
*
* These are the two roles that are required by every group: the 'member' and
* 'non-member' roles.
*
* All other default roles are provided by DefaultRoleEvent.
*
* @return array
* An array of properties, keyed by OG role.
*
* @see \Drupal\og\Event\DefaultRoleEventInterface
* @see \Drupal\og\GroupManager::getDefaultRoles()
* {@inheritdoc}
*/
public static function getDefaultRoles() {
return [
public static function getDefaultRoleProperties($default_role_name) {
if (!in_array($default_role_name, [self::ANONYMOUS, self::AUTHENTICATED])) {
throw new \InvalidArgumentException('Invalid role name.');
}
$default_properties = [
self::ANONYMOUS => [
'role_type' => OgRoleInterface::ROLE_TYPE_REQUIRED,
'label' => 'Non-member',
'name' => self::ANONYMOUS,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

static::

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

just to know, so you always use static:: instead of self::?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The main reason this is adopted in D8 is late static binding.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

indeed, question is if self should be avoided entirely in services, and always use static - in case someone extends the class?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

or am I messing concepts here? :)

@damiankloip damiankloip Jun 15, 2016

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yes, basically. static will always refer to the current class, where as self could E.g. mean the parent class. As a lot of stuff is extended in Drupal, this is the consistent standard. I would say, use static everywhere instead of self. I think it has most impact on calling class methods really though..

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the info! Fixed it.

],
self::AUTHENTICATED => [
'role_type' => OgRoleInterface::ROLE_TYPE_REQUIRED,
'label' => 'Member',
'name' => self::AUTHENTICATED,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

static::

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed, thanks!

],
];

return $default_properties[$default_role_name];
}

/**
Expand Down
72 changes: 53 additions & 19 deletions src/Event/DefaultRoleEvent.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace Drupal\og\Event;

use Drupal\Core\Entity\EntityTypeManagerInterface;
use Drupal\og\OgRoleInterface;
use Symfony\Component\EventDispatcher\Event;

Expand All @@ -21,6 +22,23 @@ class DefaultRoleEvent extends Event implements DefaultRoleEventInterface {
*/
protected $roles = [];

/**
* The OG Role entity storage handler.
*
* @var \Drupal\Core\Entity\EntityStorageInterface
*/
protected $ogRoleStorage;

/**
* Constructs a DefaultRoleEvent object.
*
* @var \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
* The entity type manager.
*/
public function __construct(EntityTypeManagerInterface $entity_type_manager) {
$this->ogRoleStorage = $entity_type_manager->getStorage('og_role');
}

/**
* {@inheritdoc}
*/
Expand All @@ -41,43 +59,46 @@ public function getRoles() {
/**
* {@inheritdoc}
*/
public function addRole($name, array $properties) {
if (array_key_exists($name, $this->roles)) {
throw new \InvalidArgumentException("The '$name' role already exists.");
}
$this->validate($name, $properties);
public function addRole(array $properties) {
$this->validate($properties);

// Provide default value for the role type.
if (empty($properties['role_type'])) {
$properties['role_type'] = OgRoleInterface::ROLE_TYPE_STANDARD;
if (array_key_exists($properties['name'], $this->roles)) {

@amitaibu amitaibu Jun 15, 2016

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think this is still a bit weird that DefaultRoleEvent is dealing with validation of OgRole creation. I think that maybe we can completely remove this method, and possibly move validation to OgRole .

See comment I'll add in OgEventSubscriber::provideDefaultRoles ;)

throw new \InvalidArgumentException("The '{$properties['name']}' role already exists.");
}

$this->roles[$name] = $properties;
// Provide default values.
$properties += [
'role_type' => OgRoleInterface::ROLE_TYPE_STANDARD,
'is_admin' => FALSE,
];

$this->roles[$properties['name']] = $this->ogRoleStorage->create($properties);
}

/**
* {@inheritdoc}
*/
public function addRoles(array $roles) {
foreach ($roles as $role => $properties) {
$this->addRole($role, $properties);
$this->addRole($properties);
}
}

/**
* {@inheritdoc}
*/
public function setRole($name, array $properties) {
$this->deleteRole($name);
$this->addRole($name, $properties);
public function setRole(array $properties) {
$this->validate($properties);
$this->deleteRole($properties['name']);
$this->addRole($properties);
}

/**
* {@inheritdoc}
*/
public function setRoles(array $roles) {
foreach ($roles as $name => $properties) {
$this->setRole($name, $properties);
$this->setRole($properties);
}
}

Expand Down Expand Up @@ -106,7 +127,11 @@ public function offsetGet($key) {
* {@inheritdoc}
*/
public function offsetSet($key, $value) {
$this->setRole($key, $value);
$this->validate($value);
if ($value['name'] !== $key) {
throw new \InvalidArgumentException('The key and the "name" property of the role should be identical.');
}
$this->setRole($value);
}

/**
Expand All @@ -130,27 +155,36 @@ public function getIterator() {
return new \ArrayIterator($this->roles);
}

/**
* {@inheritdoc}
*/
public function reset() {
$this->roles = [];
}

/**
* Validates a role that is about to be set or added.
*
* @param string $name
* The name of the role to add or set.
* @param array $properties
* The role properties to validate.
*
* @throws \InvalidArgumentException
* Thrown when the role name is empty, the 'label' property is missing, or
* the 'role_type' property is invalid.
*/
protected function validate($name, $properties) {
if (empty($name)) {
protected function validate($properties) {
if (empty($properties['name'])) {
throw new \InvalidArgumentException('Role name is required.');
}

if (empty($properties['label'])) {
throw new \InvalidArgumentException('The label property is required.');
}

if (!empty($properties['is_admin']) && !is_bool($properties['is_admin'])) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Core roles don't usually do this. Relies on just casting it in isAdmin. Not sure if this is needed or not. Or it would cause more trouble with forms? Wouldn't the config schema help enforce this?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I added it because this data is sourced from event subscribers, I thought it would help people making mistakes in their subscribers. I can remove it if you want.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Removed it.

throw new \InvalidArgumentException('The is_admin property should be a boolean.');
}

$valid_role_types = [
OgRoleInterface::ROLE_TYPE_STANDARD,
OgRoleInterface::ROLE_TYPE_REQUIRED,
Expand Down
30 changes: 23 additions & 7 deletions src/Event/DefaultRoleEventInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ interface DefaultRoleEventInterface extends \ArrayAccess, \IteratorAggregate {
* @param $name
* The name of the role to return.
*
* @return \Drupal\og\Entity\OgRole
* The OgRole entity. Note that we cannot specify OgRoleInterface here
* because of limitations in interface inheritance in PHP 5.
*
* @throws \InvalidArgumentException
* Thrown when the role with the given name does not exist.
*/
Expand All @@ -37,20 +41,21 @@ public function getRoles();
/**
* Adds a default role.
*
* @param string $name
* The name of the role to add.
* @param array $properties
* An associative array of role properties, keyed by the following:
* - 'name': The machine name of the role.
* - 'label': The human readable label.
* - 'role_type': Either OgRoleInterface::ROLE_TYPE_STANDARD or
* OgRoleInterface::ROLE_TYPE_REQUIRED. Defaults to
* OgRoleInterface::ROLE_TYPE_STANDARD.
* - 'is_admin': Whether or not the role is an administration role. Defaults
* to FALSE.
*
* @throws \InvalidArgumentException
* Thrown when the role that is added already exists, when the role name is
* empty, or when the 'label' property is missing.
*/
public function addRole($name, array $properties);
public function addRole(array $properties);

/**
* Adds multiple default roles.
Expand All @@ -63,17 +68,21 @@ public function addRoles(array $roles);
/**
* Sets a default roles.
*
* @param string $name
* The name of the role to set.
* @param array $properties
* An associative array of role properties to set, keyed by the following:
* - 'name': The machine name of the role.
* - 'label': The human readable label.
* - 'role_type': Either OgRoleInterface::ROLE_TYPE_STANDARD or
* OgRoleInterface::ROLE_TYPE_REQUIRED. Defaults to
* OgRoleInterface::ROLE_TYPE_STANDARD.
* - 'is_admin': Whether or not the role is an administration role. Defaults
* to FALSE.
*
* @throws \InvalidArgumentException
* Thrown when the role name is empty, or when the 'label' property is
* missing.
*/
public function setRole($name, array $properties);
public function setRole(array $properties);

/**
* Sets multiple default roles.
Expand All @@ -95,11 +104,18 @@ public function deleteRole($name);
* Returns whether or not the given role exists.
*
* @param string $name
* The name of the role for which to verify the existance.
* The name of the role for which to verify the existence.
*
* @return bool
* TRUE if the role exists, FALSE otherwise.
*/
public function hasRole($name);

/**
* Resets the internal static cache.
*
* Call this before dispatching the event.
*/
public function reset();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

huh? This seems like a weird thing to have to do if you're dispatching an event?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think it's strange too, but this is a so called "ContainerAware" event, which is a service that is lazy loaded when first requested and then persists in the container (much like a singleton). This has the side effect that if a second round is done the previous results are still there. In normal use it would probably be rare (but not impossible) to fire the event listener twice, but this happens all the time in tests.

The ContainerAwareEventDispatcher ignores this, it doesn't seem have support to do something like an initialize() on dispatch. The documentation on it is also very sparse.

I was looking for an issue about it on the Symfony but there are only 4 open issues mentioning the container aware event dispatcher.

As an alternative to doing a manual reset, we can also cache the data locally per entity type and bundle.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I opted to keep the reset() for now. Caching would make it more complicated and gathering the permissions is not expected to be an expensive operation. We can still add caching later if needed.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The reset() is gone now 👍


}
6 changes: 5 additions & 1 deletion src/EventSubscriber/OgEventSubscriber.php
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,11 @@ public function provideDefaultOgPermissions(PermissionEventInterface $event) {
* The default role event.
*/
public function provideDefaultRoles(DefaultRoleEventInterface $event) {
$event->addRole(OgRoleInterface::ADMINISTRATOR, ['label' => 'Administrator']);
$event->addRole([

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

How about something like

public function provideDefaultRoles(DefaultRoleEventInterface $event) {
  $role = $this->ogRoleStorage->create(['name' => OgRoleInterface::ADMINISTRATOR);
  // Letting OgRole do it's job ...
  $role->setLabel('Administrator')
    ->setAdmin();

  $event->addRole($role);
}

Now our event's addRole doesn't need to know much about the role -- it's just passing it on. It's up to OgRole::save() to validate nothing is wrong.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I mean if I understand correctly (and correct me if I don't 😉) - I think your goal is to prevent mistakes in the default definition, but it kind of causes double validations - one in OgRole and one in the event.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

OK! I get your point. This seems like a good plan. I wanted to make it "easy" to implement a subscriber, but your idea makes sense. Is it acceptable for you to change this in a followup?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Unless it is going to have a negative effect on your moral, I think it's better to have this change here. I'd prefer not to introduce a complexity where we know there's a simpler solution.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

No not at all :) I'll fix it tomorrow

'name' => OgRoleInterface::ADMINISTRATOR,
'label' => 'Administrator',
'is_admin' => TRUE,
]);
}

}
Loading