CRM-21155 - Hook batchItems does not change the csv export

[ErikHommel]

Change to use results of hook when exporting.

Overview

A brief description of the pull request. Try to keep it non-technical.

Before

The current status. Please provide screenshots or gifs (LICEcap, SilentCast) where appropriate.

After

What has been changed. Please provide screenshots or gifs (LICEcap, SilentCast) where appropriate.

Technical Details

If the PR introduces noteworthy technical changes, please describe them here. Provide code snippets if necessary

Comments

@ErikHommel was too busy drinking warm beer to fill in the pr template
Not anymore, he had been busy for too long drinking warm beer

---

• CRM-21155: Hook batchItems does not change the csv export

[highfalutin]

I'd be happy to try this out. But reading through the code, I'd like to check with @guanhuan first, who added the batch hook a couple of years ago.

The question I have is, why do we have two arrays: financialItems and batchItems? As you discovered, @ErikHommel, only the financialItems array is actually exported.

Every element in the batchItems array is a reference to an element in the financialItems array. The function passes the batchItems array to the batchItems hook.

If, in your hook implementation, you address and modify the elements of the passed items array individually, you are in fact modifying the contents of the financialtems array which will be exported. But if you clear out the items array wholesale, as @ErikHommel does in CRM-21155, only batchItems is changed, and it won't be exported.

I suggest removing the batchItems array from the function and just using financialItems throughout.

[ErikHommel]

@highfalutin did you check with @guanhuan? I suggest we move ahead if he does not agree by a certain date. I agree it looks like we only need one array, but I thought this PR needs to get processed first as it actually makes something work which does not work now.

[guanhuan]

Hi @ErikHommel,

This was long time ago so I cannot remember the exact context. However, looking at the code as well as what @highfalutin mentioned. The change you are suggesting is probably not the key for solving the issue you are facing since the reason for both the hook should work and didn't work in your implementation is "batchItems" are "financialItems" references passed in.

If we were to prettify the code, I am aligned wth the change @highfalutin suggested if it doesn't bring any impact on anything else (doesn't look like so, please verify).

Also to flag up, hook_civicrm_batchItems is a hook we normally use by pairing it up with hook_civicrm_batchQuery. Not sure if this will provide help to your scenario but just think it would be useful to let you know.

[ErikHommel]

I did notice the other hook too @guanhuan. You lost me on the explanation why the change should not be the key, I do not understand you.

I have implemented my fix at the customer and in that installation it does solve the issue. I totally agree on prettifying the code, but that was not the issue I was trying to solve :-)

[guanhuan]

Hi @ErikHommel ,

Sorry if that wasn't very clear.

If you see this line here. "batchItems" array holds reference to "financialItems" array elements. The 7th line of the first code snippet in your ticket cleared those references. Hence none of the further changes you are making to "batchItems" will get taken into considerations by "financialItems" any more.

To address this issue in the hook implementation, you should avoid clearing the references but to amend your desired changes to the existing array elements passed in by reference.

If you wish to stick to your current implementation of the hook while improve core so your implementation will take effect, the change in this PR should take further improvement by using "FinancialItems" throughout. Since instead of making "FinancialItems" redundant to the export result, duplicating "FinancialItems" to "batchItems" seems more redundant at the first place. However, you might need test more in this case.

Hope that explains better.

[guanhuan]

hook_civicrm_batchQuery already gives you great power to amend what financial records you want to include and what information you want to include. hook_civicrm_batchItems is to only allow you to amend how you want to present the information provided by batchQuery.

The intention to how hook_civicrm_batchItems was done is to help avoiding any of the queried financial records being accidentally missed in the export if the implementer's intention is only to change how to present the information that is already included for every financial item row.

hook_civicrm_batchQuery on the other hand is more for people who know what they are doing and knows the consequence of doing so.

[eileenmcnaughton]

@guanhuan - it looks like @ErikHommel has updated this in line with your comments - do you feel it is correct now?

[guanhuan]

Hi @ErikHommel , @eileenmcnaughton ,

Thanks for the update. The code removes the redundancy of passing through array elements as references to another array that will be used by the hook. It looks good to me.

Best

Guanhuan

[eileenmcnaughton]

Merge on pass based on @guanhuan review

[eileenmcnaughton]

test this please