Payment processor handling of `billing-country-5` inconsistent
This is a follow on from #3918 (closed) / https://github.com/civicrm/civicrm-core/pull/24232 / https://github.com/civicrm/civicrm-core/pull/24903
The original PR https://github.com/civicrm/civicrm-core/pull/24903 did not explain what problem was being solved & it didn't come out in the review process.... so I think in the first instance we should establish that.
This is my best guess to reverse engineer the point of the original change ...
All calls to doPayment
are expected to pass through the documented parameters https://docs.civicrm.org/dev/en/latest/extensions/payment-processors/create/#billing-fields - in this case the relevant parameter is billing_country-5
. That parameter is a bit of a pig so some/ most code ALSO passes country
- and this seems to be done consistently enough that Paypal
and Authorize.net
(our oldest processors) are using country
.
I do think it is important that when payment processors are sending out an array the contract of what keys they should pass out is clear. If we want to deprecate billing_country-5
in favour of 'country' that seems OK (we would need to update the docs) - but at the moment there seems to be some code that passes country
in a format that is not correct - which led to the regression.
I feel like in the first instance we should find & fix (with tests) the reported instances of that that came up in #3918 (closed)
Looking at the code for setBillingCountry
and the validation - I have a feeling that code may only be hit from the unit tests? In which case the case for reducing the validation seems strong enough.
One thing we COULD do in the test suite is register a hook on all unit tests for alterPaymentProcessor
& validate the country
field in it - that might track down any errant ones
I should note that PropertyBag
isn't a subsitute for us fixing core code to pass the contract parameters