dev/core#659 Catch payment processor exceptions, log, hide, do not return 500 error #13796
Conversation
|
(Standard links)
|
eileenmcnaughton
force-pushed
the
exceptions
branch
from
63b46c5 to
26c3374
Compare
|
@eileenmcnaughton I'll test and add to a client site that has the issue. The code changes make sense and it would be good to get in an rc early. |
|
@eileenmcnaughton I applied the patch, and have made payments with multiple processors. All succeed. I have set up recurring with PayPal and will monitor that. |
CRM/Core/Payment.php
Outdated
| @@ -1401,7 +1410,7 @@ public static function handlePaymentMethod($method, $params = array()) { | |||
| if (!$extension_instance_found) { | |||
| $message = "No extension instances of the '%1' payment processor were found.<br />" . | |||
| "%2 method is unsupported in legacy payment processors."; | |||
| CRM_Core_Error::fatal(ts($message, array(1 => $params['processor_name'], 2 => $method))); | |||
| throw new CRM_Core_Exception(ts($message, array(1 => $params['processor_name'], 2 => $method))); | |||
There was a problem hiding this comment.
Can we use the new array syntax here?
|
@eileenmcnaughton Can you give a bit more background here. The changes are probably ok but will affect EVERY payment processor that implements standard IPNs. It should, at least in part, be up to the processor to set the error code - eg. in stripe I use |
|
@mattwire this wouldn't stop processors setting their own code. What it would do is stop core from going into the fatal routine as a result of an uncaught exception - that routine lands it in 'abend' with a status other than 0 - hence a 500 error is triggered. |
CRM/Core/Payment.php
Outdated
| ); | ||
| } | ||
| catch (CRM_Core_Exception $e) { | ||
| Civi::log()->error('payment_callback_exception', [ |
There was a problem hiding this comment.
Can we have "ipn" somewhere in the name? Makes tracing the source a bit easier when found in logs
There was a problem hiding this comment.
I've added it - I was a little loath as ipn feels like a paypal term rather than a generic term but I guess we do tend to use it generically
There was a problem hiding this comment.
Great :-) Maybe this: https://bitpay.com/docs/invoice-callbacks or https://stripe.com/docs/charges makes you feel more comfortable about the term IPN?
|
@kcristiano is happy with this. I think I am based on your reply @eileenmcnaughton - Can you just make two minor changes per my comments and then we can merge? |
eileenmcnaughton
force-pushed
the
exceptions
branch
from
26c3374 to
33d4540
Compare
|
I've just pushed @mattwire 's changes @kcristiano do you think we should merge this to master or to the 5.12 rc? |
|
@eileenmcnaughton while this has not solved the issue @Stoob has, it is an important improvement.I think we should put in 5.12rc |
eileenmcnaughton
force-pushed
the
exceptions
branch
from
33d4540 to
b2a883a
Compare
|
Ok - it's on the rc now - @seamuslee001 I think we should merge this on pass & consider for 5.11.x |
|
Added merge on pass and will consider the issue around the 5.11 backport as well, however this makes sense to be in the RC |
|
unrelated fail - merging |
Overview
Prevent 500 errors being returned to processors
Before
When an error occurs in our processing a 500 http error may be returned. This can cause the processor to mistrust the url
After
Errors are caught & logged but do not affect processor response
Technical Details
Comments
@kcristiano