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

More del() deprecations in favour of standard deleteRecord() #25006

Merged
merged 1 commit into from
Nov 22, 2022

Conversation

aydun
Copy link
Contributor

@aydun aydun commented Nov 18, 2022

Overview

Follow on to #24999

This is a bit mechanical so please review the diffs.

@civibot civibot bot added the master label Nov 18, 2022
@civibot
Copy link

civibot bot commented Nov 18, 2022

(Standard links)

@colemanw
Copy link
Member

Thanks! Just to clarify, the pattern is to move any extra logic from the del function into hooks. In the first file of the PR, extension del contains nothing but the delete itself, so there's nothing to move. That file doesn't need a hook and doesn't need to implement HookInterface

@aydun aydun closed this Nov 21, 2022
@aydun aydun reopened this Nov 21, 2022
@aydun aydun force-pushed the bao_del branch 2 times, most recently from f899f9d to d74e726 Compare November 21, 2022 14:14
CRM/Core/BAO/Website.php Outdated Show resolved Hide resolved
@colemanw
Copy link
Member

All looks good. Thanks for working on this @aydun

CRM/Core/BAO/UFGroup.php Outdated Show resolved Hide resolved
@aydun
Copy link
Contributor Author

aydun commented Nov 21, 2022

@colemanw Can you see why this is failing? From the stack trace, it seems to be doing the expected things. It gets to deleteRecord() for MailingAB, then the post hook fires and tries to call deleteRecord() on the Mailings that are mailing_id_a, mailing_id_b and mailing_id_c but fails on one of those.

@colemanw
Copy link
Member

colemanw commented Nov 21, 2022

@aydun well the difference is that you've changed it from calling $obj->delete() on each mailing part over to using deleteRecord(). The latter throws an exception if it fails, the former does not. So the unit test may have had faulty data and no one ever noticed. I guess we could get parity by wrapping each call in a try/catch block....

Or we could punt and change them back to using $obj->delete() inside the post hook.

@eileenmcnaughton
Copy link
Contributor

Might be easier to pull out that BAO from this & handle on it's own

@aydun
Copy link
Contributor Author

aydun commented Nov 22, 2022

Ok, MailingAB moved out of here to #25024

@aydun
Copy link
Contributor Author

aydun commented Nov 22, 2022

Same error by a different path.... I'll move Mailing as well

@seamuslee001 seamuslee001 merged commit 4f87938 into civicrm:master Nov 22, 2022
@eileenmcnaughton
Copy link
Contributor

Well that got this one merged :-)

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.

4 participants