CRM-20608: IPN thinks Paypal Pro is Standard. #11777
Conversation
|
ping @eileenmcnaughton Any thoughts? Allen and I are working with the same client (JV), so I would rather not same-shop-merge, but this is a bug that has been causing a few IPN headaches. I'll make sure we are running it in production before merging. |
| 'sequential' => 1, | ||
| 'id' => $params['processor_id'], | ||
| 'api.PaymentProcessorType.getvalue' => array('return' => "name"), | ||
| )); |
There was a problem hiding this comment.
As there should only be one processor returned when specifying an id why not use PaymentProcessor.getsingle, or drop the sequential and check that the ID exists in the returned values array. Currently with this change, it will generate a PHP notice if [values][0] is undefined for some reason (eg. payment processor has been deleted).
There was a problem hiding this comment.
Fair point. What should happen if the given payment processor has been deleted? I guess a fatal "not found" error is the best we could do, right?
There was a problem hiding this comment.
@twomice I'd suggest using Civi::log()->error() with a suitable error message. Then return/exit or throw an exception rather than trigger a fatal error. @eileenmcnaughton You'll probably have an opinion on this :-)
There was a problem hiding this comment.
I think it should fatal, either through an exception or a call to fatal(), so that PayPal can detect that the IPN is failing (and the reporterror extension can detect it and notify admins). In other words, I'm a bit uncomfortable with Civi::log()->error(), because it can easily go unnoticed.
There was a problem hiding this comment.
@mlutfy Could the reporterror extension not pick up Civi::log() style errors too? I find it a real pain when you get lot's of fatal error info in the log files which is not always clear what the actual error is. A single-line Civi::log()->error() can show exactly what the issue is if the message is written in a sensible way.
There was a problem hiding this comment.
For the extension, currently it doesn't, but you're right I should add support for it. However, I still think it should fatal so that PayPal can detect the error. It's not an error that should ever happen, and if it does, it should fail badly. It's not something where we're telling something polite to the user and logging more details under the hood.
There was a problem hiding this comment.
Just on the error side of things - I think throwing an exception is generally better than 'fatal' because fatal should be seen as deprecated & Civi::Log-> is better than CRM_Core_Error::debug_log_message but I see the point that people aren't used to looking there & also Civi::Log should be pluggable in output. In general I'm not strongly on any side here
There was a problem hiding this comment.
I've replaced calls to CRM_Core_Error::debug_log_message() with exception; also added an exception if the processor can't be found by ID.
|
@mlutfy I have looked at this enough that I want to give you my blessing to go ahead & merge it when you are happy. I feel confident the code makes enough sense that the same-shop-merge side is covered off & there are enough people engaged that I don't think I need to get heavily involved on this one |
…wrong processor type.
twomice
force-pushed
the
CRM-20608_paypal_ipn_confuses_pro_vs_standard
branch
from
1e200a0 to
b501170
Compare
|
Merging based on:
|
Overview
As described in the ticket, regardless of whether the old way (extern/ipn.php) or new way (civicrm/payment/ipn/#) is used, the IPN interprets the IPN message as if it's Paypal Standard. As a result, IPN notifications for recurring payments are not processed properly.
Before
All incoming PayPal Pro IPN notifications for recurring contributions are successfully recorded in civicrm_system_log, but never make it as far as being recorded as payments in CiviCRM.
After
All incoming PayPal Pro IPN notifications for recurring contributions are correctly recorded as payments in CiviCRM.
Technical Details
As far as I can see, the existing code gets it backwards when asking "is this paypal pro or standard?". This PR reframes the question by querying the API for the given payment processor and testing against paymentprocessor.name instead of the slightly less clearly meaningful paymentprocessor.class_name.
Comments
I guess we want unit tests for this?