Develop getter & setter structure for Payment classes

added needs:concept approval triaged type:proposal labels

changed the description

mentioned in issue #76

Strong agreement ... also, for bonus points, let's name things in a way that is compatible or at least not overly confusing when compared to other systems (e.g. omnipay and drupal commerce).

@AlanDixon here are the Omnipay fields.

firstName
lastName
number
expiryMonth
expiryYear
startMonth
startYear
cvv
issueNumber
type
billingAddress1
billingAddress2
billingCity
billingPostcode
billingState
billingCountry
billingPhone
shippingAddress1
shippingAddress2
shippingCity
shippingPostcode
shippingState
shippingCountry
shippingPhone
company
email
card
token
amount
currency
description
transactionId
clientIp
returnUrl
cancelUrl

I would say clientIP would be more consistent with our patterns & we have chosen paymentToken in other conversation & I think cardNumber. I think 'type' should be cardType for clarity.

Most of the others are billing & shipping fields & seem OK-ish. Any thoughts from the Commerce side?

The only commerce plugin I wrote was a million years ago and I've forgotten everything. Here's the documentation I could find: https://docs.drupalcommerce.org/commerce2/developer-guide/payments/create-payment-gateway/on-site-gateways/creating-payments

It does occur to me that our payment processing interface is fairly high level, e.g. we don't specify completed vs authorization. I know you've written some re-usable bits as has Matt, to avoid re-inventing so many wheels, so I'm not sure if this is the right place to think through that as well, or if that's a later step?

In my experience the naming we've got is reasonable enough, so feel free to consider this full approval in concept from me.

@AlanDixon yes let's open separate issues for related improvements & keep this one on the getters & setters

mentioned in issue #83

mentioned in commit core@06a551e4

mentioned in issue #93

We merged this for a bunch of these. We haven't done the more 'personal' ones - address, name etc, at this stage

I'm late to the party and my opinion goes against the grain here.

What we're doing here is providing getters and setters to what are essentially supposed to be method arguments.

My concerns are that we end up with empty method signatures that give no clues to its use or what parameters it's assuming to be available via getters. This is my experience with quickform ("where is _contactID set?!"). We could request that coders set or reset all properties before calling a method, for clarity, but that doesn't seem particularly reasonable, esp. when there's lots of logic that might go into determining which params should be set.

CRM_Core_Payment does a lot more than doPayment, and different processors will require different data for different functions; e.g. take Stripe which needs one of two different types of token depending on whether it's a recurring contrib or not (and some other inputs) for a doPayment.

And take the case of cancelSubscription() - what this needs may depend on the processor; currently the code makes the (false) assumption that all it needs is the value of civicrm_contribution_recur.processor_id passed in as subscriptionId. There's a PR (thanks @eileen) that now ensures the contribution_recur_id is passed in, but you'd never guess that from the function signature (or, currently, the docblock).

And then there's situations where you might feasibly need current and new data seprately - e.g. updating a subscription, you might want to check whether the existing amount is different to the new amount. Can't do that with a single getAmount() getter.

It's great that we're trying to get rid of mixtures of things like invoiceID and invoice_id. And it's great that we want to validate currency and pseudoconstants etc. - I'm all on board with that.

But it does feel like we're mixing up the needs of a logical function with object data properties that might not be correct for different a different function.

A better pattern might be:

$data = new CRM_Core_Payment_StandardPropertiesBag();
$data->setCurrency('GBP')
 ->setPaymentInstrumentID('Stripe')
 ->setInvoiceID('aabbccddeeff');
$myPaymentProcessor->doPayment($data);

...
class CRM_Core_Payment_MyProcessor {
  public function doPayment(CRM_Core_Payment_StandardPropertiesBag $data) {
    $data->require(['currency', 'paymentInstrument', 'invoiceID']);

    // OK, we're good to use our getters!
    $pay_instrument_id = $data->getPaymentInstrumentID();
  }
}

That way you can have your basic validation go on in the standard properties bag's setters and feed in the appropriate data to the doPayment() function.

