Add Payment PropertyBag for payment processor method arguments

[artfulrobot]

Summary

Below is a long thread that nobody should have to suffer, and also see https://lab.civicrm.org/dev/financial/issues/82 but I've edited this description as a PR summary/overview.

• This PR adds a new class, Civi\Payment\PropertyBag which will eventually replace the wild $params array that gets thrown at payment processors, with a standardised, at least minimally validated and type cast object. It has a test class, too, and is designed for backwards compatibility with older processor code that expect arrays as inputs.

• This PR only actually uses this class in one very minimal way at present (to validate currencies), and as such it should not cause any grief or changes to UI/UX or APIs, but it paves the way for a bigger PR to follow.

• The rest of the PR is comment updates.

I am working on documentation, but that can't really come out until the PropertyBag is properly used in core, but here's a static copy of that draft documentation to give you an idea of what the problem is and why we solved it this way. again, note that the stuff below can't happen until more changes are made in core, which are out of scope for this PR.

Introducing PropertyBag objects

Currently as of 5.19 $params is a sprawling array with who-knows-what keys. From 5.xx, a better way is to pass in a Civi\Payment\PropertyBag object instead.

This object has getters and setters that enforce standardised property names and a certain level of validation and type casting. For example, the property contactID (note capitalisation) has getContactID() and setContactID().

For backwards compatibility, this class implements ArrayAccess which means if old code does $propertyBag['contact_id'] = '123' or $propertyBag['contactID'] = 123 it will translate this to the new contactID property and use that setter which will ensure that accesing the property returns the integer value 123. When this happens deprecation messages are emitted to the log file. New code should not use array access.

Checking for existence of a property

Calling a getter for a property that has not been set will throw a BadMethodCall exception.

Code can require certain properties by calling
$propertyBag->require(['contactID', 'contributionID']) which will throw an InvalidArgumentException if any property is missing. These calls should go at the top of your methods so that it's clear to a developer.

You can check whether a property has been set using $propertyBag->has('contactID') which will return TRUE or FALSE.

Multiple values, e.g. changing amounts

All the getters and setters take an optional extra parameter called $label. This can be used to store two (or more) different versions of a property, e.g. 'old' and 'new'

<?php
use Civi\Payment\PropertyBag;
//...
$propertyBag = new PropertyBag();
$propertyBag->setAmount(1.23, 'old');
$propertyBag->setAmount(2.46, 'new');
//...
$propertyBag->getAmount('old'); // 1.23
$propertyBag->getAmount('new'); // 2.46
$propertyBag->getAmount(); // throws BadMethodCall

This means the value is still validated and type-cast as an amount (in this example).

Custom payment processor-specific data

Sometimes a payment processor will requrie custom data. e.g. Stripe may use a paymentMethodID and a paymentIntentID on its doPayment() method.

Property names should be prefixed, e.g. stripe_paymentMethodID and set using PropertyBag->setCustomProperty($prop, $value, $label = 'default').

The payment class is responsible for validating such data; anything is allowed by setCustomProperty, including NULL.

Where support for custom properties is needed for a method, e.g. doPayment(), you should implement a method as follows:

<?php
use Civi\Payment\PropertyBag;
use Civi\Payment\Exception\PaymentProcessorException;

class CRM_Core_Payment_MyExtension extends CRM_Core_Payment {
...
  /**
   * Copy our custom properties from the form data.
   *
   * @param PropertyBag
   * @param Array form values
   * @param String $component as for doPayment()
   */
  public function extractCustomPropertiesForDoPayment(PropertyBag $propertyBag, Array $input, $component = 'contribute') {

    // We require a paymentIntentID
    if (empty($input['paymentIntentID'])) {
      throw new PaymentProcessorException("paymentIntentID missing");
    }
    // (possible validation of the value goes here since it could be malicious)
    $propertyBag->setCustomProperty( 'stripe_paymentIntentID', $input['paymentIntentID']);

    // We might need a paymentMethodID but sometimes not.
    if (!empty($input['paymentMethodID'])) {
      $propertyBag->setCustomProperty( 'stripe_paymentMethodID', $input['paymentMethodID']);
    }
  }
  ...
}

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[artfulrobot]

test this please

[seamuslee001]

@artfulrobot you will need to rebase your branch as

In "web/sites/all/modules/civicrm/", rename "master" to "backup-master-20191105033950-10".
In "web/sites/all/modules/civicrm/", create "master" using "origin/master".
In "web/sites/all/modules/civicrm/", apply "https://github.com/civicrm/civicrm-core/pull/15697" on top of "302c4f9378c27c401c40a3833b7aa49f4a148bd5".

                                                                                           
  [GitScan\Exception\ProcessErrorException]                                                
  Process failed:                                                                          
  [[ COMMAND: git apply --check --ignore-whitespace ]]                                     
  [[ CWD: /home/jenkins/bknix-dfl/build/core-15697-1b71x/web/sites/all/modules/civicrm ]]  
  [[ EXIT CODE: 1 ]]                                                                       
  [[ STDOUT ]]                                                                             
  [[ STDERR ]]                                                                             
  error: patch failed: CRM/Core/Payment.php:275                                            
  error: CRM/Core/Payment.php: patch does not apply                                        
  error: patch failed: CRM/Core/Payment.php:291                                            
  error: CRM/Core/Payment.php: patch does not apply                                        
  error: patch failed: CRM/Core/Payment.php:335                                            
  error: CRM/Core/Payment.php: patch does not apply   

