Suppress legacy warnings by default in propertyBag to allow transition to propertyBag without hitting legacy warnings on unconverted payment processors

[mattwire]

Overview

Per mattermost thread https://chat.civicrm.org/civicrm/pl/3tzw46wtfidwbjhbdxqg6y6qfa

This allows us to actually implement propertyBag and pass it to doPayment() - example in 0bed925#diff-32a00907a954b52a78852aca14a4d2cca61f9dba805905c9053a1f2ec8ef70c0R340 without causing tests to fail when payment processors are accessing legacy properties.

By passing propertyBag we have full control over what parameters are being passed to the paymentprocessor and do not need to worry about setting multiple keys in case the processor looks for one rather than the other - eg. we use $this->setIsRecur() and the processor ideally accesses via $this->getIsRecur() but can also access using ArrayAccess via $params['is_recur'] or $params['isRecur'].

Before

Payment processors that do not cast to propertyBag trigger deprecated warnings on the legacy parameters if accessing them - eg. $params['is_recur'] instead of $params['isRecur']. Both keys contain the same value.

After

Payment processors that do not cast to propertyBag DO NOT trigger deprecated warnings on the legacy parameters if accessing them - eg. $params['is_recur'] instead of $params['isRecur']. Both keys contain the same value.

Technical Details

Explained above.

Comments

@eileenmcnaughton

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

[eileenmcnaughton]

Yes - we discussed & agreed this

[seamuslee001]

Test fails relate

Civi.Payment.PropertyBagTest.testSetContactIDLegacyWay
Civi.Payment.PropertyBagTest.testSetCustomProp

[mattwire]

Tests should pass now

[artfulrobot]

@mattwire I'm ok with suppressing deprecated notices by default, with the idea that they can be enabled for tests, what I don't get is that:

• looks like suppress deprecated is TRUE for all these tests now, since none of them call set...() and as TRUE is the default
• in which case, why the if (getSuppresed())?
• if the default got flipped back to FALSE, what would those tests do then? Be skipped, I think? Seems not quite right.

Would it be better to remove the if isSuppressed checks with a line after new PropertyBag() that calls setSuppressed(TRUE)? Or to provide tests for the if-not-suppressed case?

[mattwire]

@artfulrobot You are right, updated!