dev/core#3977 Handle dodgier calls to setBillingCountry in property bag.

[artfulrobot]

Alternative to #24903

As a solution to https://lab.civicrm.org/dev/core/-/issues/3977 & to getting back the regression in https://lab.civicrm.org/dev/core/-/issues/3918

This does what @mattwire's suggestion does but includes @eileenmcnaughton's suggestion of munging.

TL;DR:

• lots of code calls setBillingCountry incorrectly, some passing country IDs, nulls, empty strings, country names, the time of day - who knows.
• propertyBag thowing errors caused too much pain.
• this PR munges crap input where possible (translating IDs, names to ISO codes) but logs it as a warning.
• anything that can't be munged gets set to '' (empty string). boo.
• no exceptions thrown
• big test added.

[civibot]

(Standard links)

• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[mattwire]

@artfulrobot Have not tested yet but looks good - I like the approach and especially the warning "Given input is plain weird."

[eileenmcnaughton]

This looks good to me (I haven't reviewed in detail) & is closer to what I thought the point of the propertyBag is - ie to make it easier to work with inconsistent input (when I tried to use PropertyBag I found that it was actually stricter than just using the params in many cases - eg. getters that error if there is no value to get & require a 'has' check before 'getting')

I would note that primary way in which values are passed to doPayment is still by array & we should still

a) document any change in contract - ie if we are deprecating some keys & 'requesting' others that needs to be clear at https://docs.civicrm.org/dev/en/latest/extensions/payment-processors/create/#billing-fields
b) fix places that don't comply - in this case the regression has highlighted at least one

I would push for the more visible / test killing deprecation warnings over the logging because it shows up in our dev environments & unit tests

[artfulrobot]

This looks good to me (I haven't reviewed in detail)

Glad you're happy, and thanks for the review :-)

... is closer to what I thought the point of the propertyBag is - ie to make it easier to work with inconsistent input

yeah, we just disagree here, but that's ok. To me, property bag was not there to make up for sloppy coding elsewhere, it was to highlight it. Otherwise we'll continue to be sloppy and nobody will know whether it's contactID or contactId or contact_id or cid or id....

https://docs.civicrm.org/dev/en/latest/extensions/payment-processors/create/#billing-fields

Yes, that doc needs a lot of work. It's a great example of how messed up the CRM_Core_Payment class is in terms of separation of concerns etc. - e.g. it documents getters and setters/params but that class should not even be storing that data. IMHO. But I agree that we should document that billing country needs to be ISO-2.

I would push for the more visible / test killing deprecation warnings over the logging because it shows up in our dev environments & unit tests

test-killing deprecations - yes I always forget those. I can't recall how to (a) do it - I can grep around though, and (b) how this affects a test when you want to test that a deprecation message has been generated?

[eileenmcnaughton]

@artfulrobot this is how we add a deprecation error CRM_Core_Error::deprecatedWarning('blah blah);

Generally I would say our normal approach to highlight sloppy code is to add deprecation warnings & also to leverage our unit tests to find inconsistencies & lock in consistent behaviour (e.g registering a hook in our unit test classes that errors if the correct params are not set or are invalid). The PropertyBag is trying to do that in a way I guess - but in a way where it fatals on live sites rather than causes tests to fail / makes people doing core-development see the error :-)

However, it's probably good that we have flushed out that this is the underlying difference in our thinking about PropertyBag - because I think not being clear about that has muddied other conversations.

[artfulrobot]

@eileenmcnaughton ok how's this looking?

• now uses deprecatedWarning()
• except during the setBillingCountry test, because then we are testing that a deprecation warning is thrown, and we don't want that to show up as a test fail!

On the other behaviour of PropertyBag, pretty much all setters have been written to throw exceptions if invalid input is received. We did have it emit more deprecation/logs stuff in the past but this resulted in complaints about log size(!). Also I think we - I - thought that it would reveal dodgy calls much sooner than it has, which would clean up the upstream code (typically the changes are minor). But I guess the people that do the clean up are people who run tests, and if there's no tests covering that, the problem isn't shown up in testing and is instead found in production where people don't notice the warnings, so don't fix code. As a result we have had a long protracted series of adding heuristics and guesswork and munges, so I guess it's failed as an initiative or it just has a long way to go yet. Anyway, hopefully this PR whacks this mole for now!

[eileenmcnaughton]

@artfulrobot - the alternative to your last patch is

civicrm-core/tests/phpunit/CRM/Mailing/MailingSystemTest.php

Line 58 in 6b730d5

error_reporting(E_ALL && !E_USER_DEPRECATED);

In general the PropertyBag has two fairly separate concerns - to enforce code quality & to smooth inconsistent code quality - the former is great in a dev environment & the latter is great in prod. I guess we didn't have a shared understanding of these & which we were going for before starting.

However, I have some ideas about how we could improve code-quality enforcement via tests. and / or adding a new v4 Payment.pay api & migrating to it

[artfulrobot]

@eileenmcnaughton I think we still need my last patch to use deprecatedWarning() and the advantage of the way it is done as opposed to error reporting is that the way I've done it is specific; should the test cause code to run that touches other deprecated code, we would want that to show.

I have some ideas about how we could improve code-quality enforcement via tests. and / or adding a new v4 Payment.pay api & migrating to it

Sounds good feel free to @ me on discussions and I'll try to join in. Honestly I'm thinking we need something to replace CRM_Core_Payment. Anyway this is all off thread! 🤐

[eileenmcnaughton]

@artfulrobot OK - if you prefer it the way it is then let's merge it!