WIP Multiple fixes/improvements to \Civi\Payment\PropertyBag

[mattwire]

Overview

• Map additional legacy params to propertyBag standard params.
• Add in billingStateProvince and getter/setter which is required for full billingAddress coverage.
• Ensure that property "getter" is used instead of generic getter (so we don't skip special handling eg. default for getIsRecur()).
• For billing address fields make sure we actually return the "new"/propertyBag name if we have a legacy property.
• Add default for non "essential" property description = ''
• Add default for invoiceID - generate per CiviCRM core if not defined - makes invoiceID generation consistent if using propertyBag for payment and does not rely on calling code to make sure it has been generated. Calling code can generate if it wants to.

Before

Missing functionality and some bugs

After

More functionality and less bugs

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]

Ensure that property "getter" is used instead of generic getter (so we don't skip special handling eg. default for getIsRecur()).

[artfulrobot]

Looks important. Could we have a test for this? Maybe add a case to

civicrm-core/tests/phpunit/Civi/Payment/PropertyBagTest.php

Line 416 in 2879e80

public function otherParamsDataProvider() {

(I think)?

[mattwire]

Add default for non "essential" property description = ''

There is no reason to fail if we don't have a description set. This makes it easier to use getDescription() when filling parameter arrays without having to check it's set first.

[artfulrobot]

There is no reason to fail if we don't have a description set.

Says who? Can't the same be said of anything else, e.g. getBillingStateProvince ?

[mattwire]

Add default for invoiceID - generate per CiviCRM core if not defined - makes invoiceID generation consistent if using propertyBag for payment and does not rely on calling code to make sure it has been generated. Calling code can generate if it wants to.

This is a property that is generated by calling code in various places using the same logic. In many cases it is not actually used at all and this reduces coding requirements when submitting payments using propertyBag because it will always have a default if not set manually.

[artfulrobot]

Hmmm. The purist in me thinks this is problematic, along the lines of: PB is supposed to ensure well-formed, structured inputs, not to generate content, defaults or default-providers.

I can see that it's a handy place to put in a shortcut though and can't bring to mind any practical problems it would cause. I suppose it feels wrong though if you had code like:

pseudo code:

$someParams = ['this' => 'that'];
$pb1 = PropertyBag::from($someParams);
$pb2 = PropertyBag::from($someParams);
//  You should expect pb1 == pb2 but it's not:
$this->assertEquals($pb1->getInvoiceID(), $pb2->getInvoiceID());

Or maybe:

// Civi core something
$contrib = creatependingcontrib();
$pb = new PropertyBag();
$pb->setBlah('blah');
$processor->doPayment($pb);

// in the processor:
callThirdPartyApi([
  'amount' =>$pb->getAmount(),
  'ourRef'=> $pb->getInvoiceID(),
]);

// Civi core
// ...

i.e. core has not set an invoiceID; the processor created one and sent it to a third party but then did not write it to the Contribution.

I'd be happier with this function somewhere else. e.g. a generateNewInvoiceID() in the Contribution BAO or something - so we're explicit when we're generating it.

[artfulrobot]

↑ maybe a PropertyBag->generateInvoiceID() would be better, then it's explicit when it gets created.

[mattwire]

@artfulrobot I've pulled out the more straightforward stuff into #21583