Skip to content
This repository has been archived by the owner on Aug 18, 2024. It is now read-only.

Remove getAllGroupBundles and replace with getGroupMap #402

Merged
merged 10 commits into from
Aug 10, 2020
Merged

Remove getAllGroupBundles and replace with getGroupMap #402

merged 10 commits into from
Aug 10, 2020

Conversation

mariano-dagostino
Copy link
Contributor

#372

The ::getAllGroupBundles() method can be removed. It is incompletely documented and passing an argument to it completely changes the nature of the data that is returned. Moreover, if this method is called with no arguments, then it is an exact duplicate of ::getGroupMap(), and if it is called with an argument it duplicates ::getGroupsForEntityType(). So we can safely remove it after changing the calling code to the two other methods.

pfrenssen
pfrenssen suggested changes Jul 4, 2018
View reviewed changes
Copy link
Contributor

@pfrenssen pfrenssen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, just one small remark. Thanks!

src/Plugin/EntityReferenceSelection/OgSelection.php Outdated
@@ -68,13 +68,13 @@ protected function buildEntityQuery($match = NULL, $match_operator = 'CONTAINS')
$definition = \Drupal::entityTypeManager()->getDefinition($target_type);

if ($bundle_key = $definition->getKey('bundle')) {
$bundles = Og::groupTypeManager()->getAllGroupBundles($target_type);
$map = Og::groupTypeManager()->getGroupMap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can replace this with a call to GroupTypeManager::getGroupsForEntityType(), this can be passed the $target_type parameter directly, so we don't need to retrieve the full map and can save some memory.

@pfrenssen
Copy link
Contributor

Oh, something went wrong in the merge here, the getAllGroupBundles() method has reappeared in the GroupTypeManager, and in the GroupTypeManagerInterface. Can you remove them again?

@pfrenssen
Copy link
Contributor

Now we have some failures due to ::getGroupsForEntityType() being renamed to ::getGroupBundleIdsByEntityType(). This happened in #404 which was merged recently.

We'll get there eventually 😬

@MPParsley MPParsley changed the title Replace getAllGroupBundles with getGroupMap Remove getAllGroupBundles and replace with getGroupMap Aug 9, 2020
@MPParsley MPParsley self-assigned this Aug 9, 2020
@MPParsley MPParsley added this to the 8.x-1.0-alpha6 milestone Aug 9, 2020
@MPParsley MPParsley requested a review from pfrenssen August 9, 2020 17:22
@pfrenssen pfrenssen merged commit 54c30ab into Gizra:8.x-1.x Aug 10, 2020
@MPParsley MPParsley mentioned this pull request Aug 10, 2020
Merged
This was referenced Aug 12, 2020
Merged
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MPParsley MPParsley MPParsley left review comments

@pfrenssen pfrenssen pfrenssen approved these changes

Assignees

@MPParsley MPParsley

Labels
Clean-up Drupal 8 Ready for review
Projects
None yet
Milestone
8.x-1.0-alpha6
Development

Successfully merging this pull request may close these issues.
3 participants
@mariano-dagostino @pfrenssen @MPParsley