Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

APIv4 ConformanceTest - Demonstrate entity APIs which fail to emit hook_civicrm_post(delete) #22205

Merged
merged 2 commits into from
Dec 7, 2021

Conversation

totten
Copy link
Member

@totten totten commented Dec 2, 2021

Overview

Add assertions to APIv4's ConformanceTest to determine whether FooEntity::delete() emits hook_civicrm_post('delete', 'FooEntity').

ping @colemanw

Before

Some entities emit hook_civicrm_post('delete'...), and some don't... and it's hard to be sure which is which.

After

Some entities emit hook_civicrm_post('delete'...), and some don't... and the test tells us which is which.

Comments

As discussed in #22199 (comment), the event hook_civicrm_post('delete'...) becomes more important in the current master -- if this event is not supported, then the managed-entities cannot be cleaned up.

The list of non-conformant entities is fairly long: CaseType, Contact, ContactType, FinancialAccount, FinancialType, LocationType, MembershipStatus, MembershipType, MessageTemplate, OptionGroup, PaymentProcessor, PaymentProcessorType, RelationshipType, WordReplacement.

(Note: Contact is probably OK, but it fails the test because it uses alternative names like Individual.)

@civibot
Copy link

civibot bot commented Dec 2, 2021

(Standard links)

@civibot civibot bot added the master label Dec 2, 2021
@totten
Copy link
Member Author

totten commented Dec 2, 2021

@colemanw Wonder if it might be easier to switch ManagedEntities to listen for civi.dao.postDelete instead of hook_civicrm_post(delete)? OTOH, I guess we don't really need full compliance - we only need compliance for entities that use ManagedEntity trait?

@colemanw
Copy link
Member

colemanw commented Dec 2, 2021

IMO its pretty ridiculous that so many entities are non-compliant, and speaks to how these BAOs were originally written (by hand, with some loose conventions but no conformance tests).

I think this test is great, we should fix the DAOs to comply, and also add tests for 'pre' and 'post' hooks for every crud action.

FYI, the direction I'm trying to take our DAOs is to use generic create and delete functions that rely on late-static-binding and hook listeners, so adding this test can help nudge us further along that direction.

…`Contact`.

1. Expand assertions used for `hook_post` to check `hook_pre`.

2. Recognize the quirks in how `Contact` records are treated in the hooks.
@totten
Copy link
Member Author

totten commented Dec 3, 2021

A couple updates:

  1. Added a carve-out for the pre-existing quirk where Contact::delete is emitted as hook_post(delete, Individual, ...).
  2. Added assertion about hook_pre(delete) that works the same as hook_post(delete).
  3. Extracted a helper function for capturing the log. Should be easier to add more assertions in other subtests.

@colemanw
Copy link
Member

colemanw commented Dec 4, 2021

I've fixed all 13 non-compliant entities: #22207

@totten
Copy link
Member Author

totten commented Dec 6, 2021

jenkins, test this please

@seamuslee001 seamuslee001 merged commit 401a13b into civicrm:master Dec 7, 2021
@totten totten deleted the master-hook-conform branch December 7, 2021 01:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants