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] Throw exceptions from Authorize.net rather than return errors #17500

Merged
merged 1 commit into from
Jun 8, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

This is part of 'modelling good behaviour' - curently doPayment converts the errors to thrown exceptions,
but the recommendation is that the payment processor functions should throw exceptions themselves.
If they do they willl bypass the doPayment handling, but acheive the same thing

Before

return self::error(9001, 'Payment interval must be at least one week');

is received by CRM_Core_Payment::doPayment and which then throws an exception

After

Exception thrown in the first place

Technical Details

@KarinG basically we need to do this in a bunch of places - in core & also encourage extension writers to do so.

Comments

@civibot
Copy link

civibot bot commented Jun 5, 2020

(Standard links)

@civibot civibot bot added the master label Jun 5, 2020
This is part of 'modelling good behaviour' - curently doPayment converts the errors to thrown exceptions,
but the recommendation is that the payment processor functions should throw exceptions themselves.
If they do they willl bypass the doPayment handling, but acheive the same thing
@KarinG
Copy link
Contributor

KarinG commented Jun 5, 2020

Right - I started replacing the self::error (for iATS) with:

throw new CRM_Core_Exception

but I'll change that to

throw new PaymentProcessorException

Learning something every day :-)

@eileenmcnaughton
Copy link
Contributor Author

Yep - it would probably work the same but the latter is better form

if (is_a($result, 'CRM_Core_Error')) {
return $result;
}
$this->doRecurPayment();
return $params;
Copy link
Contributor

Choose a reason for hiding this comment

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

won't calling functions potentially miss error messages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doRecurPayment won't return an error now - it will throw exceptions

@seamuslee001
Copy link
Contributor

changes seem fine to me

@seamuslee001 seamuslee001 merged commit 5b4c15a into civicrm:master Jun 8, 2020
@seamuslee001 seamuslee001 deleted the renew_can branch June 8, 2020 01:54
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