dev/core#4418 Add in Upgrade script to fix double json encoding

[seamuslee001]

Overview

This adds in an upgrade script to fix potential double json encoding of the accepted credit cards

Before

Bad data possibly in the database

After

All good data

ping @larssandergreen @eileenmcnaughton @colemanw

[civibot]

Thank you for contributing to CiviCRM! ❤️ Here's what's next:

• If this is your first PR, an admin will greenlight automated testing with the command "add to whitelist".
• A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
• A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
• If this process needs to be repeated, an admin will issue the command "test this please" to rerun tests and build a new demo site.
• Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
• A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
• Here are some handy links for reviewers:
  • Demo site for this PR
  • Review standards
  • Review template

[demeritcowboy]

ts is unusual here, since it's just a one-off message for admins.

[seamuslee001]

There is some ts usage here https://github.com/civicrm/civicrm-core/blob/master/CRM/Upgrade/Incremental/php/FiveSixtyThree.php#L40 so shrugs

[demeritcowboy]

Those are common strings repeated across upgrades, so are better candidates for ts.

[demeritcowboy]

Although I'd argue they are still just for admins so shouldn't have ts either.

[seamuslee001]

I've removed the ts now

[demeritcowboy]

I'm getting TypeError: array_keys(): Argument #1 ($array) must be of type array, null given in array_keys() (line 70 of ...\CRM\Upgrade\Incremental\php\FiveSixtyFour.php)

If a pp doesn't have any selected the value is null.

[demeritcowboy]

Also calling json_decode on $paymentProcessor['accepted_credit_cards'] errors even when it's good because the api call returns an array (or something like that - error is TypeError: json_decode(): Argument #1 ($json) must be of type string, array given in json_decode() (line 71 of C...\CRM\Upgrade\Incremental\php\FiveSixtyFour.php)

[seamuslee001]

thanks @demeritcowboy I found some issues with my unit test which weren't pointing out those issues, I believe I have now fixed both issues you have reported and made sure the unit test covers all scenarios

[demeritcowboy]

Hehe made you work for this one. Take the rest of the day off ... as per dave's comments...

Works for me.

[larssandergreen]

Unfortunately, it's messier than this and this doesn't actually work to fix the values that are stored in the db for me.
I think because the test uses addValue it won't capture what happens when you pass an array that contains JSON as we were doing with save records => ['accepted_credit_cards' => json_encode($creditCards)]. Then you end up with this ["{\"Visa\":\"Visa\",\"MasterCard\":\"MasterCard\"}"], which isn't going to be fixed by json_decode.

[seamuslee001]

@larssandergreen they should be identical addValue v saveRecords should produce identical situations

[demeritcowboy]

I also had ["{\"Visa\":\"Visa\",\"MasterCard\":\"MasterCard\"}"] in the db, and the api.get returns it as an array, so then the [0] then gives the json string, which decoded for me to the right value.

But maybe we shouldn't be using api in the upgrade script, as has come up elsewhere.

[seamuslee001]

@demeritcowboy I think the API is safer now than earlier with regards to upgrades

[demeritcowboy]

Is Civi\Api4\PaymentProcessor a real class or is it some kind of virtual entity? It had no problem finding it during the real upgrade, but I don't see it in the tree?

[demeritcowboy]

Oh it's in the ext\civi_contribute extension, which I guess isn't enabled in the upgrade test suite.

[demeritcowboy]

So that might fail in real life if somebody didn't have that component enabled.

[seamuslee001]

@demeritcowboy oh probably because extension install tasks go last right so enabling the component extensions stuffs things up hmm

[seamuslee001]

ok @demeritcowboy have shifted to use executeQuery instead of the API in the upgrade should be right now right?

[larssandergreen]

OK, I just figured out how to make this work with the API version, but now I see there is a different SQL version. I'll give that a try.

[larssandergreen]

To make this work for me, I needed:
$paymentProcessors = PaymentProcessor::get(FALSE)->addWhere('is_test', 'IS NOT NULL')->execute();
and
PaymentProcessor::update(FALSE)->addValue('accepted_credit_cards', json_decode(($paymentProcessor['accepted_credit_cards'])[0], TRUE))->addWhere('id', '=', $paymentProcessor['id'])->execute(); (adding [0])

[demeritcowboy]

@larssandergreen I think you were testing the initial version. There was a second version that had those.

But @seamuslee001 did you want to keep the is_test IS NOT NULL AND domain_id IS NOT NULL part? I assume those are weeding out anomalies from pp's created by api or something.

[larssandergreen]

@demeritcowboy Oh, that's frustrating, must have missed that. Working for me now anyways.

[seamuslee001]

@demeritcowboy no i don't need that those were weeding out implicit filters that the API adds in (i.e. by default if you don't put a where in and an entity has domain_id column it will filter to the current domain and same with the is_test filtering by default to be true)

[demeritcowboy]

Ah ok got it.

So yes this works for me too.