Details
-
Type:
Bug
-
Status: Done/Fixed
-
Priority:
Major
-
Resolution: Fixed/Completed
-
Affects Version/s: 4.6.14, 4.7.2
-
Component/s: CiviContribute
-
Labels:
-
Documentation Required?:None
-
Funding Source:Contributed Code
Description
ran into a rather obtuse error with with paypal standard IPNs.
to set it up, create a payment processor record using paypal standard. disable it. create a new payment processor record using paypal standard and attach to a contribution page. run a payment on the page. result: the payment remains in pending status because the IPN cannot locate the payment processor.
I traced it and the issue seems to originate in CRM/Core/Payment/PayPalIPN.php, around line 324, where we have:
$paymentProcessorID = CRM_Core_DAO::getFieldValue('CRM_Financial_DAO_PaymentProcessorType',
'PayPal_Standard', 'id', 'name'
);
This pulls the first available payment processor of type PayPal Standard – even if it's not the one used by this particular contribution. A few thoughts:
- in my case, the error was triggered because that ID makes its way to CRM/Contribute/BAO/Contribution.php around line 2351 (loadRelatedObjects) where we use the ID to retrieve the paymentProcessor. that then triggers an error in BaseIPN.php because no paymentProcessor is retrieved (since it's disabled). I suspect if the first processor was enabled it would succeed – even though it would be attached to the wrong payment processor.
- it seems the primary issue is that we need to know what payment processor was used for the transaction at that point in the process. it seems we should be able to know that from the contribution ID. but I'm always a bit fuzzy as to what should be retrieved from data and what is used by the IPN POST as a check/balance against the contribution.
Attachments
Issue Links
- is supplemented by
-
-
- Done/Fixed
-
- links to
Activity
@Brian I came to the same conclusion yesterday. Had a conversation with Eileen on IRC this morning and she agreed that it appears to be a bug.
I also wondered why the lack of payment_processor_id should throw a fatal error - especially when I find everything works just fine if I change "throw new CRM_Core_Exception" to "CRM_Core_Error::debug_log_message" in "wp-content/plugins/civicrm/civicrm/CRM/Core/Payment/BaseIPN.php" on line 177.
This problem also exists for PayPalProIPN. I reported it as ignoring the active flag https://issues.civicrm.org/jira/browse/CRM-18101 but the problem looks larger than I reported.
Tommy, Jane, and I did some research because we have a client where this is an issue--and Brian's right on that line in CRM/Core/Payment/PayPalIPN.php. It's equally an issue in 4.6.
Now, the problem gets a little hairier: if you're replacing that line with one looking up the payment processor ID rather than the payment processor type ID, how do you know which processor it is? You might have multiple PayPal Standard accounts, and there's nothing stored in the contribution or available in the message from PayPal with the ID.
We figured out that the business email address is in the IPN message, and that should be usable.
Created
https://github.com/civicrm/civicrm-core/pull/8011 for 4.6, and
https://github.com/civicrm/civicrm-core/pull/8012 for 4.7.
I have not tried #8012 with an actual IPN message from PayPal, so Brian Shaughnessy, it would be great if you could try it out. I have #8011 working on a live site with four separate PayPal Standard accounts plus two other payment processors, and it successfully updates transactions with the right one.
Note that the direction we are going in is to tell people to use ipn urls like
civicrm/payment/ipn/xx where xx is the payment processor ID.
I think this works with paypal but is not tested - hence we haven't taken any positive actions towards migrating people
I agree that Eileen McNaughton's method is more elegant for the future. In the meantime, however, I feel that just getting the account's email address and using the username to look up the processor should do it. Regardless, Brian Shaughnessy's initial bug definitely is a problem. I think those PRs solve both.
Tested and merged both the PRs
I did not realize this issue was already submitted, and I created what could be considered a duplicate (CRM-18302). The fix is actually quite simple. Add a parameter to the PayPal notify URL for the paymentProcessorID (in CRM_Core_Payment_PayPalImpl::doTransferCheckout) so it is returned to the IPN handler (CRM_Core_Payment_PayPalIPN::main) where it retrieved from the $_GET. Works like a charm. PR has been submitted.
If we are starting to change the notify url I'd rather change it to the non-deprecated one per https://issues.civicrm.org/jira/browse/CRM-18107?focusedCommentId=86710&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-86710
Yeah, I like Eileen's approach better for being more consistent with other payment processors. The rationale for not changing the URL is that existing payments need to work, and they'll have the old URL saved. A new URL following the standard pattern would be ideal, but the old one will need to work for at least a few years into the future while people have recurring contributions.
Just noting that I had cause to test the civicrm/payment/ipn/xx url on wordpress today & it worked although there was an issue that I patched because the payment processor encodes part of the url & we need to be able to handle that since they like doing random things to the urls
So on further examination, it's a bit more messed up.
The code listed above (getFieldValue) is completely wrong – that DAO is for the payment process type – not the payment processor actually used for this contribution. The above will almost always return "1" because that is the ID for the PayPal Standard type in default installs of Civi. I think this hasn't been noticed previously simply because most people will create only 1 payment processor in their system, and so if they're using PayPal standard, it will be ID = 1 (the above issue seems to be limited to PayPal Standard).
I think the correct way to fix this is to delete the above code and pass NULL to the validateData function. That let's the payment processor ID be retrieved from the corresponding contrib page downstream, and seems to work properly.