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

BAO - Noisily deprecate and stop using deprecated functions #25677

Merged
merged 1 commit into from
Mar 9, 2023

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Feb 27, 2023

Overview

This noisily deprecates functions that have already been deprecated in the BAO, and ensures they are not called from any core code.

Before

A bunch of BAO functions superseded by writeRecord and deleteRecord, but only soft-deprecated and still called from some places.

After

Now they're deprecated-er!

Technical Details

The @deprecated flag means that neither APIv3 nor APIv4 will call these functions, which means they're already unused by the API, so this removes the rest of the references to them and adds noisy deprecation which gets us a step closer to completely removing them.

@civibot
Copy link

civibot bot commented Feb 27, 2023

(Standard links)

@civibot civibot bot added the master label Feb 27, 2023
@eileenmcnaughton
Copy link
Contributor

@colemanw a couple of tested places hitting this - eg.

api_v3_SyntaxConformanceTest::testCustomDataGet with data set #12 ('Contribution')
Failure in api call for Contribution delete: Deprecated function CRM_Core_BAO_Address::del, use deleteRecord.
#0 [internal function]: PHPUnit\Util\ErrorHandler->__invoke(16384, 'Deprecated func...', '/home/jenkins/b...', 1089)
#1 /home/jenkins/bknix-dfl/build/build-0/web/sites/all/modules/civicrm/CRM/Core/Error.php(1089): trigger_error('Deprecated func...', 16384)
#2 /home/jenkins/bknix-dfl/build/build-0/web/sites/all/modules/civicrm/CRM/Core/BAO/Address.php(1215): CRM_Core_Error::deprecatedFunctionWarning('deleteRecord')
#3 /home/jenkins/bknix-dfl/build/build-0/web/sites/all/modules/civicrm/CRM/Core/BAO/Block.php(310): CRM_Core_BAO_Address::del('1')
#4 /home/jenkins/bknix-dfl/build/build-0/web/sites/all/modules/civicrm/CRM/Contribute/BAO/Contribution.php(1704): CRM_Core_BAO_Block::blockDelete('Address', Array)

@eileenmcnaughton
Copy link
Contributor

@colemanw on the first one I rather thought we could drop the call to the silly BlockDelete

image

@colemanw
Copy link
Member Author

Thanks @eileenmcnaughton I think I may have fixed them all now.

@eileenmcnaughton
Copy link
Contributor

@colemanw well I put it up separately anyway #25684

BTW - this PR changes 110 files....

@colemanw
Copy link
Member Author

@eileenmcnaughton yep, 110 files with minor changes, and all tests passing :)

This checkbox can help a lot with review:
image

@eileenmcnaughton
Copy link
Contributor

eileenmcnaughton commented Feb 28, 2023

@colemanw yeah - I need to work through the bits with more substantive change more carefully though - & it would go stale before I did since it's hard to even figure out which bits they are. My limit is about 10 files in a 'sitting'

I've pulled off the easy tests to a separate PR since I can fully rely on jenkins for then & that will get it down a bit

#25688

@colemanw
Copy link
Member Author

@eileenmcnaughton now only 87 files :)

@colemanw
Copy link
Member Author

colemanw commented Mar 1, 2023

@eileenmcnaughton 76 :)

@@ -189,7 +189,7 @@ public function postProcess() {
CRM_Utils_System::flushCache();

if ($this->_action & CRM_Core_Action::DELETE) {
CRM_Core_BAO_Job::del($this->_id);
CRM_Core_BAO_Job::del(['id' => $this->_id]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI this is not right

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Fixed.

eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Mar 1, 2023
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Mar 1, 2023
@colemanw
Copy link
Member Author

colemanw commented Mar 1, 2023

@eileenmcnaughton 59!

@colemanw
Copy link
Member Author

colemanw commented Mar 7, 2023

@eileenmcnaughton is this a reviewable number of changes?

@eileenmcnaughton
Copy link
Contributor

@colemanw I wind up checking it out one function at a time - ie confirm how one del works & then check all the changes to it

@colemanw
Copy link
Member Author

colemanw commented Mar 9, 2023

@eileenmcnaughton only 2 files left

@eileenmcnaughton
Copy link
Contributor

@colemanw yeah - I'm a bit torn about this one - it just feels wrong to catch the exception & quietly do nothing if the delete fails. I feel like it shouldn't occur - in which case we shouldn't catch it

@colemanw
Copy link
Member Author

colemanw commented Mar 9, 2023

Ok, yea. The catch wasn't there before so let's leave it out.

@colemanw
Copy link
Member Author

colemanw commented Mar 9, 2023

@eileenmcnaughton it's updated now

@eileenmcnaughton
Copy link
Contributor

test this please

@eileenmcnaughton eileenmcnaughton merged commit e985f1e into civicrm:master Mar 9, 2023
@eileenmcnaughton eileenmcnaughton deleted the noisyDeprecate branch March 9, 2023 22:46
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.

2 participants