Add billingStateProvince and standardized property names

[mattwire]

Overview

Extracted from #21527. These are the simple bits that have been approved by @artfulrobot

• Add in billingStateProvince and getter/setter which is required for full billingAddress coverage.
• Map additional legacy params to propertyBag standard params.

Before

After

Technical Details

Comments

[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).

[mattwire]

It turns out we already have good test coverage on this - if you add the properties without this change you get errors like invalid function getBilling_state_province

[mattwire]

Merging per approval from @artfulrobot on #21527

[adixon]

I confess that the implications of the property bag are eluding me, but in my payment processor code for iats, I'm using code like this to convert params into something similar to pass off to the payment processor but with different array keys. I'd guess most payment processors have some similar code in them.

$request = array();
    $convert = array(
      'firstName' => 'billing_first_name',
      'lastName' => 'billing_last_name',
      'address' => 'street_address',
      'city' => 'city',
      'state' => 'state_province',
      'zipCode' => 'postal_code',
      'country' => 'country',
      'invoiceNum' => 'invoiceID',
      'creditCardNum' => 'credit_card_number',
      'cvv2' => 'cvv2',
    );

    foreach ($convert as $r => $p) {
      if (isset($params[$p])) {
        $request[$r] = htmlspecialchars($params[$p]);
      }
    }

Since this patch breaks it, I'm guessing that the isset test has stopped working, and that there should be a different way of testing whether a property of params is set.

What's the correct and backwards compatible way to replace this test?

[artfulrobot]

https://docs.civicrm.org/dev/en/latest/extensions/payment-processors/create/#introducing-propertybag-objects

@adixon In short, the equivalent to your code would be:

<?php

foreach ($convert as $r => $p) {
  if ($params->has($r)) {
    $request[$r] = htmlspecialchars($params->getter($r));
  }
}

An opinionionated view might be:

• you code currently relies upon mapping legacy array key values to the official proper names. street_address should be streetAddress in property bag speak.
• One of the points of property bag was to force us to be be explicit about the various bits of data we hold and require.
• This includes ideas like: a payment processor should not just pass every random key it received from who knows where onto the third party payment processor APIs (or Civi's APIs) (you're doing check already with your array filter). It should know what it requires in what situations and should ensure that its requirements are met, that the data is suitable in a standardised form, and that nothing else gets through.
• This pain in the behind reasoning was to guard against the array key free-for-all that had accumulated to-date, which led to a lot of confusion and made it hard to see what data keys were being required or used.

So, back to your code, let's assume that name, invoiceID, zip code and card details are required but all the other address details are not, it could be reworked like:

<?php

/** @var \Civi\Payment\PropertyBag $params */

// The IATS processor always requires the following data; if we don't have that we cannot proceeed.
$params->require([
  'firstName', 'lastName', 'invoiceID', 'billingPostalCode', /* Standardised properties */
  'cvv2', 'credit_card_number' /* our custom properties */
]);

// Map the values we've got into IATS request format:
$mapIatsToPropertyName = [
  'firstName' => 'firstName',
  'lastName' => 'lastName',
  'address' => 'billingStreetAddress',
  'city' => 'billingCity',
  'state' => 'billingStateProvince',
  'zipCode' => 'billingPostalCode',
  'country' => 'billingCountry',
  'invoiceNum' => 'invoiceID',
  'creditCardNum' => 'creditCardNum',
  'cvv2' => 'cvv2',
];
$request = [];
foreach ($map as $iats => $propertyName) {
  if ($params->has($propertyName)) {
    $request[$iats] = htmlspecialchars($params->getter($propertyName));
  }
}

There was a big discussion about credit card details and whether they should be standardised fields or not. They're not, in the end (not common enough across processors).Though this being Civi, there never really is an 'end' ;-)

What this means is that when property bag gets credit_card_number set on it, it reluctantly assumes the caller meant to set a custom field.

Ideally, custom fields should all be prefixed, e.g. iats_card_number or such, so that it's obvious who wants that data and why it's there. But this whole area has been a tough one with many opinions!

Hope this is helpful.

[adixon]

Thanks, super helpful! Opinionated is good! And thanks for spearheading the propertybag initiative!

Aside from being too lazy to dig into documentation, my other concern was about backwards compatibility, i.e. at what core version can I start safely accessing params as a property bag? [Okay, not as lazy as I claimed, I see you posted the link and that it was 5.24 which seems reasonable to asssume in my next release!].

As an aside, my fear here is that there's too much magic. I kind of like when things were simpler. Magic is great when it works, and when it doesn't, trying to debug it makes my hair fall out. So any new magic should first of all assume that it might not work all the time and account for humans trying to figure out why (e.g. well documented debugging mechanisms all along the pathways ...). No reply required, this is just me thinking out loud.

[artfulrobot]

@adixon

The magic is what we're moving away from. We're moving away from 'magic' that meant that 'contact_id', 'contactID' and 'contactId' all worked - if you were lucky., and that each payment processor chose a different version to rely on.

The only remaining magic is explicitly for the purposes of backwards compat. You're not supposed to rely on the magic any more, hence all the very strict getters and setters.

The best thing to do in your doPayment() is to start with:

$propertyBag = \Civi\Payment\PropertyBag::cast($params);

as seen at https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/Payment.php#L1366

And then work with that instead of $params. That way you'll be up-to-date and won't even need to change anything if/when doPayment starts getting called with a PropertyBag - the cast() call will simply return the original if it's already one.

The idea is that the UI (e.g. quickform) is decoupled from the payment system. So we can implement other payment UIs and still use Civi for processing them.

Of course different processors place requirements on the UI and need different fields. e.g. you need credit_card_number and Stripe needs paymentIntentID.

This requires there to be a mapping from whatever the UI generates to what payment processors can work with.

At the moment, it's assumed that doPayment takes data direct from quickform in an array of $params. i.e. our processors are kinda having to assume a UI and deal with some of that UI's quirks. But ideally that would change. I've made proposals, but it's a difficult area to make progress in because of all the existing payment processors that sort of need to move forward together.

[adixon]

Thanks for this.

[magnolia61]

With specific decimal separator settings this pr causes contribution checksum link payments to fail:
#21583

I opened up an issue for this: https://lab.civicrm.org/dev/core/-/issues/3413

[eileenmcnaughton]

This line may have caused a regression - https://lab.civicrm.org/dev/core/-/issues/3413 - it appears to be treating total_amount as amount when both are passed - I think this was probably done by accident since I think amount was consolidated quite a long time ago

[eileenmcnaughton]

Just noting that this has been reported as causing a regression (per my comment above) -