[artfulrobot]

I don't understand why the tests are failing - is this me or something outside of me? There's no logs when you click the details links!

[seamuslee001]

@artfulrobot it can't apply the PR ontop of master that is why it is failing

[artfulrobot]

@seamuslee001 interesting. Applies FF for me:

$ git checkout -b test1 origin/master
Branch 'test1' set up to track remote branch 'master' from 'origin'.
Switched to a new branch 'test1'
$ git merge mine/payment-property-bag 
Updating 302c4f9378..242de3baef
Fast-forward
 CRM/Core/Payment.php                              | 159 ++++++++++++++++++++++++++-----
 Civi/Payment/PropertyBag.php                      | 677 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/phpunit/CRM/Core/PaymentPropertyBagTest.php |  98 +++++++++++++++++++
 tests/phpunit/Civi/Payment/PropertyBagTest.php    | 259 +++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 1172 insertions(+), 21 deletions(-)
 create mode 100644 Civi/Payment/PropertyBag.php
 create mode 100644 tests/phpunit/CRM/Core/PaymentPropertyBagTest.php
 create mode 100644 tests/phpunit/Civi/Payment/PropertyBagTest.php

any ideas?

[seamuslee001]

@artfulrobot i would try when your on payment-property-bag do git pull origin master --rebase and follow the prompts on your cli. if it completes successfully then do git push mine payment-property-bag -f

[eileenmcnaughton]

@artfulrobot we don't need to add the legacy wrappers back in now do we? I mean they are wrappers that never were from a release POV

[artfulrobot]

@eileenmcnaughton do you ever sleep?! isn't it 4am where you are?!

Ah, that's interesting. See the ones that are left are ones that weren't added by the commit you reversed. So I left them in. Some of them work a bit weird.

[eileenmcnaughton]

Sleep not so good tonight....I'm pretty sure I DID add getContactID

[artfulrobot]

Updated status to WIP - I lost loads of code doing the rebase. Trying to play catchup.

[mattwire]

@eileenmcnaughton @artfulrobot It would be nice to see this in 5.20 as there's lots of payment related changes there. But perhaps we can merge early in the RC rather than rushing this or delaying to 5.21. As far as I can tell this will be an "optional" feature initially that processors can use (ie not going to break anything existing) and the sooner it's available the better!

[artfulrobot]

In theory yes, although until I get all tests passing I live in fear of it causing grief.
Also, just had big power cut here and it's killed my router! 😱 😨

[artfulrobot]

Just FYI: I've had to stop work on this because everything is against me. First the power cut, then the router, now buildkit won't ... you know build ... so I don't have a working dev env any more.

If anyone has any advice on this buildkit bug pls ping @artfulrobot on MM.

[eileenmcnaughton]

@artfulrobot I had decided this would miss 5.20 & we should maybe prioitise getting it right over the next couple of weeks. Basically my civi priorities over the next 2 weeks are this, any 5.19 regressions & chipping away at #15706

[artfulrobot]

@eileenmcnaughton good call, this isn't nearly ready. I've got buildkit reinstalled now (thanks to recent core fix by Tim) but the normal Civi tests won't run now because of Trait 'Symfony\Bridge\PhpUnit\SetUpTearDownTrait' not found in /buildkit/build/dmaster/sites/all/modules/civicrm/vendor/cache/integration-tests/src/SimpleCacheTest.php so I'm no further into being able to test! Have asked abou this on MM though.

[artfulrobot]

OK, I'm back up and running. I think I've fixed the un-passing tests. Notes:

• I had to change an old test - interesting, the new propertyBag picked up the error in this test that had gone unnoticed for I don't know how long.

• I started some legacy support hacks in an appropriate place (in the offsetSet which handles legacy code setting stuff via array accessor); I found 2 places in the existing tests that set fee_amount to a zero length string. Rather than change those tests to properly supply data, I added a specific case to coerce this to zero, as I suspect that pattern is found elsewhere too.

[artfulrobot]

done rebase

[totten]

Small aside (and not to increase the pressure...)... when you get past the WIP stage, I'd encourage a think on the comms (r-explain/r-doc). There's a lot of discussion in the PR+GL, which suggests that something interesting is going on, but the live discussion may only be accessible to folks who work regularly on Civi payment-processor code. You don't need to summarize now/midthread... but when you're happy on the substance, then the description or docs would be a good place for a recap.

[artfulrobot]

Assuming the tests pass, I'll remove the WIP from this and update the description as @totten suggests, along the lines of:

• introduce concept of Payment PropertyBag and our discussions
• explain that it's not actually used yet, except for validating currency
• explain that the PR mostly updates comments(!) and should not result in any changes to UI/UX/APIs

[eileenmcnaughton]

@artfulrobot sounds good to me - we need some best practice recommendations in the PR template and / or docs

[artfulrobot]

I've updated the PR description at the top.

I propose that (tests allowing) this gets merged and we move on to implementing PropertyBag in a new PR.

[eileenmcnaughton]

I'm going to merge this because I think it provides us with a good basis for improving our code. I'm still not entirely comfortable with the custom properties portion but I think if we can mark that as 'experimental' in the docs we can work through that with this code merged over the next little bit