Or even (pseudocode again) - something yet more typed, so that we can reuse types in multiple fields, e.g. a text field with a max length, or if we needed to pass in old_financial_type and new_financial_type...

use Civi\Payment\StandardPropertiesBag;
use Civi\Payment\StandardProperty\Currency;
use Civi\Payment\StandardProperty\Amount;
use Civi\Payment\StandardProperty\PaymentInstrument;

$data = new CRM_Core_Payment_StandardPropertiesBag();

$data->setCurrency(new Currency('GBP'))
 ->setPaymentInstrument(new PaymentInstrument('Stripe'))
 ->setInvoiceID('aabbccddeeff') // simple string - or could validate length...
 ->setCustomNamedProperty('new_amount', new Amount(1.23))
 ->setCustomNamedProperty('current_amount', new Amount(1.05));

It would also be nice to sort out when the translation from name to ID happens, when it doesn't and how to get the version you need.

Sorry, I realise I'm late in on this, I also realise that we're not in a position to start from scratch, but wanted to chip in.

@artfulrobot I agree it would be better to pass in a properties bag instead of an array. The psuedocode you lay out looks nice.

The question is how do we transition. doPayment expects an array & I don't think we can really change that. We could aspire to getting to the point where we do

$paymentProcessor->setPropertyPage() $paymentProcessor->doPayment([]);

And the processor can 'act like' the property bag was passed in.

Hmmm. Does it actually require an array, or does it require something that implements ArrayAccess interface?

If we could get away with the latter, then we could implement ArrayAccess on the property bag. And if we trained the more recent setters and getters to use an internal propsbag, we'd have one source for validation and access.

Before calling doPayment...

Old code will still be like this:

$params = ['contribution_recur_id' => 123, 'amount' => 1.23, 'currencyID' => 'GBP'...];
$processor->doPayment($params);

Current code:

// What 'newer' code passes in to doPayment is a transitional messs, right?
// Some props may be passed in as an array, while it assumes others will be gotten
// via a getter - take recent cancelSubscription() conclusion where we now
// pass in a subscriptionId but also setContributionRecurID().

$processor->setContributionRecurID(123);
$processor->doPayment(['someother' => 'param']);

Future code

$propsBag = new PropsBag();
$propsBag->setCurrency('GBP');
// ...
$processor->doPayment($propsBag);

And inside doPayment...

Old code:

// We can access stuff like an array:
// if the caller was old, it will actually be an array.
// if the caller was newer/newest, the PropsBag ArrayAccessor returns our values
// the propsBag could handle deprecated calls like the following by mapping to new.
$currency = $params['currencyID'];

Current code:

// We might have mixed things like this:
$currency = $this->getCurrency();
$other_prop = $params['other'];
// In which case: the getter works (value returned from internal propsbag)
// The array access works (either it is an array, or it's a props bag that looks it up)


// Or this
$this->inputParams = $params;
$currency = $this->getCurrency();
$other_prop = $this->getOtherProp();
// ...which also works because the getters use the props bag.
// getters could merge input params into the props bag on access
// which is probably the best we can hope for.

Future code:

// While we are transitioning (see below):
$propsBag = $this->handleLegacy($params);
// Clarify for all which props are required at the start of function.
$propsBag->require(['currencyID', 'contributionRecurID', ...]);
$currency = $propsBag->getCurrency();

Future code's CRM_Core_Payment::handleLegacy($params) would look like

function handleLegacy($mixedParams) {
  // get the bag used by setters.
  $propsBag = $this->getInternalPropsBag();
  // merge in the input params - might be a props bag or an array.
  // the merge method uses the propbag's setters, thereby validating/munging dodgy array data.
  $propsBag->merge($mixedParams);
  return $propsBag;
}

I think the outstanding issue is how a props bag handles multiple values, e.g. old and new, for a particular type of field. The aproach I penned above (with one object per thing) seems pretty full on, but perhaps we could have optional labels for getters and setters, e.g. $propsBag->setAmount(1.23, 'new'); ... and $propsBag->getAmount('new') - with 'default' being used if the label is not provided.

