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

Standardize deletion mechanics across ~13 BAOs #22207

Merged
merged 13 commits into from
Dec 6, 2021

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Dec 2, 2021

Overview

Refactors various BAO code to use new deleteRecord function and deprecate ad-hoc del() functions. Gets the test in #22205 to pass.

Note: This fixes some intra-alpha regressions with ManagedEntities. Following #21989 and #22068, it becomes mandatory to support the hook_post for APIv4-based cleanup of managed-entities. (This is true regardless of whether downstream's specific record is inserted via APIv4. It only matters if the entity declares support for APIv4 management.)

Before

Less standardization of delete functions and the hooks they call.

After

More standardization.

Technical Details

Annotating a BAO::del() function as @deprecated tells APIv4 not to use it. It will call deleteRecord instead.

Entities failing the test in #22205:

  • CaseType
  • Contact (false-positive, test needs fixing)
  • ContactType
  • FinancialAccount
  • FinancialType
  • LocationType
  • MembershipStatus
  • MembershipType
  • MessageTemplate
  • OptionGroup
  • PaymentProcessor
  • PaymentProcessorType
  • RelationshipType
  • WordReplacement

@civibot
Copy link

civibot bot commented Dec 2, 2021

(Standard links)

@civibot civibot bot added the master label Dec 2, 2021
CRM/Case/BAO/CaseType.php Outdated Show resolved Hide resolved
@colemanw colemanw force-pushed the baoDeleteHooks branch 4 times, most recently from 585abf0 to 39dfda3 Compare December 4, 2021 04:51
@colemanw
Copy link
Member Author

colemanw commented Dec 4, 2021

@totten I'm not sure how to interpret the test output. Retest this please.

@colemanw
Copy link
Member Author

colemanw commented Dec 4, 2021

@totten this now fixes all 13 noncompliant BAOs, but Jenkins seems to be choking on something and I think it's unrelated to this PR. "Fatal: At least one command failed abnormally [ mixin]"

@colemanw
Copy link
Member Author

colemanw commented Dec 6, 2021

Retest this please

@colemanw
Copy link
Member Author

colemanw commented Dec 6, 2021

@totten this is passing now. We could merge it and then retest #22205

@totten
Copy link
Member

totten commented Dec 6, 2021

this is passing now. We could merge it and then retest #22205

Yeah, sounds good. This looks like it was a significant effort - requiring some cleanup in ~13 entities!

I had some concern/hesitation about the size of the changes, and I want to leave a few notes on that. At a high level, moving logic from the create/delete methods to the hook-listeners poses a risk for contrib hook-listeners (because a contrib listener could find its relative placement changed in the flow of code). However, I think we should go ahead and merge. There are two major mitigating factors here:

  1. These specific entities do not currently emit pre/post hooks - that's why they've been targeted for changes. No hooks fired in the past -- so no existing contrib-listeners. It's hard to be absolutely confident, but at a high level it should be fine.

  2. The change is not just an idle refactoring - it's needed to fix intra-alpha regressions with handling of managed entities. And we need to move to beta ASAP. It's better to get the fix we have (and which now passes the main test-suite) so that we can freeze (and then get more r-run exposure in the beta period). The alternatives would either lock-in the regression or reduce the length of the beta period.

So, yeah, let's merge - and branch for 5.45.beta soon.

@totten totten merged commit 781f55c into civicrm:master Dec 6, 2021
@totten totten changed the title [REF] - Switch BAOs to use standard delete function which calls hooks Standardize deletion mechanics across ~13 BAOs Dec 6, 2021
@seamuslee001 seamuslee001 deleted the baoDeleteHooks branch December 6, 2021 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants