getGroupsForEntityType() and getAllGroupBundles() do the same thing

[pfrenssen]

GroupManager::getGroupsForEntityType() and GroupManager::getAllGroupBundles() do basically the same thing. We should keep one and discard the other.

The implementation of getAllGroupBundles() seems less than ideal, it will return all results if a key for a certain entity type is not set. This is intended as a shortcut to check if the passed in entity type ID is empty, but this will unexpectedly return a full result set if an entity type ID is passed which is not a group type.

My personal preference goes to keeping getAllGroupBundles() because the method name sounds clearer to me. It will need to be reworked so it will return an empty array when an invalid entity type ID is passed, maybe something like this:

  public function getAllGroupBundles($entity_type_id = NULL) {
    if (empty($entity_type_id)) {
      return $this->groupMap;
    }
    return !empty($this->groupMap[$entity_type_id]) ? $this->groupMap[$entity_type_id] : [];
  }

The documentation is also incomplete. The parameter needs to be documented. It's also better to use $entity_type_id instead of $entity_type.

[idimopoulos]

Hi. I just wanted to add my opinion. The thing is that for me that am not that familiar with the code and am trying to get into it a bit, the naming of the methods are a bit confusing. I mean, e.g. the name getAllGroupBundles, for someone that does not know, means what? group bundles the group references to? bundles that reference to this group? bundles that are this group(?) ? And the other one is getGroupsForEntityType. It really can have to many meanings. Why not use something like getReferencingBundles or getReferencingBundlesFor[Group|EntityType] in order to return all the bundles that refer to the given entity type and then have the corresponding getReferencedGroups or getReferencedGroupsFor[Group|EntityType] to fetch the content that this entity type points to. My point is that maybe the 'reference' word is not perfect but since the words 'entityType' and 'group' are like common words in the module, using them in the names back and forth can be really confusing. If we can use a word like 'reference' to distinguish the direction of the connection it would really be helpful. With best regards

[pfrenssen]

I had the same confusion with getGroupsForEntityType(): my initial impression was that I could give it a group content entity type and get all groups that are referenced by it.

getAllGroupBundles() is clear in my own mind: it simply returns all the existing group bundles. So for example if you have two groups of type node, then it will return an array like this:

[
  'node' => [
    'group1',
    'group2',
  ],
]

But it would be even clearer if we call it getAllGroupBundleIds(), because it returns IDs, not full EntityType objects.