fix parameters in Payment.get

[MegaphoneJon]

Overview

There's a bug in how financial transaction IDs are searched in APIv3 Payment.get.

Technical Details

The old code produced a malformed parameter such that Payment.get with a contribution ID specified never returned a value. Since this hasn't been caught be tests, I don't think this is very serious - I came across it while testing something that turned out to be unrelated.

Comments

Since tests are passing and no one has reported an issue here, any test failures this provokes should be indications of a deeper bug elsewhere.

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

[MegaphoneJon]

OK - this passed all tests, and meanwhile I decided to do a git blame and found that the last edit to these lines was a refactor - and before the refactor, something very similar to my code exists. Feeling much more confident about this as a solution.

[colemanw]

@MegaphoneJon I just stared at this PR diff for 5 minutes and I still don't see the functional difference between the two. They both look like they'd produce the same thing, an array like

$params['financial_trxn_id'] = ['IN' => [1,2,3]]

What am I missing?

[mlutfy]

I tried testing before/after and it also seems to give the same result.

I like the shorter syntax though, and the correctly-initialized array.

I tested this on PHP 8.2, in case it was a PHP version issue:

$eft = [];
$eft[] = ['financial_trxn_id' => 1];
$eft[] = ['financial_trxn_id' => 2];
$eft[] = ['financial_trxn_id' => 3];

$params = [];
foreach ($eft as $entityFinancialTrxn) {
  $params['financial_trxn_id']['IN'][] = $entityFinancialTrxn['financial_trxn_id'];
}
print_r($params);

$params = [];
$ftIds = array_column($eft, 'financial_trxn_id');
$params['financial_trxn_id'] = ['IN' => $ftIds];
print_r($params);

Output:

Array
(
    [financial_trxn_id] => Array
        (
            [IN] => Array
                (
                    [0] => 1
                    [1] => 2
                    [2] => 3
                )

        )

)
Array
(
    [financial_trxn_id] => Array
        (
            [IN] => Array
                (
                    [0] => 1
                    [1] => 2
                    [2] => 3
                )

        )

)

[colemanw]

I agree Civi code tends to overuse the foreach loop when there are cleverer methods available, I'm not against merging this on those grounds... but what of the bug @MegaphoneJon ?

[mattwire]

Merging since before/after seems to be the same but it's obviously caused something buggy for @MegaphoneJon. Also using array_column is more performant than foreach. @MegaphoneJon be good if you can comment on the bug per @colemanw comment?