In summary, I think the value today for something like this:

validates and normalises inputs
calls to $propsBag->require() make it clear what a method needs - self documenting.
handles legacy crap
handles current hybrid mess

And the value in the future, once the legacy (and use of current Payment setters/getters) stuff is gone is:

we can clear out calls to handleLegacy()
we can remove the setters/getters (which would be just wrappers around a propsbag)
we have tidy code and isolation of method inputs between calls, which is good especially in cases where a processor object might be reused for a couple of purposes - there's no left over props from the last call that could interfere.

Thoughts?

Tagging you @mattwire as I know you've done work on this in mjwshared.

@artfulrobot I'm interested! We can pretty much switch from setters to a property bag now if we move before the rc is cut- if you want to put up a poposal

I think within the processors I'd still go with the getters we have. I think it's a smoother transition - easy to port into the extensions & they wont care if the getter is doing return $this->getValueFromPropertyBag() or just return $this->value;

We need the processors to switch over significantly over the next ? 6-9 months IMHO & if it's a moving taget we want it to be really stable for thtem

OK, I'll see if I can get a proof of concept up.

(Really wish we'd included a setter for inputParams...!)

We can add that - we can also take some out - right up until Wed when the rc is cut it can be changed

OK, here you go https://github.com/civicrm/civicrm-core/pull/15697

I'd draw your attention to the 2 test files which hope to proove that (a) the property bag works in the way it should and (b) that the altered CRM_Core_Payment can use the property bag as envisaged.

See what you think - I think it actually looks quite promising. I have some more things to discuss on this.

@artfulrobot @eileen This approach looks really promising. I'm going to have to make changes to (all) my payment processors to work with 5.20 because a number of the new core getters clash with my implementions (differences in declarations etc). I don't know if @eileen will have the same problem with omnipay? That said, I'd rather make the changes once and move to something really robust and "future-proof" like this.

Ok, so my next implementation questions are:

What do we do about setContactID('123')? Currently PHP's signature specifies the parameter as an int but of course in CiviCRM we frequently have these things as strings. At the mo, as seen in the tests, it works to pass a string (I'm unsure at which stage it gets coerced into an int) but php will probably be generating warnings. Should we be really strict (likely cause havok with existing code) or should we relax the signature (we can keep it in the docblock)
What do we do about pseudoconstants like contributionStatusID? It's nice to set them as strings - makes code more succinct and readable. But if we setContributionStatusID('Completed') then is getContributionStatusID() 1 or is it Completed? Note that this particular example is helped by PropertyBag's labels - because contributionstatusid could apply to Contributions or ContributionRecurs (which require different validators) - hmmm!

What do we do about setContactID('123')? Currently PHP's signature specifies the parameter as an int

My feeling is let it be passed in as string but validate as Integer > 0 in setContactID, throw and exception if it doesn't validate. There are various reasons why things might come in as a string, such as untyped return values from APIs but I can't see any reason why you'd want to set an invalid contact ID.

contributionStatusID as int/string.

Historically we've had massive problems with labels/names and stuff breaking. With an int (looked up via pseudoconstant) you can't go wrong. It would be nice to be able to pass in name (eg. Completed) and have the ability to get it back as a name or integer - maybe even request it back as a label if we were ever going to display this on screen? Maybe: getContributionStatusID(), getContributionStatusIDName(), getContributionStatusIDLabel() or something similar/more automatic?

I like all of the above @mattwire.

I definitely think we should support setting by string name, or integer.

Yes - we can check is_numeric & is so cast to an int in the setter

@mattwire @artfulrobot I had a thought - Matt's point about the getters conflicting with other functions has been worrying me a little TBH. What about if we move them into a Trait so processors can include them in a slightly less time-forced manner

Actually I see @artfulrobot has marked them all legacyMethods anyway - let's strip out all the ones added in 5.20 since Rich thinks they shouldn't be part of the final fix & they could cause issues

@eileen Now we have a promising alternative from @artfulrobot let's remove them for 5.20. Adding them to a trait wouldn't help me because I'm already doing that and shipping those traits in mjwshared - so if I included core ones I'd still have to update all my getters.

