Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
24 changes: 0 additions & 24 deletions og.module
Original file line number Diff line number Diff line change
Expand Up @@ -1912,30 +1912,6 @@ function _og_get_all_group_content_bundle() {
return $return;
}

/**
* Return TRUE if entity belongs to a group.
*
* @param $group_type
* The entity type of the group.
* @param $gid
* The group ID.
* @param $entity_type
* The entity type.
* @param $entity
* The entity object. If empty the current user will be used.
* @param $states
* (optional) Array with the state to return. If empty groups of all state will
* return.
*
* @return
* TRUE if the entity (e.g. the user) belongs to a group and is not pending or
* blocked.
*/
function _og_is_member($group_type, $gid, $entity_type = 'user', $entity = NULL, $states = array(OG_STATE_ACTIVE)) {
$groups = og_get_entity_groups($entity_type, $entity, $states);
return !empty($groups[$group_type]) && in_array($gid, $groups[$group_type]);
}

/**
* Check if group should use default roles and permissions.
*
Expand Down
37 changes: 32 additions & 5 deletions src/Og.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

use Drupal\Component\Render\FormattableMarkup;
use Drupal\Component\Utility\NestedArray;
use Drupal\Core\Entity\EntityInterface;
use Drupal\Core\Field\FieldConfigInterface;
use Drupal\Core\Field\FieldDefinitionInterface;
use Drupal\field\Entity\FieldConfig;
Expand Down Expand Up @@ -103,10 +104,8 @@ public static function createField($plugin_id, $entity_type, $bundle, array $set
/**
* Gets the groups an entity is associated with.
*
* @param $entity_type
* The entity type.
* @param $entity_id
* The entity ID.
* @param \Drupal\Core\Entity\EntityInterface $entity
* The entity to get groups for.
* @param $states
* (optional) Array with the state to return. Defaults to active.
* @param $field_name
Expand All @@ -117,7 +116,10 @@ public static function createField($plugin_id, $entity_type, $bundle, array $set
* the OG membership ID and the group ID as the value. If nothing found,
* then an empty array.
*/
public static function getEntityGroups($entity_type, $entity_id, $states = [OG_STATE_ACTIVE], $field_name = NULL) {
public static function getEntityGroups(EntityInterface $entity, $states = [OG_STATE_ACTIVE], $field_name = NULL) {
$entity_type = $entity->getEntityTypeId();

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.

btw, sometimes we call it $entity_type, and sometimes $entity_type_id. Lets go with $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 don't mind too much, trouble with that is now that D8 has $entity_type, which can be an EntityType object, just the ID.

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 don't mind too much

Me neither, just want to be consistent. I could go with $entity_type_id as-well.

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 think I've been using the _id suffixed versions. So go with that??
On 1 Dec 2015 10:50, "Amitai Burstein" notifications@github.com wrote:

In src/Og.php
#53 (comment):

@@ -117,7 +116,10 @@ public static function createField($plugin_id, $entity_type, $bundle, array $set
* the OG membership ID and the group ID as the value. If nothing found,
* then an empty array.
*/

  • public static function getEntityGroups($entity_type, $entity_id, $states = [OG_STATE_ACTIVE], $field_name = NULL) {
  • public static function getEntityGroups(EntityInterface $entity, $states = [OG_STATE_ACTIVE], $field_name = NULL) {
  • $entity_type = $entity->getEntityTypeId();

I don't mind too much

Me neither, just want to be consistent. I could go with $entity_type_id
as-well.


Reply to this email directly or view it on GitHub
https://github.com/amitaibu/og/pull/53/files#r46264240.

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.

ok, updated :)

$entity_id = $entity->id();

// Get a string identifier of the states, so we can retrieve it from cache.
if ($states) {
sort($states);
Expand Down Expand Up @@ -168,6 +170,31 @@ public static function getEntityGroups($entity_type, $entity_id, $states = [OG_S
return static::$entityGroupCache[$identifier];
}

/**
* Return TRUE if entity belongs to a group.
*
* @param \Drupal\Core\Entity\EntityInterface $group
* The group entity to check.
* @param \Drupal\Core\Entity\EntityInterface $entity
* The entity to get groups for.
* @param array $states
* (optional) Array with the state to return. If empty groups of all state

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.

(optional) Array with the membership states to check the membership. Defaults to active memberships.

* will return.
*
* @return bool
* TRUE if the entity (e.g. the user) belongs to a group and is not pending
* or blocked.
*/
public static function isMember(EntityInterface $group, EntityInterface $entity, $states = [OG_STATE_ACTIVE]) {

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.

Do we really need static methods?

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.

hi @dawehner welcome to the party! :)

Do we really need static methods?

What would be the alternative here? Or in other words, why not 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.

I think we were going to review this after. But just moving stuff into
services and using those services instead.
On 7 Dec 2015 18:02, "Amitai Burstein" notifications@github.com wrote:

In src/Og.php
#53 (comment):

@@ -169,6 +171,31 @@ public static function getEntityGroups($entity_type, $entity_id, $states = [OG_S
}

/**

  • * Return TRUE if entity belongs to a group.
  • * @param \Drupal\Core\Entity\EntityInterface $group
  • * The group entity to check.
  • * @param \Drupal\Core\Entity\EntityInterface $entity
  • * The entity to get groups for.
  • * @param array $states
  • * (optional) Array with the membership states to check the membership.
  • * Defaults to active memberships.
  • * @return bool
  • * TRUE if the entity (e.g. the user) belongs to a group and is not pending
  • * or blocked.
  • */
  • public static function isMember(EntityInterface $group, EntityInterface $entity, $states = [OG_STATE_ACTIVE]) {

hi @dawehner https://github.com/dawehner welcome to the party! :)

Do we really need static methods?

What would be the alternative here? Or in other words, why not static?


Reply to this email directly or view it on GitHub
https://github.com/amitaibu/og/pull/53/files#r46853442.

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.

For now we do, yes :) we don't have anywhere else to put them at the
moment. See my other comment about that. We need to see what we've got and
then how it makes sense to split that IMO.
On 7 Dec 2015 17:54, "Daniel Wehner" notifications@github.com wrote:

In src/Og.php
#53 (comment):

@@ -169,6 +171,31 @@ public static function getEntityGroups($entity_type, $entity_id, $states = [OG_S
}

/**

  • * Return TRUE if entity belongs to a group.
  • * @param \Drupal\Core\Entity\EntityInterface $group
  • * The group entity to check.
  • * @param \Drupal\Core\Entity\EntityInterface $entity
  • * The entity to get groups for.
  • * @param array $states
  • * (optional) Array with the membership states to check the membership.
  • * Defaults to active memberships.
  • * @return bool
  • * TRUE if the entity (e.g. the user) belongs to a group and is not pending
  • * or blocked.
  • */
  • public static function isMember(EntityInterface $group, EntityInterface $entity, $states = [OG_STATE_ACTIVE]) {

Do we really need static methods?


Reply to this email directly or view it on GitHub
https://github.com/amitaibu/og/pull/53/files#r46852592.

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.

Does this mean your getting involved then @da_wehner?

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.

It's the trivia night gang all over again! :)

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.

Mh, if that damian is involved I could be motivated

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.

Fair point regarding static methods, but yeah having kernel tests helps a lot of with refactoring those things

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.

Exactly! We need a proper discussion about our services I think. Now we
have a lot of the building blocks there I think soon could be a good time.
On 7 Dec 2015 18:30, "Daniel Wehner" notifications@github.com wrote:

In src/Og.php
#53 (comment):

@@ -169,6 +171,31 @@ public static function getEntityGroups($entity_type, $entity_id, $states = [OG_S
}

/**

  • * Return TRUE if entity belongs to a group.
  • * @param \Drupal\Core\Entity\EntityInterface $group
  • * The group entity to check.
  • * @param \Drupal\Core\Entity\EntityInterface $entity
  • * The entity to get groups for.
  • * @param array $states
  • * (optional) Array with the membership states to check the membership.
  • * Defaults to active memberships.
  • * @return bool
  • * TRUE if the entity (e.g. the user) belongs to a group and is not pending
  • * or blocked.
  • */
  • public static function isMember(EntityInterface $group, EntityInterface $entity, $states = [OG_STATE_ACTIVE]) {

Fair point regarding static methods, but yeah having kernel tests helps a
lot of with refactoring those things


Reply to this email directly or view it on GitHub
https://github.com/amitaibu/og/pull/53/files#r46856897.

$groups = static::getEntityGroups($entity, $states);
$group_entity_type = $group->getEntityTypeId();
// We need to create a map of the group ids as getEntityGroups returns a map

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.

getEntityGroups => Og::getEntityGroups

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, but we are already inside the Og class.

// of membership_id => group entity for each type.
return !empty($groups[$group_entity_type]) && in_array($group->id(), array_map(function($group_entity) {
return $group_entity->id();
}, $groups[$group_entity_type]));
}

/**
* Check if the given entity type and bundle is a group.
*
Expand Down
2 changes: 1 addition & 1 deletion src/Plugin/EntityReferenceSelection/OgSelection.php
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ public function getConfiguration($key = NULL) {
* @return ContentEntityInterface[]
*/
protected function getUserGroups() {
$other_groups = Og::getEntityGroups('user', $this->currentUser->id());
$other_groups = Og::getEntityGroups($this->currentUser->getAccount());
return isset($other_groups[$this->configuration['target_type']]) ? $other_groups[$this->configuration['target_type']] : [];
}

Expand Down
4 changes: 2 additions & 2 deletions src/Plugin/Field/FieldWidget/OgComplex.php
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ protected function formMultipleElements(FieldItemListInterface $items, array &$f
$parents = $form['#parents'];

$target_type = $this->fieldDefinition->getTargetEntityTypeId();
$user_groups = Og::getEntityGroups('user', \Drupal::currentUser()->id());
$user_groups = Og::getEntityGroups(\Drupal::currentUser()->getAccount());
$user_groups_target_type = isset($user_groups[$target_type]) ? $user_groups[$target_type] : [];
$user_group_ids = array_map(function($group) {
return $group->id();
Expand Down Expand Up @@ -228,7 +228,7 @@ protected function otherGroupsWidget(FieldItemListInterface $items, FormStateInt

$target_type = $this->fieldDefinition->getTargetEntityTypeId();

$user_groups = Og::getEntityGroups('user', \Drupal::currentUser()->id());
$user_groups = Og::getEntityGroups(\Drupal::currentUser()->getAccount());
$user_groups_target_type = isset($user_groups[$target_type]) ? $user_groups[$target_type] : [];
$user_group_ids = array_map(function($group) {
return $group->id();
Expand Down
216 changes: 216 additions & 0 deletions tests/src/Kernel/Entity/EntityGroupsTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,216 @@
<?php

/**
* @file
* Contains \Drupal\Tests\og\Kernel\Entity\EntityGroupsTest.
*/

namespace Drupal\Tests\og\Kernel\Entity;

use Drupal\entity_test\Entity\EntityTest;
use Drupal\KernelTests\KernelTestBase;
use Drupal\Component\Utility\Unicode;
use Drupal\og\Entity\OgMembership;
use Drupal\og\Og;
use Drupal\user\Entity\User;

/**
* Tests entity group methods

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.

=> Tests getting the memberships of an entity.

*
* @group og
*/
class EntityGroupsTest extends KernelTestBase {

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.

This sounds a bit too general. Maybe GetEntityGroupsTest

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.

Hmm, this does not just test that though...

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.

the way I see it isMember is just sugar on top of getEntityGroups. If you don't agree, it's minor we can keep as-is.


/**
* {@inheritdoc}
*/
public static $modules = ['system', 'user', 'field', 'og', 'entity_test'];

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 user and field modules will be enabled anyway, by OG - do you think we should still explicitly call them?

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 don't think that will happen in kernel tests, I think you need to be explicit about the modules and schemas you want available.


/**
* @var \Drupal\user\Entity\User
*/
protected $user1;

/**
* @var \Drupal\user\Entity\User
*/
protected $user2;

/**
* @var \Drupal\user\Entity\User
*/
protected $user3;

/**
* @var \Drupal\entity_test\Entity\EntityTest
*/
protected $group1;

/**
* @var \Drupal\entity_test\Entity\EntityTest
*/
protected $group2;

/**
* The machine name of the group node type.

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.

shouldn't this be above the @var?

*
* @var string
*/
protected $groupBundle;

/**
* The machine name of the group content node type.
*
* @var string
*/
protected $groupContentBundle;

/**
* {@inheritdoc}
*/
protected function setUp() {
parent::setUp();

$this->installConfig(['og']);
$this->installEntitySchema('og_membership');
$this->installEntitySchema('user');
$this->installEntitySchema('entity_test');
$this->installSchema('system', 'sequences');

$this->groupBundle = Unicode::strtolower($this->randomMachineName());
$this->groupContentBundle = Unicode::strtolower($this->randomMachineName());

// Create users.
$this->user1 = User::create(['name' => $this->randomString()]);
$this->user1->save();

$this->user2 = User::create(['name' => $this->randomString()]);
$this->user2->save();

$this->user3 = User::create(['name' => $this->randomString()]);
$this->user3->save();

// Define the group content as group.
Og::groupManager()->addGroup('entity_test', $this->groupBundle);

// Create a group and associate with user 1.
$this->group1 = EntityTest::create([
'type' => $this->groupBundle,
'name' => $this->randomString(),
'user_id' => $this->user1->id(),
]);
$this->group1->save();

// Create a group and associate with user 2.
$this->group2 = EntityTest::create([
'type' => $this->groupBundle,
'name' => $this->randomString(),
'user_id' => $this->user2->id(),
]);
$this->group2->save();
}

/**
* Tests group owners have the correct groups.
*/
public function testOwnerGroupsOnly() {
$actual = Og::getEntityGroups($this->user1);

$this->assertCount(1, $actual['entity_test']);
$this->assertGroupExistsInResults($this->group1, $actual);

// Also check isMember.
$this->assertTrue(Og::isMember($this->group1, $this->user1));
$this->assertFalse(Og::isMember($this->group1, $this->user2));

$actual = Og::getEntityGroups($this->user2);

$this->assertCount(1, $actual['entity_test']);
$this->assertGroupExistsInResults($this->group2, $actual);

// Also check isMember.
$this->assertTrue(Og::isMember($this->group2, $this->user2));
$this->assertFalse(Og::isMember($this->group2, $this->user1));
}

/**
* Tests other groups users are added to.
*/
public function testOtherGroups() {
// Should be a part of no groups.
$this->assertEquals([], Og::getEntityGroups($this->user3));
$this->assertFalse(Og::isMember($this->group1, $this->user3));
$this->assertFalse(Og::isMember($this->group2, $this->user3));

// Invalidate the caches so the static cache is cleared and group data is
// fetched again for the user.
Og::invalidateCache();

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.

Add comment why.


// Add user to group 1 should now return that group only.
$this->createMembership($this->user3, $this->group1);

$actual = Og::getEntityGroups($this->user3);

$this->assertCount(1, $actual['entity_test']);
$this->assertGroupExistsInResults($this->group1, $actual);

$this->assertTrue(Og::isMember($this->group1, $this->user3));
$this->assertFalse(Og::isMember($this->group2, $this->user3));

Og::invalidateCache();

// Add to group 2 should also return that.
$this->createMembership($this->user3, $this->group2);

$actual = Og::getEntityGroups($this->user3);

$this->assertCount(2, $actual['entity_test']);
$this->assertGroupExistsInResults($this->group1, $actual);
$this->assertGroupExistsInResults($this->group2, $actual);

$this->assertTrue(Og::isMember($this->group1, $this->user3));
$this->assertTrue(Og::isMember($this->group2, $this->user3));
}

/**
* Creates an Og membership entity.
*
* @todo This is a temp function, which will be later replaced by Og::group().
*
* @param \Drupal\user\Entity\User $user
* @param \Drupal\Core\Entity\EntityInterface $group
*
* @return \Drupal\og\Entity|OgMembership
*/
protected function createMembership($user, $group) {

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 as a note: This is a temp function, which will be later replaced by Og::group() (the port of og_group). In D7, calling og_membership_create() directly was discouraged, as it didn't do all the necessary access check, and association with the right group audience field.

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.

See above docblock, I added words to that effect as a todo.

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.

lets add a @todo here about replacing this?

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.

oh, sorry, missed it :)

$membership = OgMembership::create(['type' => OG_MEMBERSHIP_TYPE_DEFAULT])
->setEntityId($user->id())
->setEntityType('user')
->setGid($group->id())
->setGroupType($group->getEntityTypeId())
->save();

return $membership;
}

/**
* Asserts whether a group ID exists in some results.
*
* Assumes entity_type is used.
*
* @param \Drupal\Core\Entity\EntityInterface $group_to_check
* @param array $results
*/
protected function assertGroupExistsInResults($group_to_check, array $results) {

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.

$group_to_check => $group

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.

$group_to_check => (EntityInterface $group_entity

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.

oh, never mind, I see you already use $group.

btw, I wonder if we should go with $group_entity, or just $group?

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 I prefer just $group, plus we have been using that everywhere else up to now I think.

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.

Oh, do you mean in the closure? If we have a $group param, we can use $group_entity - sure!

$found = FALSE;
foreach ($results['entity_test'] as $group) {
if ($group->id() == $group_to_check->id()) {
$found = TRUE;
break;
}
}

$this->assertTrue($found);
}

}