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] Simplify error handling in contribution import #24351

Merged
merged 1 commit into from
Aug 23, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

[REF] Simplify error handling in contribution import

Before

private function deprecatedFormatParams($params, &$values, $create = FALSE)
returns an array which is converted into an exception & thrown

After

The exception is just thrown directly

Technical Details

Although there is handling for different types of error those were not being hit - so this doesn't change that (just throws the type of error it was winding up being.) I also removed a validate on the campaign as that is checked in the validate process as is the fact the contact_id is an integer (although I cast it to an int as good practice when using in a query)

Comments

@civibot
Copy link

civibot bot commented Aug 22, 2022

(Standard links)

@civibot civibot bot added the master label Aug 22, 2022
@@ -626,19 +615,14 @@ private function deprecatedFormatParams($params, &$values, $create = FALSE) {

switch ($key) {
case 'contact_id':
if (!CRM_Utils_Rule::integer($value)) {
return civicrm_api3_create_error("contact_id not valid: $value");
Copy link
Contributor

Choose a reason for hiding this comment

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

@eileenmcnaughton why not just throw an exception here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cos it has already happened earlier on - that is a redundant check that was done during validate

@eileenmcnaughton eileenmcnaughton force-pushed the import_except branch 2 times, most recently from 742b29d to be79c37 Compare August 23, 2022 00:57
@eileenmcnaughton
Copy link
Contributor Author

Well merging that suggestion caused syntax errors so I decided to ride with the scope creep & fully remove that check to a sane place using shared code.

}
$values['contribution_campaign_id'] = $params['contribution_campaign_id'];
Copy link
Contributor

Choose a reason for hiding this comment

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

@eileenmcnaughton is there a reason for removing the campaign id validation here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep - because it is already done in the validate stage (so is the email validation now I look - that should come out too)

@eileenmcnaughton
Copy link
Contributor Author

Dang it has passed now - AND it's stale. I guess I might as well pull out that redundant email validation (validation is done before all the legacy code is hit - the legacy code doesn't do much but there is a lot of it :-)

Update CRM/Contribute/Import/Parser/Contribution.php

Co-authored-by: Seamus Lee <seamuslee001@gmail.com>
@colemanw colemanw merged commit e305b41 into civicrm:master Aug 23, 2022
@colemanw colemanw deleted the import_except branch August 23, 2022 21:36
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