Also it's impossible to handle a trait not existing in PHP (as far as I know) so it would trigger a hard minimum version on 5.20 for any extension that included them.

@mattwire OK - if we are changing our pattern let's change it. I can't 'just revert' because we have 2 code places using the setters already but we can remove all the parts of https://github.com/civicrm/civicrm-core/pull/15509 that we no longer want

I've added a PR to revert both the setters & the 2 places it is in use- none of this code has hit the rc so I think reverting & re-doing is cleanest. Note some of the comment block text & the usages should be brought back in the new version

https://github.com/civicrm/civicrm-core/pull/15707

Do you think we should have setRecurringContributionStatusID() and setContributionStatusID() as separate things, while we're here? Currently code just uses contribution_status_id for ContributionRecur but they're not actually the same thing.

@artfulrobot yes - wee should stop squashing the 2

@mattwire @artfulrobot did you see my pr to revert the pre-Rich settters? I think we should merge that & then do this without the confusion of having it still in

mentioned in commit core@805b6544

@artfulrobot I think @mattwire merged mine so just rebase on master

(just deleted my comment after seeing that!)

@eileen @mattwire how many of these do you think I should add setters/getters for on Payment\PropertyBag?

In general I don't like the card specific parameters because they are specific to a payment processor rather than being generic to all. For example, stripe used to use them but now does not use any of them. No sites should be using the card number field except for storing the last 4 digits unless they are running on PCI compliant servers. I'm not sure quite how you'd separate these into logical "namespaces" or "collections" but payment processors supporting multiple methods (eg. stripe, Authorize.net) generally separate them into logical group. AuthorizeNet: https://lab.civicrm.org/extensions/authnet/blob/master/CRM/Core/Payment/AuthNetCreditcard.php#L138 + https://lab.civicrm.org/extensions/authnet/blob/master/CRM/Core/Payment/AuthNetEcheck.php#L141

We don't need shipping address.

Billing address we do need, but look at various functions in core that already manipulate them for inspiration such as prepareParamsForPaymentProcessor

Q1. I feel we should have getters and setters for every field in civicrm_contribution and civicrm_contribution_recur. This usually a superset of things needed by a given processor, but that's OK.

Q2. Maybe all other fields should be "custom" (new code should use $propertyBag->setCustomProp('myprocessor_xxx'))

The CRM_Core_Payment still has (at the mo):

baseReturnUrl
successUrl
cancelUrl
billingProfile
backOffice

Q3. Should any of these be moved into the internal property bag, or do they legit belong on the payment class itself?

Ouch there are a lot of fields in contribution & contribution recur. I wouldn't encourage we pass more fields than we already do because then people will be even more annoyed about them being available sometimes & not other times. I'd rather focus on being more consistent than on having more to be consistent about.

I think it's fine to leave the ones you mention on CRM_Core_Payment

I like how Omnipay accepts card expiry date & then you can retrieve with getExpiryMonth() and getExpiryYear(), likewise for amount it has some variants to get it in cents

I think we should still treat card_number, cvv and expiry date as generic parameters - in general we want to standardise parameters - although there are a small number that can't be - in Omnipay these are defined per processor with a function call & then matching params are set & passed through.

  /**
   * Get data that needs to be passed through to the processor.
   *
   * @return array
   */
  protected function getProcessorPassThroughFields() {
    $passThroughFields = $this->getProcessorTypeMetadata('pass_through_fields');
    return $passThroughFields ? $passThroughFields : [];
  }

I'd rather focus on being more consistent than on having more to be consistent about.

I'm not sure I understand. Where we're at now is:

Old code that calls a CRM_Core_Payment method will carry on passing in arrays of random stuff. Very occasionally some of those key/values may be subject to extra validation now (e.g. old code using get/set payment instrument).
New code should call CRM_Core_Payment methods with a PropertyBag that's got all the data set on it properly.

