Acting upon group type creation #187
Conversation
|
I need help from the bug guns @damiankloip @pfrenssen @chx - How can I inject a service into the group manager service? After I'm creating a new group I want to invoke a module(or should this need to be an event dispatching?) but I can't user the What i'm missing here? |
|
That is very odd. Did you do a |
|
@RoySegall what's left here? |
|
I want to have a second look on this one and extend testing. |
|
👍 |
| @@ -44,6 +45,62 @@ function og_entity_insert(EntityInterface $entity) { | |||
| } | |||
|
|
|||
| /** | |||
| * Implements hook_og_group_created(). | |||
| */ | |||
| function og_og_group_created($entity_type, $bundle) { | |||
There was a problem hiding this comment.
Can you add some documentation on why this hook is implemented and what is being done here? That will be helpful for people that are not familiar with this PR.
There was a problem hiding this comment.
Why do we implement our own hook? Wouldn't it be better to handle this in GroupManager::addGroup()? This just adds unnecessary overhead of passing the data to a new function.
There was a problem hiding this comment.
seem to me much more easy to pass this logic into a hook rather keep the code in the group manager. I'll try something.
There was a problem hiding this comment.
But I still keeping the hook invocation.
| * | ||
| * @see og_og_group_created() | ||
| */ | ||
| function hook_og_group_created($entity_type, $bundle) { |
There was a problem hiding this comment.
@pfrenssen ^^
What do yo think?
|
@amitaibu I'm waiting for a couple of comments from our comrades but this is ready for review. |
RoySegall
changed the title
| @@ -459,6 +470,12 @@ protected function refreshGroupRelationMap() { | |||
|
|
|||
| foreach ($this->entityTypeBundleInfo->getAllBundleInfo() as $group_content_entity_type_id => $bundles) { | |||
| foreach ($bundles as $group_content_bundle_id => $bundle_info) { | |||
|
|
|||
| if ($group_content_bundle_id == 'user') { | |||
There was a problem hiding this comment.
we've seen we might have multiple user bundles, so probably this needs a better check.
There was a problem hiding this comment.
This seems like a good idea, as in Og8 we indeed no longer treat a user as group content. A user is a special case.
/cc @pfrenssen
|
I've added a few comments, but in general looks good. Thanks. |
|
A re roll is needed and improving the test. |
|
@amitaibu I added another bundle for the user for testing. Ready for review. |
|
@amitaibu I fixed another conflicts issues. Ready for review and merge. |
| * The created group. | ||
| */ | ||
| public function createUserGroupAudienceField(GroupCreationEventInterface $event) | ||
| { |
| * Mocked method calls when system under test should attach OG audience field | ||
| * on the user. | ||
| */ | ||
| protected function expectGroupCreation() { |
There was a problem hiding this comment.
I don't think this needs its own method. You can move the code back to above method.
|
Minor comment, after that I'll merge. |
|
@amitaibu I changed the location. |
|
Thanks. |
#174