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

Clean up error handling in legacy functions in import parser #19160

Merged
merged 1 commit into from
Dec 12, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Clean up error handling in legacy functions in import parser

Before

Complicated error handling and code hard to move around because of the return statements

After

straight forward exception handling approach

Technical Details

This makes the handling of errors cleaner & makes it easier for us to unravel what is going on here.

Comments

@seamuslee001 this is another minor clean up step. I think it's going to make sense to copy the formatProfileContact into the import now that most of the first move is tidied up & then we can switch to calling the api - which will handle your bug

@civibot
Copy link

civibot bot commented Dec 10, 2020

(Standard links)

@civibot civibot bot added the master label Dec 10, 2020
return civicrm_api3_create_error('Invalid value for custom field \'' .
CRM_Utils_Array::value('name', $custom) . '\''
throw new CRM_Core_Exception('Invalid value for custom field \'' .
$custom['name'] . '\''
Copy link
Contributor

Choose a reason for hiding this comment

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

So in theory it could have returned NULL here right?

Copy link
Contributor Author

@eileenmcnaughton eileenmcnaughton Dec 10, 2020

Choose a reason for hiding this comment

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

@seamuslee001 yeah - there is some risk this would give an e-notice but it seems like our metadata is more consistent now so less of this belt & braces handling is better

@seamuslee001
Copy link
Contributor

Looks fine to me lets see what the tests say

@seamuslee001
Copy link
Contributor

test fails look to relate

@seamuslee001
Copy link
Contributor

@eileenmcnaughton test fails still occurring

@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 yeah - I think I have to take a step back on this & clean up some of the weird duplicate stuff first - the first step (the weird if-else stuff) is merged now so will look again

@eileenmcnaughton eileenmcnaughton force-pushed the import branch 2 times, most recently from b3fcc8e to f93665e Compare December 12, 2020 03:26
@seamuslee001
Copy link
Contributor

Jenkins re test this please

This makes the handling of errors cleaner & makes it easier for us to unravel what is going on here.
@seamuslee001 seamuslee001 merged commit 54a44bd into civicrm:master Dec 12, 2020
@seamuslee001 seamuslee001 deleted the import branch December 12, 2020 08:40
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