So at present, a PropertyBag has a setAmount and a setFeeAmount but it has no setTaxAmount, for example. And there's no setFinancialTypeID. There's also no setEmail. Anything that doesn't have a setter has to be set as a custom property via $propertyBag->setCustomProp('name', $value) - so that means we're forcing people to make up their own names for possibly standard things, which is what we wanted to get away from.

Maybe we don't need all the fields from contribution and contribution recur tables but we need a fair few of them.

Special Payment fields not directly related to tables in CiviCRM

amount (generic, usually total_amount but may be not)
description
feeAmount
isRecur
selected others from the list above?

Contribution fields

These are implemnented:

id contributionID
contact_id contactID
payment_instrument_id contactID
fee_amount We have feeAmount
invoice_id invoiceID
trxn_id transactionID
contribution_recur_id contributionRecurID
currency currency

These are not implemented but I think would be needed

total_amount ??? Well, we have amount
financial_type_id
receive_date
net_amount
cancel_date
cancel_reason
amount_level
is_test
check_number

These are not implemented but I don't think they need to be (because they're unlikely to be needed by a payment processor's interactions with the 3rd party)

contribution_status_id - probably out of scope
contribution_page_id - more likely details fed in through 'description' ?
invoice_number - again, maybe.
source - maybe
non_deductible_amount - I'm unsure on this one
thankyou_date
tax_amount
address_id
is_pay_later
campaign_id
receipt_date - out of scope for a processor, perhaps, unless the processor sends the receipts (GoCardless does)?
creditnote_id
revenue_recognition_date
is_template

ContributionRecur fields

These are implemnented:

id contributionRecurID
contact_id contactID
amount
currency
frequency_unit recurFrequencyUnit
frequency_interval recurFrequencyInterval
payment_token_id paymentToken
trxn_id transactionID BUT NOTE THIS IS SHARED WITH Contribution - do we need recurTransactionID?
invoice_id invoiceID BUT NOTE THIS IS SHARED WITH Contributionn, recurInvoiceID?
payment_instrument_id - shared, probably OK though

These are not implemented but I think would be needed

installments
start_date - may be set by the processor
end_date
is_test
cycle_day
processor_id - terribly named field. recurProcessorID?
financial_type_id

These are not implemented but I don't think they need to be

cancel_date - in future, we might have cancelSubscription that takes a date, but we don't now.
cancel_reason
create_date
modified_date
contribution_status_id - probably out of scope
next_sched_contribution_date
failure_count
failure_retry_date
auto_renew - I'm unclear on how this differs from it being a recurring contribution
payment_processor_id - Well we'd just use $processor->getID() rather than pass it in a propertyBag?
campaign_id
is_email_receipt

So I agree these are needed

description
installments
is_recur
start_date

I think sticking with 'amount' is fine.

We want the things that the payment processor to expect to receive to be the things we actually have available to pass to it & also avoid passing things that we think should be core not processor things - ie Processors should return fee_amount (sometimes) & not concern themselves with net_amount & some of those other fields definitely feel like cases where the processor is likely trying to do more than be a processor if it needs them.

Custom properties should really be limited to things that are specific to the processor - eg. I think Paypal passes through a 'PayerID' as well as a token. I would prefer to see the PaymentProcessor class have to define (like the getPaymentFields, getBillingFields) any additional fields it can receive. It kind of has to anyway in that the way to add them to the quickform & get them to the processor is by passing them through.

end_date? otherwise we expect processors to calculate this from installments + start_date + recurFrequencyUnit and recurFrequencyInterval? I expect some processors want an end date, instead of installments.

Yes I suppose is_test we know from the payment processor config.

Gosh stepping back this has been one helluva journey. Where it started was adding a whole bunch of getters and setters, adding inputParams. Then property bag came and tried to work on top of all that. Then we reaslised that had never made it to release and we stripped it all back again.

I'm now left thinking that part of the problem that was originally being discussed is on the output side.

