backdrop-contrib / og

The Organic Groups module provides users the ability to create, manage, and delete 'groups' on a site.
GNU General Public License v2.0
1 stars 8 forks source link

og_membership entries are not deleted when group content nodes are deleted #63

Closed argiepiano closed 2 years ago

argiepiano commented 2 years ago

How to reproduce:

  1. Create a group node type and create a group
  2. Create a group content node type
  3. Create a group content node and select the group created in step 1

When you do this, a new entry is added to the og_membership table indicating that the node created in step 3 is a member of the group node created in step 1

Then:

  1. Delete the node created in step 3 above
  2. Check the og_membership table.

The entry is not removed.

This has the potential of creating a huge og_membership table for large sites that have a lot of content removal.

Additional notes:

Once again, as in #56, the culprit is the order of things in Backdrop. In D7, hook_entity_delete() was invoked before any of the field attachers that take care of removing field entries for the deleted entity. This order is reversed in Backdrop. Therefore, in Backdrop OgBehaviorHandler::delete() runs BEFORE og_entity_delete(), resulting in the og_membership entry not being removed.

Unlike #56, the solution here is a bit more straightforward and does not require Backdrop core changes. I will provide a PR.

argiepiano commented 2 years ago

PR #64. @laryn, I've tested with the above steps and it works. Also I've tested by adding a user to a group, and then deleting that user completely from the system, to check that the membership is removed - it is. I have not tested with custom entities.

Still, before merging, let's take a look to see if this PR breaks any of the tests that were not broken before 😆 I can check that later.

argiepiano commented 2 years ago

@laryn, I've checked the tests. Everything looks fine - in fact this PR solved OgRoleRevoke, which was failing before and now it's not 👍🏽

So this is good to merge in my opinion if you think it looks good.

indigoxela commented 2 years ago

So this is good to merge in my opinion

Same here.

laryn commented 2 years ago

Thanks @argiepiano and @indigoxela!