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

[REF] Deprecate unnecessary del() functions #21200

Merged
merged 1 commit into from
Aug 22, 2021

Conversation

colemanw
Copy link
Member

Overview

This moves us toward a standard delete function for APIv4, by cleaning up and deprecating all the BAO::del() functions which do nothing special.

Before

Generic functions implemented copy/paste style.

After

Deprecated in favor of a single, tested generic function.

@civibot
Copy link

civibot bot commented Aug 20, 2021

(Standard links)

@civibot civibot bot added the master label Aug 20, 2021
@colemanw colemanw changed the title Deprecate unnecessary del() functions [REF] Deprecate unnecessary del() functions Aug 22, 2021
* @return CRM_Batch_DAO_EntityBatch
*/
public static function del($params) {
if (!is_array($params)) {
$params = ['id' => $params];
}
$entityBatch = new CRM_Batch_DAO_EntityBatch();
$entityId = $params['id'] ?? NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

As alarming as this is - I wonder if there is a reason for it

Copy link
Member Author

Choose a reason for hiding this comment

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

I suspect the "reason" is careless use of CRM_Utils_Array::value() which historically was used more than necessary, and recently would have been auto-converted to ??.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh fudge I was wrong:
https://github.com/civicrm/civicrm-core/blob/5.40/CRM/Financial/Page/AJAX.php#L473-L483
Ok I've updated the code to lookup the id when needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

@colemanw I think that is still wrong - I think that is a multiple row delete - perhaps replace that with a v4 api call & standardise delete? (We should do a universe search but could keep it 'liight' since people know they need to use the api for crud functions)

Copy link
Member Author

Choose a reason for hiding this comment

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

Crap. Ok I'll revert that file from this PR & we can tackle it separate.

Copy link
Contributor

Choose a reason for hiding this comment

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

good plan

Copy link
Member Author

Choose a reason for hiding this comment

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

@eileenmcnaughton
Copy link
Contributor

@colemanw the only one that is giving me pause here is EntityBatch - could there be a place where it is called with $params = ['batch_id' => 4];

CRM_Utils_Hook::post('delete', 'Campaign', $id, $dao);

return $result;
return 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

this one is weird to return bool - silly but OK

@eileenmcnaughton
Copy link
Contributor

OK - what's left looks equivalent to me (or in some cases will add hook calls which is a good thing)

@colemanw colemanw merged commit 994b284 into civicrm:master Aug 22, 2021
@colemanw colemanw deleted the delFunctions branch August 22, 2021 05:14
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