e.g. payment prop bags have [gs]etFeeAmount() because that was one of the things added in the journey above, but you're now pointing out that this isn't something we'd ever set from the outside - a Payment Processor might know about the fee and might return it from a doPayment (or possibly from other actions, e.g. if there are transaction fees asssociated with changes to a subscription - although not sure we handle those now) - but we don't request a fee, so it shouldn't be an input.

So half the problem seems to be standardising the data currently passed into the following functions as an array:

doPayment()
changeSubscription()
cancelSubscription()
doRefund()
doQuery()

An array allowed too much freedom ending up with different spellings of params and different coding of values, so PropertyBag helps here; it means processors can reliably find their inputs under known names.

But is the other half of the problem the output? e.g. somebody (@mattwire ?) wanted setFeeAmount() - is that for the output of one of these functions? I sort of feel this shouldn't be such an issue but that the output results should be specified in the docblock (including adding commented code for changeSubscription() - we can't include a stub because of the implementation of supportsChangeSubscription). Or perhaps these functions should return a (new) propertyBag themselves, in which case we do need things like setFeeAmount().

Basically I would like to achieve

CRM_Core_Payment object not holding any data on contribution or contribution_recur data - when that's needed it should be passed in, not stored on the object itself. PropertyBag helps here and it also helps to standardise these data inputs
Clarify and standardise the processor interface generally - e.g. cancel and update recurring methods are problematic for me because they only provided the value of (get ready for this): civicrm_contribution_recur.processor_id which is extracted to a subscriptionDetails object under property name subscription_id and then passed in to changeSubscription() as subscriptionId (yes, Id not ID) - that's one bit of data given 3 different names, and it's not always what you need to do the job! We agreed before to also pass in ContributionRecurID but if we're doing that why not just pass in the BAO object anyway, which surely solves many of the issues of ambiguous naming?

I guess traditionally the BAO objects have not always been created at this point- we are trying to change that but it would generally mean that if the calling code has been cleaned up it would have to cast to BAOs before calling the processor unless it happens to have one from some process or other.

One of the main goals of this is to also start to clean up the code that passes stuff to the payment processor. At the moment the array of params is built up in some bits of code in crazy ways & we don't quite know what is going on. If we can see it being set on a settings bag ready to pass to the processor then we can distinguish between random mixing of arrays & actually useful things it does.

side notes

getFeeAmount would have been added for the calling function to get any generated value
I haven't hit any cases where the processor has needed to know the end date & there is kind of a reason for that in that they generally use frequency info or just create ongoing tokens.
I think we should add new doCancelRecurring, doChangeRecurring that wrap the current functions.
subsription_id / processor id is problematic because the temptation to use a meaningful name is very strong

Payments API means:

create [recur and] contrib records as pending
attempt payment
record successful payment outcome in recur, contrib records (and behind the scenes, the fin trans stuff)

CiviContribute payment pages also do this, don't they?

In what situation is a contribution/recur record not created before a payment processor is called to 'do' the payment?

Also, to be clear I'm talking about cancelSubscription and updateSubscription here - so there must be a record, right? Suggesting passing in a BAO object not an ID + random fields is just to save lookups really, it's likely that the caller already has these objects loaded.

getFeeAmount would have been added for the calling function to get any generated value

I think that's wrong. The function should return this data in its output; this transactional data should not be a gettable property on the payment processor.

I'm also not in favour (at the mo, I could be talked round) of adding/changing data on the passed-in arguments, whether that's &$params array or the PropertyBag object. Cleaner to keep functions as input data → process → return values output

I haven't hit any cases where the processor has needed to know the end date & there is kind of a reason for that in that they generally use frequency info or just create ongoing tokens.

OK, fine by me (GoCardless uses installments), thanks for clarity.

I think we should add new doCancelRecurring, doChangeRecurring that wrap the current functions.

What would they do that is different from the cancelSubscription, updateSubscription?

subsription_id / processor id is problematic because the temptation to use a meaningful name is very strong

I'd be in favour of renaming the table column from processor_id to subscription_id or even processor_subscription_id to be even clearer on where it comes from, and standardising that throughout getSubscriptionDetails and elsewhere.

In what situation is a contribution/recur record not created before a payment processor is called to 'do' the payment?

