Proposal - Require Civi\Payment\PropertyBag. Partial compatibility break.
This is a duplication of what I wrote in the dev-digest to explain the compatibility break decision that needs to be made with PropertyBag to the wider community.
As I mentioned in the digest I spend nearly 2 days doing documentation updates & research to write this all up but that is the end of my involvement - I won't be reading / commenting on / making decisions on this.
===== FROM dev-digest=============
We have a proposal to change the contract that payment processors have with core and with other extensions. This change will hopefully not cause issues for many sites but if it does it’s likely to result in payment processing breaking.
The history
It has traditionally been difficult for payment processors, and code calling payment processors, to know what parameters they can expect to receive from the form layer. In fact I think the documentation cleanup I did in preparation for this dev-digest is the first time they were documented. Some of those params have been standardised over the years but it’s still not uncommon to see code ‘testing’ multiple params - eg. authorize.net still checks ‘total_amount’ even though we standardised on ‘amount’ many years ago. The format for the billing fields is pretty funky (‘billing_street_address_5’ where 5 is the location type id for billing).
In late 2019 we looked at adding a bunch of getters to the CRM_Core_Payment class that the other processors inherit - eg. getBillingStreetAddress()
. However, it turned out some of these might cause breakage (ie stripe had already implemented functions with the same names) and Rich Lott came up with the idea of the PropertyBag - an object that implemented array access that could be passed to doPayment as if it were an array and accessed as if it were an array - allowing us to pass through something more powerful without breaking the contract.
However, as we worked through it we found that we couldn’t just pass out the PropertyBag in place of the $params array and be confident that nothing would break. We can be confident that the vast majority of code will continue working but not 100% of code. Historically, this was mitigated by making the PropertyBag optional - each payment-processor could internally use the PropertyBag (or continue using the array).
The change
Recently, Matt opened a PR which would pave the way to a full transition to the PropertyBag (https://github.com/civicrm/civicrm-core/pull/20408). The test suites have shown that Authorize.net integration would break if we switch to PropertyBag, so the PR specifically fixes Authorize.net. However, these unit tests are the “canary in the coal mine” - changing the processor to make them pass requires a decision to proceed with a compatibility break which we have avoided making up until now.
It turns out, this compatibility-break can be avoided by altering PropertyBag design (in ways that may or may not be acceptable), but a contract-change would bring other, unavoidable compatibility breakage so the decision must be made at some point.
The community / other mergers must decide if the benefit of the change outweighs the risk. I have detailed below information to assist with that.
The contract
function doPayment($params, $component);
function hook_alterPaymentProcessorParams($paymentObj, &$rawParams, &$cookedParams);
Currently, doPayment($params)
and alterPaymentProcessorParams(....$cookedParams)
are references to a PHP array.
The proposal changes $params
and $cookedParams
to PropertyBag
references.
What still works if we pass a PropertyBag in place of the array
$paymentToken = $params['payment_token'] ?? NULL;
Any parameter that the processor is used to accessing as an array key is still available. In addition the newly defined ‘preferred variants’ work.
$paymentToken = $params['paymentToken'] ?? NULL;
And the new property methods work ie
$paymentToken = $params->has('paymentToken') && $params->getPaymentToken();
What breaks
Type hinting - eg. in wmf code it turned out we passed $params to an function like
function parseRecurFields(array $params)
which resulted in a fatal if $params was a PropertyBag
Array functions used on the params array eg. array_merge, array_diff_key etc. The only one of the php array functions that could still work (depending on which approach we use) is array_key_exists. This pattern was hit in Stripe (which I think Matt has now removed)
Iterating through $params. This is the pattern in Authorize.net that is the canary currently up for execution - so for example
$isRecur = $params[‘is_recur’];
Will be 1 if it is set but
$myParams = [];
foreach($params as $key => $value) {
$myParams[$key] = $value;
}
Will leave $myParams
as an empty array
What is affected Matt has done an audit of published payment processors with recent-ish updates and concluded pattern 3 is not present in any of them. I think he would have spotted patterns 1 & 2 as well.
We really can’t do much to audit the prevalence of these patterns in unpublished code (I picked up the type hinting issue in Wikimedia code because I was able to throw the change at the internal CI process). This is an unknown unknown.
Options
3 ways we can proceed with the property bag:
-
Shoot the canary - accept the 3 forms of breakage discussed. Change the code to not break rather than change property bag to not break the code.
-
Leave the canary alone - but agree to some breakage later - In the course of writing this up I tested whether we could avoid breakage type 3 and I found it was avoidable, at the cost of some strictness (as of writing tests are still running on my preferred variant) and this one has passed tests
-
Resolve to keep property bag as an optional helper class that payment processors can cast to if they want. The input array can be converted by calling
$propertyBag = \Civi\Payment\PropertyBag::cast($params);
This means that the propertyBag will never ‘pass the doPayment veil’ but we can potentially use it on both sides of that line. (ie. never break contract)
Strictness vs helpfulness
I think this is the fundamental tension of the PropertyBag
. I think we went into it with 2 goals with eventually turned out to be in conflict
- Strictness - get rid of the ‘anything goes’, make it consistent and predictable, create a mechanism to push out deprecation notices to ‘improve coders behaviour’ and allow us to eventually remove weird stuff
- Helpfulness - make it easy for the dev because they ‘all work’. Make the code UI discoverable
Option 2 moves the needle from strictness to helpfulness considerably - since you can simply do
$params = (array) $propertyBag;
And you basically have the original array, plus some extra params that are consistent and preferred (option 2 & 3 are not mutually exclusive so this could be the case with option 3 too.)
However, this approach kinda neuters efforts to be strict - ie you can pretty much avoid all the notices that the property bag tries to send you and you can ‘just use it like an array with reliable variables’.
The design for property bag already leans to the side of strictness over usability (e.g if you do $propertyBag->getBillingSupplementalAddress2()
it will give an error if the value has not been set and there is no IDE-discoverable way to retrieve the submitted credit card expiry date or helpers libraries offer like getting the card expiry year padded to 4 digits). Depending on your point of view being able to circumvent the propertyBag design is an upside or downside of option 2. (Both these strictness over usability decisions were contentious so points of view on the benefits of making it easier to circumvent the property bad design are likely to differ)