Throw an exception when calling `MembershipManager::getGroupIds()` with an entity which is not group content #311

pfrenssen · 2017-09-18T10:48:56Z

While working on a test for #296 I noticed that og_invalidate_group_content_cache_tags() calls MembershipManager::getGroupIds() even when the entity is a group and not group content. This is useless, ::getGroupIds() only works on group content and is clearly documented as such.

This results in a useless method being called and its useless results cached every time one of the entity hooks is firing on a group entity. Let's fix the bug and also throw an exception when the wrong entity is being passed, so that developers are alerted if they are using the method incorrectly.

pfrenssen · 2017-09-19T09:02:53Z

The schema errors are possibly due to https://www.drupal.org/node/2870971.

pfrenssen · 2017-09-19T09:11:03Z

og.module

  // 'og-group-content:{group entity type}:{group entity id}'.
-  if (Og::isGroup($entity->getEntityTypeId(), $entity->bundle()) || Og::isGroupContent($entity->getEntityTypeId(), $entity->bundle())) {
+  if (Og::isGroupContent($entity->getEntityTypeId(), $entity->bundle())) {


This check to see whether the entity is a group is useless, in MembershipManager::getGroupIds() the queries are performed on group audience fields. If there are no group audience fields it won't return any results.

pfrenssen · 2017-09-19T09:14:59Z

src/OgAccess.php

-    // The entity might be a user or a non-user entity.
-    $groups = $entity->getEntityTypeId() == 'user' ? $this->membershipManager->getUserGroups($entity) : $this->membershipManager->getGroups($entity);
-
-    if ($is_group_content && $groups) {


We already know at this point whether the entity is group content, so a small reshuffle of these two lines into the if statement is sufficient to not do an unneeded call to MembershipManager::getGroups().

src/OgDeleteOrphansBase.php

pfrenssen · 2017-09-19T09:17:21Z

config/schema/og.schema.yml

+
+entity_reference_selection.og:default:
+  type: entity_reference_selection.default
+  label: 'OG entity selection handler settings'


Schema change required due to https://www.drupal.org/node/2870971 - I can split this off to a separate ticket if required.

Let's try leaving this out, this hasn't proven to be problematic in the last 10 months so it was maybe a temporary hiccup that is no longer needed.

pfrenssen · 2018-07-04T16:01:45Z

og.module

@@ -317,8 +317,7 @@ function og_entity_type_alter(array &$entity_types) {
 function og_invalidate_group_content_cache_tags(EntityInterface $entity) {
  // If group content is created or updated, invalidate the group content cache
  // tags for each of the groups this group content belongs to. This allows
-  // group listings to be cached effectively. Also invalidate the tags if the
-  // group itself changes. The cache tag format is
+  // group listings to be cached effectively. The cache tag format is


This change should be undone, the code is now effectively checking if the group itself changes, this was added in #337.

pfrenssen · 2018-07-05T11:55:53Z

Haha Github doesn't allow me to reject my own changes 😁

pfrenssen · 2018-07-05T12:01:57Z

The schema change can just be removed, it has already been fixed in #323 by @claudiu-cristea. It is now duplicating it. I will clean this up and then rebase to clean up the commit history.

…oup content entities.

…e deleting them. This check only makes sense for group content entities.

This does a needless query, and now also throws an exception.

pfrenssen · 2018-07-05T15:05:54Z

OK this is looking good now. Some of the changes that were included here have been solved in other PRs in the meantime, so It is easier to read. It should be pretty easy to review now.

tatarbj · 2018-07-06T14:25:49Z

For me it seems a bit over of the original scope, maybe renaming the PR would make sense. Apart of it as we've also discussed there are no other cases when deleteOrphan() will be executed so it seems safe to delete the entity when it's not a group content (else case).
It looks good to me, thanks @pfrenssen for the clear comments, helped me a lot of understand the cases, also the inline comments are clear and proper!

pfrenssen · 2018-07-09T13:14:14Z

Do you mean this comment?

+    // If the entity is not group content (e.g. an OgMembership entity), just go
+    // ahead and delete it.

That is already what was happening before this PR, I just added some documentation here to explain what is going on.

I don't see where this is going out of scope? The PR adds the new exception that is being thrown if the getGroupIds() method is called with a wrong argument. There were two instances where this was called with the wrong argument in our own code, and these two are now fixed by first checking that the entity is group content before calling the method. And a test is added. There is nothing else in the PR?

…ontent

…ntent

src/MembershipManager.php

MPParsley · 2019-05-10T08:58:43Z

src/OgAccess.php

+      $cache_tags = $entity_type->getListCacheTags();
+
+      // The entity might be a user or a non-user entity.
+      $groups = $entity->getEntityTypeId() == 'user' ? $this->membershipManager->getUserGroups($entity) : $this->membershipManager->getGroups($entity);


Or we could test for Drupal\user\UserInterface?

Also use === of you want strict typing

Also might have been useful to have this check within getGroups().

I agree with this proposal but these lines of code are not in fact being changed here, this PR just introduces this single line and moves the related code in it:

if ($is_group_content) { // All code in here is in fact unchanged, but shows up in Github as a huge chunk of changed code. }

I will create a separate PR to implement this suggestion, so this can stay on scope.

Co-Authored-By: Maarten Segers <MPParsley@users.noreply.github.com>

pfrenssen added Bug Drupal 8 labels Sep 18, 2017

pfrenssen commented Sep 19, 2017

View reviewed changes

src/OgDeleteOrphansBase.php Show resolved Hide resolved

pfrenssen commented Sep 19, 2017

View reviewed changes

pfrenssen added the Ready for review label Sep 19, 2017

pfrenssen commented Jul 4, 2018

View reviewed changes

pfrenssen added 3 commits July 5, 2018 14:05

Make sure that MembershipManager::getGroupIds() is only called for gr…

a8f7ca8

…oup content entities.

Don't check if OgMembership entities are associated with groups befor…

55ffd91

…e deleting them. This check only makes sense for group content entities.

Don't request groups for entities that are not group content.

ca33d8b

This does a needless query, and now also throws an exception.

pfrenssen force-pushed the reject-non-group-content branch from b5d89ce to a8f7ca8 Compare July 5, 2018 12:10

pfrenssen added 2 commits April 23, 2019 22:11

Merge remote-tracking branch 'origin/8.x-1.x' into reject-non-group-c…

36148e8

…ontent

Merge remote-tracking branch 'gizra/8.x-1.x' into reject-non-group-co…

e4e6b65

…ntent

pfrenssen requested a review from MPParsley May 10, 2019 08:37

MPParsley reviewed May 10, 2019

View reviewed changes

src/MembershipManager.php Show resolved Hide resolved

MPParsley reviewed May 10, 2019

View reviewed changes

MPParsley approved these changes May 10, 2019

View reviewed changes

Apply suggestions from code review

978bbe6

Co-Authored-By: Maarten Segers <MPParsley@users.noreply.github.com>

pfrenssen merged commit 74319ac into 8.x-1.x May 10, 2019

pfrenssen deleted the reject-non-group-content branch May 10, 2019 18:16

pfrenssen mentioned this pull request May 10, 2019

Improve the checks if an entity is a user #495

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw an exception when calling `MembershipManager::getGroupIds()` with an entity which is not group content #311

Throw an exception when calling `MembershipManager::getGroupIds()` with an entity which is not group content #311

pfrenssen commented Sep 18, 2017 •

edited

Loading

pfrenssen commented Sep 19, 2017

pfrenssen Sep 19, 2017

pfrenssen Sep 19, 2017

pfrenssen Sep 19, 2017 •

edited

Loading

pfrenssen Jul 4, 2018

pfrenssen Jul 4, 2018

pfrenssen commented Jul 5, 2018

pfrenssen commented Jul 5, 2018

pfrenssen commented Jul 5, 2018

tatarbj commented Jul 6, 2018

pfrenssen commented Jul 9, 2018

MPParsley May 10, 2019 •

edited

Loading

pfrenssen May 10, 2019 •

edited

Loading

Throw an exception when calling MembershipManager::getGroupIds() with an entity which is not group content #311

Throw an exception when calling MembershipManager::getGroupIds() with an entity which is not group content #311

Conversation

pfrenssen commented Sep 18, 2017 • edited Loading

pfrenssen commented Sep 19, 2017

pfrenssen Sep 19, 2017

Choose a reason for hiding this comment

pfrenssen Sep 19, 2017

Choose a reason for hiding this comment

pfrenssen Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

pfrenssen Jul 4, 2018

Choose a reason for hiding this comment

pfrenssen Jul 4, 2018

Choose a reason for hiding this comment

pfrenssen commented Jul 5, 2018

pfrenssen commented Jul 5, 2018

pfrenssen commented Jul 5, 2018

tatarbj commented Jul 6, 2018

pfrenssen commented Jul 9, 2018

MPParsley May 10, 2019 • edited Loading

Choose a reason for hiding this comment

pfrenssen May 10, 2019 • edited Loading

Choose a reason for hiding this comment

Throw an exception when calling `MembershipManager::getGroupIds()` with an entity which is not group content #311

Throw an exception when calling `MembershipManager::getGroupIds()` with an entity which is not group content #311

pfrenssen commented Sep 18, 2017 •

edited

Loading

pfrenssen Sep 19, 2017 •

edited

Loading

MPParsley May 10, 2019 •

edited

Loading

pfrenssen May 10, 2019 •

edited

Loading