-
Notifications
You must be signed in to change notification settings - Fork 133
Avoid exception in case a membership is already found. #296
Avoid exception in case a membership is already found. #296
Conversation
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.
Thanks. I'd love to get a test as-well to confirm the functionality.
og.module
Outdated
$membership = Og::createMembership($entity, $entity->getOwner()); | ||
$membership->save(); | ||
// Other modules might interfere and create a membership. | ||
if (!Og::getMembership($entity, $entity->getOwner())) { |
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 think here you will get only the active
memberships.
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.
Indeed!! I have also been trapped by this in the past, which is why I created #292 to lobby for changing this to return all memberships by default. I think it is not intuitive that this method returns filtered results by default, and an additional parameter should be passed to get the actual membership.
og.module
Outdated
@@ -47,8 +47,11 @@ function og_entity_insert(EntityInterface $entity) { | |||
return; | |||
} | |||
|
|||
$membership = Og::createMembership($entity, $entity->getOwner()); | |||
$membership->save(); | |||
// Other modules might interfere and create a membership. |
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.
maybe // Implementing modules might have created a membership ahead of us.
I think it's OK for me to merge this since I did not write the original patch but just the test coverage that proves the bug is real. |
From issue #295