Ugh, too many situations currently: #53

getFeeAmount would have been added for the calling function to get any generated value

I think that's wrong. The function should return this data in its output; this transactional data should not be a gettable property on the payment processor.

All that matters to me is that there is a defined way to return parameters like the fee amount and know that they will be saved onto the appropriate contribution/payment record without any further action by the payment processor. Fees, transaction IDs and result codes (eg. "card declined", "authorised via 3dsecure", "whatever") are the only ones that come to mind but there may be additional custom stuff such as when the next payment will be taken, info that the next payment will require additional auth etc which we may want to add later.

I haven't hit any cases where the processor has needed to know the end date & there is kind of a reason for that in that they generally use frequency info or just create ongoing tokens.

OK, fine by me (GoCardless uses installments), thanks for clarity.

Annoyingly Stripe doesn't support an end date / installments on it's subscriptions. So you have to control that bit via CiviCRM. In 6.x I added support which does rely on the end_date here: https://lab.civicrm.org/extensions/stripe/blob/master/CRM/Core/Payment/StripeIPN.php#L437-461

I think we should add new doCancelRecurring, doChangeRecurring that wrap the current functions.

What would they do that is different from the cancelSubscription, updateSubscription?

Probably just that we could "start afresh" with sensible calling parameters and make them more consistent.

subsription_id / processor id is problematic because the temptation to use a meaningful name is very strong

I'd be in favour of renaming the table column from processor_id to subscription_id or even processor_subscription_id to be even clearer on where it comes from, and standardising that throughout getSubscriptionDetails and elsewhere.

See #57 (comment 19168)

OK - where are we at - lots of stuff going on here. At this stage I feel like the scope of THIS change is

add the PropertyBag class with setters that match properties that are currently passed in via $params
define / document the recommended way for processors to access those

Issues that this this seems currently stalled on-

how do we handle processor specific values
how do we cope with people wanting extra values we haven't thought of
how might we make CancelRecurring & ChangeRecurring more standardised.

I think 3 is kind of a side discussion but 1 & 2 take us back a bit to the point of this. If we look at the existing code that we want to clean up it basically merges a crazy amount of parameters into an array & passes it off to the payment processors. It's hard to touch / clean this up because we don't know what is going on so one goal is to say to the payment processors 'this is what you can expect to get from us, no more, if you want more then query the DB yourself'

On the processor side the goal is to no longer have to handle the fact that those accepted parameters can vary depending on where they are being passed in from (contributionID vs contributionID) & say just call $bag->getContributionID() & all is good.

In terms of processor specific custom values I don't think we should have any form of open ended passing variables in because it messes with the above. I think the payment processor needs to declare any other values it needs - e.g ones that come from js card handling & the calling code would only ever pass those as extra variables -

I get that people might want extra things & that discussing & negotiating that is a pain but I'd rather make the interface strict & have people discuss stuff than continue the situation where you never know if a parameter that is being passed in is actually required or whether it's just some legacy.

Re BAOS - the argument for a BAO over an ID is that IF the calling code already has all the info that the BAO might hold then it could cast that info to a BAO & pass that in. That's a big IF & basically means the calling code could only reliably meet the contract by doing a DB fetch before calling the payment action - which doesn't seem to gain us much.

My preference is to merge the most narrowly defined version of the change that takes us forwards & then negotiate further changes as we try to implement it

Thanks, @eileen I agree with your direction here, and I've hopefully done some thinking to ease the way forward.

Nb. I've also summarised the initial PR at the top of https://github.com/civicrm/civicrm-core/pull/15697 - I think that can be merged now and we start on a new PR for stage 2.

I've begun documentation at https://github.com/civicrm/civicrm-dev-docs/pull/735
which includes how to use the propertybags including a suggestion for core changes and how to allow processors to support their custom needs. I think that the proposals in the documentation address your concerns.

We can discuss them over there, or I can copy and paste bits into this thread. I'm not sure what's easier.

