Standardise payment processing
This is a meta issue for getting more standardisation and clearer best practices in place for payment processing.
Many of our preferred practices are not new but are not clearly & consistently documented and processors that people copy are not modelling them well.
The key goals at this point are to
- model best practices in our core processors
- ensure our docs are up-to-date on best practice
- build a list of urls of payment processors & create tickets against them to implement the best practices
- get payment processor writers to subscribe to this meta-issue for updates.
- start the process of moving the unused core processors to extensions & then entirely out of core.
- done for eway, PayflowPro
- start to add deprecation notices to functions we do not recommend. Note that these should only show on dev environments as we recommend live sites set appropriate php error level settings. We should make a decent start on 1-4 above in the next 1-2 months and then get more serious about this one
Best practices to implement in core & promote
All processors should implement doPayment and not doDirectPayment, doTransfer. In order to make this change they need to do 3 things a) rename the existing function b) set
$result['payment_status_id'] = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Completed');c) handle $params['amount'] == 0 (generally return at that point, setting payment_status_id) d) throw exceptions on failure - do NOT return CRM_Core_Error
Handle the settings bag Processors should make their code able to handle the settings bag. The settings bag works like an array except that
- some array functions don't work - eg array_merge()
- it standardises input.
In order to prepare processors should
- Add the line
$propertyBag = PropertyBag::cast($params);as the first line it doPayment to ensure that they are consistently working with a propertyBag object
- replace array functions with functions that work on the property bag - eg replace empty() checks with $params->has('recurProcessorID')
Note that we will add to this as we convert core processors & identify the difficulties.
- Not extend BaseIPN & work to eliminate that class. currently we recommend processors implement
handlePaymentNotification()rather than have an IPN class per se but the core processors don't do that. We should model 'good behaviour' in our core processors & document. Currently our core processors are a bit nasty - subtasks on this are
- Getting a clear api/ expectations for cancel & failed payments out of BaseIPN
- Use guzzle not CURL & use GuzzleTestTrait to add tests. Note that we should focus on moving weird ones like FirstData outside of core but it's proven very easy to switch A.net over & we want to model this #143