dev/core#667 Fix bug where entity_tags are not deleted by 'soft FK'

[eileenmcnaughton]

Overview

Fixes a bug where deleting a contact does not delete tags associated with the contact

Before

1. Create a contact with a tag
2. Delete the contact (don't delete to trash, full delete)
3. check entity_tag record still exists in civicrm_entity_tag

After

entity_tag record is cleaned up

Technical Details

This utilises an approach @colemanw & @totten & I discussed about switching to a pre hook rather than ad hoc function calls. It's an initial foray & it would be good to limit it to being called on delete. Other entities could do with this - e.g the participants are deleted & then the participant notes in the current flow - I'm pretty sure the note won't be deleted due to the participant going first.

Comments

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[seamuslee001]

This looks fine to me would appreciate @totten just doing a review as well

[colemanw]

Wait... what?
Why would this hook be called with no id? Is that a bug?

[eileenmcnaughton]

It feels like it to me - but in activity BAO

  public static function deleteActivity(&$params, $moveToTrash = FALSE) {
    // CRM-9137
    if (!empty($params['id']) && !is_array($params['id'])) {
      CRM_Utils_Hook::pre('delete', 'Activity', $params['id'], $params);
    }
    else {
      CRM_Utils_Hook::pre('delete', 'Activity', NULL, $params);
    }

[colemanw]

Firing off an ad-hoc query feels icky somehow. Shouldn't we call pre/post hooks before deleting entityTags? And shouldn't there already be a bao function to delete entityTags? And can we avoid an infinite loop in the process of calling a pre hook from within a pre hook listener?

[eileenmcnaughton]

good point about the loop.

My instinct here was that this is kind of a bottom-of-the-food-chain entity & the efficiency of a single query makes sense

[eileenmcnaughton]

Oh - the existing entity-tag functions hurt my head

[colemanw]

Hang on, it looks like there was a civi.dao.preDelete event added a couple years ago. It's a bit lower-level and might be more appropriate than "hook_pre". https://github.com/civicrm/civicrm-core/blob/5.24/CRM/Core/DAO.php#L596-L609

[eileenmcnaughton]

ohhh

[eileenmcnaughton]

@colemanw so I feel like to merge this we need to resolve the following

1. should we use that preDelete hook - I didn't see in in your PR that I merged
2. is it OK the for kinda 'trivial' entities to not be individual deletes with hooks?

Performance wise it's a bit mixed - doing a get before any deletes does reduce locking operations but if a contact had hundreds of tags individual deletes would be a bit of a drag.

[colemanw]

So for:

1. Yes the preDelete hook gets called further down, inside $dao->delete() which is called by the new deleteRecord function (and I think $dao->delete() is at the bottom of every BAO delete function).
2. I guess it's a judgement call and there may be some grey areas, but since entityTag is essentially a join table, this seems like a pretty clear-cut "yes".

I guess it doesn't need to be in this PR, but I'd like to see a dispatcher system that makes it easier for entities to subscribe to create/update/delete events fired by other entities.

[eileenmcnaughton]

@colemanw I've updated it to use the other listener

[colemanw]

Looks good!

[eileenmcnaughton]

test this please

[colemanw]

test this please

[eileenmcnaughton]

Hmm it failed around the same place

`not ok 139 - Error: CRM_Case_XMLProcessor_ProcessTest::testCreateActivityWithDefaultContactByRelationship
not ok 140 - Error: CRM_Case_XMLProcessor_ProcessTest::testCreateActivityWithDefaultContactByRelationshipMissing
not ok 141 - Error: CRM_Case_XMLProcessor_ProcessTest::testCreateActivityWithDefaultContactByRelationshipBidirectional
not ok 142 - Error:

[eileenmcnaughton]

Ah it's because I deleted that fn you questioned - but it IS being called