Skip to content

Convert og_is_member to Og:isMember and add test coverage#53

Merged
amitaibu merged 5 commits into
amitaibu:8.x-1.xfrom
damiankloip:og-is-member
Dec 1, 2015
Merged

Convert og_is_member to Og:isMember and add test coverage#53
amitaibu merged 5 commits into
amitaibu:8.x-1.xfrom
damiankloip:og-is-member

Conversation

@damiankloip

Copy link
Copy Markdown
Collaborator

This converts the method and adds new test coverage for it. The EntityGroupsTest also covers Og::getEntityGroups as Og::isMember is basically that and doesn't currently have coverage.

Comment thread src/Og.php Outdated

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.

We need to create a map of the group ids as getEntityGroups currently returns a map of membership_id > group entity for each 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.

Worth adding it as a comment.

@damiankloip

Copy link
Copy Markdown
Collaborator Author

This is blocker number 1 for #40

@damiankloip damiankloip changed the title Convert og_is_memeber to Og:isMember and add test coverage Convert og_is_member to Og:isMember and add test coverage Nov 30, 2015
@damiankloip

Copy link
Copy Markdown
Collaborator Author

@amitaibu @RoySegall this is looking pretty good.

Comment thread src/Og.php Outdated

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 maybe it's a good time to replace the arguments with the entity objects, so it will be isMember(EntityInterface $group_entity, EntityInterface $entity) instead. Thoughts?

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.

Yeah I would Def prefer this but just kept it the same. I am happy to
change it here though.
On 30 Nov 2015 18:11, "Amitai Burstein" notifications@github.com wrote:

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

  • * The entity type of the group.
  • * @param string|int $group_id
  • * The group ID.
  • * @param string $entity_type
  • * The entity type.
  • * @param string|int $entity_id
  • * The entity ID.
  • * @param array $states
  • * (optional) Array with the state to return. If empty groups of all state
  • * 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($group_entity_type, $group_id, $entity_type, $entity_id = NULL, $states = [OG_STATE_ACTIVE]) {

I think maybe it's a good time to replace the arguments with the entity
objects, so it will be isMember(EntityInterface $group_entity,
EntityInterface $entity) instead. Thoughts?


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

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.

Go for it. Back in D7 I took the approach that if we must get the entity type, then at least we won't force getting the entity object, and maybe save some entity loading. I prefer the D8 way :)

@damiankloip

Copy link
Copy Markdown
Collaborator Author

@amitaibu Should all be addressed now. Having the entities as parameters instead is soooo much nicer :)

Comment thread src/Og.php Outdated

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 :)

amitaibu added a commit that referenced this pull request Dec 1, 2015
Convert og_is_member to Og:isMember and add test coverage
@amitaibu amitaibu merged commit e220a77 into amitaibu:8.x-1.x Dec 1, 2015
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.

3 participants