Rename method to Og::getUserMembershipsAndGroups

[amitaibu]

Name is a bit long, but makes it clear what you are getting. Also the type hinting will prevent sending non-user entities.

PR also adds cache to \Drupal\og\Og::getGroupsIds

[amitaibu]

What's left here is to mock in \Drupal\og\Og::getGroups the $entities = \Drupal::entityTypeManager()->getStorage($entity_type)->loadMultiple($entity_ids); part.

[amitaibu]

tests are green. ready for review.

[pfrenssen]

@amitaibu I am assigned to work on OG related functionality next week. We will be integrating roles and permissions in our project. One of the things that would be very helpful for implementing this is to have getUserMemberships() available as we discussed above. The memberships are the entry point for dealing with roles and permissions.

If you are busy, would it be OK for you if I continue work on this, starting Monday?

[amitaibu]

I'm out on vacation with the kids, so sure go ahead :) On Apr 29, 2016 5:41 PM, "Pieter Frenssen" notifications@github.com wrote:

@amitaibu https://github.com/amitaibu I am assigned to work on OG related functionality next week. We will be integrating roles and permissions in our project. One of the things that would be very helpful for implementing this is to have getUserMemberships() available as we discussed above. The memberships are the entry point for dealing with roles and permissions.

If you are busy, would it be OK for you if I continue work on this, starting Monday?

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/amitaibu/og/pull/184#issuecomment-215739854

[pfrenssen]

@amitaibu wow ok so you are being kept very busy :) Enjoy the holiday!

Going to start working on this now. I will start by splitting it into two methods. Second I will take a look at the static caches. I will look up how PHP deals with static caching of objects, hopefully it only caches a pointer to the object and not the full object. Then the memory consumption will be negligible.

[chx]

Let's talk on IRC :)

[pfrenssen]

@chx right! I'm there now :)

[pfrenssen]

Small summary of what I've done:

• There are now three methods to replace getEntityGroups():
  • getUserMemberships() returns the OgMembership entities for a given user. This is doing a query and the result is cached. I was doubting the name of this method, feedback is welcome.
  • getUserGroupIds() returns the group IDs of the given user, this aligns well with getGroupIds() and was already useful in Og::isMember() to save processing the groups in a closure. It leverages getUserMemberships() and does very little work on its own so it was not needed to cache this.
  • getUserGroups() returns full group entities. Again a very simple method that simply gives back results that are already cached by the entity storage. This is a little bit different from its group content counterpart getGroups() in that the array that is returned is keyed by entity type. This is done to give back the same results as the original getEntityGroups() that this is replacing. In reality there is no need to split up the results by entity type - the entity type can be derived from the objects themselves. I would like this and getGroups() to be consistent though, this one is not keyed by entity type. Which approach would be preferable?
• I added a unique prefix to the cache identifiers. There is a very slim but possible chance for a collision. Better to make it bullet proof now.

I did not add any specific tests since the full functionality is already indirectly tested through other tests. This is something that I would still like to add for completeness.

[amitaibu]

In reality there is no need to split up the results by entity type

I think splitting by entity-type still makes sense, even-though the info can be derived, to which entity-type it belongs is still often a crucial piece of info. as long we are consistent, we can always revise it later on.

[pfrenssen]

OK, shall I rework getGroups() to also return results keyed by entity type then? This would be the most consistent solution.

I'll pick this back up tomorrow. I'll address the remarks and provide test coverage.

[pfrenssen]

OgMembership doesn't implement OgMembershipInterface?

[pfrenssen]

@amitaibu already came to the same conclusion that OgMembership should implement OgMembershipInterface :)

https://github.com/amitaibu/og/issues/183

[pfrenssen]

Sorry for the big mess in the last couple of commits. I was in a hurry to finish this. Never good to rush stuff :-/

I only added very rudimentary testing for the field_name property, since that currently doesn't work well and seems to be not used for its intended purpose. I have read the reasoning for this in the documentation of OgMembership but I think that duplicating data is not a good solution for "easily displaying the group and group content in the UI". That's another discussion though and not related to this PR.

This is ready for review again.

[amitaibu]

Needs a re-roll

[pfrenssen]

Ok! Will be for tomorrow :)

[pfrenssen]

Reroll complete, this is ready for another look.

[pfrenssen]

Thanks for the review! Going to address the remarks!

[pfrenssen]

Addressed remarks.

[amitaibu]

Awesome stuff! thanks