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

[NFC] Minor code cleanup #16823

Merged
merged 1 commit into from
Mar 17, 2020
Merged

[NFC] Minor code cleanup #16823

merged 1 commit into from
Mar 17, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Mar 17, 2020

Overview

Fix some exception declaration, comparisons, handling for non-present param

Before

Exceptions not in comment blocks, single quotes where doubles are OK, handling for param that can't be null

After

Above cleaned up

Technical Details

Comments

@civibot
Copy link

civibot bot commented Mar 17, 2020

(Standard links)

@civibot civibot bot added the master label Mar 17, 2020
*/
function civicrm_api3_contact_delete($params) {
$contactID = CRM_Utils_Array::value('id', $params);
$contactID = (int) $params['id'];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Required param so we don't need to check it - must be an int so let's cast to one before passing on

@@ -466,15 +469,13 @@ function civicrm_api3_contact_delete($params) {
// restrict permanent delete if a contact has financial trxn associated with it
$error = NULL;
if ($skipUndelete && CRM_Financial_BAO_FinancialItem::checkContactPresent([$contactID], $error)) {
return civicrm_api3_create_error($error['_qf_default']);
throw new API_Exception($error['_qf_default']);
Copy link
Contributor

Choose a reason for hiding this comment

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

@eileenmcnaughton i note here your throwing an \API_Exception where your otherwise throwing a \CiviCRM_API3_Exception which one should it be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was actually just noticing that - I think API_Exception is right - not for a great logical reason - just more consistent. The other is thrown by the wrapper

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Fix some exception declaration, comparisons, handling for non-present param
@colemanw
Copy link
Member

Agree with the cleanup

@colemanw colemanw merged commit 7755d70 into civicrm:master Mar 17, 2020
@eileenmcnaughton eileenmcnaughton deleted the cont_api branch March 17, 2020 19:44
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.

3 participants