I've merged @artfulrobot's PR although I'm not 100% comfortable with the custom property handling. I feel that only properties that can be 'discovered' as required by the calling code should be able to be handled. However let's keep working on it over the next month & I reserve the right to comment out those functions in the rc if I don't reach full comfort with them :-)

@eileen at the mo we have data from the contribution/event pages that includes some known and some unknown fields (e.g. Stripe's tokens) which are known to the processor but not to the calling code.

So my suggestion was to let the processor handle any of it's custom needs.

The alternative would be that in the calling code, e.g. contrib page form ...

$requiredCustomFields = $processor->whatCustomStuffDoYouNeedFromTheInput();
foreach ($requiredCustomFields as $field) {
  // Some process to extract that, validate it and add it to the propbag.
  // Doubtless this would mean having to ask the processor how to do this.
}

But this creates a lot of duplication for no benefit (that I can see). So you probably meant something else and I've misunderstood :-)

Here's a potential problem: https://civicrm.stackexchange.com/questions/34215/exhausted-memory-error/34231

No action required, just posting for my own reference.

@AlanDixon shiesh, yeah that's not nice. I sometimes run up against things like that because php errors are passed from php-fpm in headers and it's possible to exceed the max size allowed for headers. Anyway, I have commented. But if he can fill up 1GB then surely there's a loop going on somewhere?

@artfulrobot @eileen I've put together a simple extension https://github.com/mjwconsult/civicrm-contributiontransactlegacy/blob/master/api/v3/Contribution/Transact.php that replaces (temporarily) the Contribution.Transact API as there are a few client who've come to me with problems with drupal webform_civicrm + stripe. It was a useful test of the documentation that @artfulrobot has provided here https://docs.civicrm.org/dev/en/latest/financial/orderAPI/#transitioning-from-contributiontransact-api-to-order-api

It's definitely pretty hacky.. and there's some nasty stuff in there to convert from propertybag to something that paymentprocessor.pay accepts.

The good points:

I fixed some typos in @artfulrobot documentation.
It forces people to upgrade to 5.21 if they have problems because this extension requires propertybag which is in 5.21 (so we get to start testing it).
@colemanw release webform_civicrm 5.0 today so we are now in a position we can look at getting a PR into webform_civicrm to replace usage of contribution.transact.
It allows us to test the code we'd need for a PR to webform_civicrm etc. in production.

The bad points:

It's a sticking plaster for integrations using contribution.transact.

@eileen @JoeMurray @artfulrobot It would be great if you had time to take a quick look over the code and comment - for example I hit a couple of issues which I've documented in the code eg. Class \Civi\Payment\CRM_Utils_Money not found

Thanks for the typos @mattwire

This line (and the next) has to go: https://github.com/mjwconsult/civicrm-contributiontransactlegacy/blob/master/api/v3/Contribution/Transact.php#L71

I would prefer to add this to propertybag:

/**
 * Returns all properties as an array.
 *
 * This is for legacy/transition use only.
 * @return array
 */
public function legacyToArray() {
  $a = [];
  foreach (array_keys($this->props) as $prop) {
     $a[$prop] = $this->getter($prop);
  }
  return $a;
}

In the API call at https://github.com/mjwconsult/civicrm-contributiontransactlegacy/blob/master/api/v3/Contribution/Transact.php#L81 we should not use array access on the propertyBag here. Array access is only for legacy code we can't control. So...

- 'total_amount' => $payParams['amount'],
+ 'total_amount' => $propertyBag->getAmount(),

- 'payment_processor_id' => $payParams['payment_processor_id'],
+ 'payment_processor_id' => $propertyBag->getter('paymentProcessorID', TRUE),

Thanks,

changed title from Develop getter & setter structure for Payment classes to Develop getter & setter structure for Payment classes (Payment\PropertyBag)

added Doing label and removed needs:concept approval label

Outstanding issues:

set/getAmount uses localised money format - it should use a number format eg. return filter_var($params['amount'], FILTER_SANITIZE_NUMBER_FLOAT, FILTER_FLAG_ALLOW_FRACTION);

We should add more parameters:

trxn_id
installments
recur start/end dates?