dev/core#117 Fix use of each in CRM/Pledge

[seamuslee001]

Overview

Another PHP7.2 foreach fix this is also backed by a unit test its part of api_v3_ContributionPageTest test suite
ping @eileenmcnaughton @monishdeb @mattwire

[eileenmcnaughton]

@seamuslee001 is the unit test actually testing the output of the function? Or is it 'just passing through'? We should update the PR with steps to replicate that the function returns the correct result.

Have you figured out what to do in the UI to call this ?

[seamuslee001]

@eileenmcnaughton its more the passing through https://github.com/civicrm/civicrm-core/blob/master/tests/phpunit/api/v3/ContributionPageTest.php#L1544 i haven't actually tried to replicate in the UI i have only been checking against unit tests.

[seamuslee001]

it also looks like there are other unit tests that might more directly test it

civicrm-core/tests/phpunit/CRM/Pledge/BAO/PledgeTest.php

Line 153 in 0174c6b

public function testGetPledgeStartDate() {

[seamuslee001]

@eileenmcnaughton I have just tried poking about in the UI and can't seem to find anyway of generating it, the code appears to have come about from https://issues.civicrm.org/jira/browse/CRM-18854, I'm hoping that maybe @monishdeb might be able to see something in JMA's history about how to use this code, Tho the fact that a Unit test kicked up the error and the unit test still passes after the code changes does point towards that this code isn't causing harm at least

[eileenmcnaughton]

@seamuslee001 my reading of the pr is that I would expect it to cause harm - because the original structure is expecting an array like

[$field, $value]

& the latter is expecting

[$field => $value]

which is why I was concerned to figure out how to ensure we are not fixing these things blind

[seamuslee001]

@eileenmcnaughton i would disagree, the list($field, $value) is expecting to pull out 2 string variables the key and the value of each element of the array as per the each() function.

Also when you look at this

civicrm-core/tests/phpunit/CRM/Pledge/BAO/PledgeTest.php

Line 154 in 0174c6b

$startDate = json_encode(array('calendar_month' => 6));

the $field is the key of each element of array with the field's value the value of the key pair

civicrm-core/tests/phpunit/CRM/Pledge/BAO/PledgeTest.php

Line 154 in 0174c6b

$startDate = json_encode(array('calendar_month' => 6));

Also i think this gives a good guide to how the current function is outputing https://www.w3schools.com/php/showphp.asp?filename=demo_func_each2 see also http://php.net/manual/en/function.each.php

[eileenmcnaughton]

@seamuslee001 OK cool- I'm not so familiar with the each function - hence my focus on figuring out how to make sure we are actually testing the output of each change, not just that the test suite is loading that line of code.

[seamuslee001]

@eileenmcnaughton sure the form changes i don't know enough about and haven't been able to generate them and they relate to JMA stuff the testGetPledgeStartDate() covers off the getPledgeStartDate change i believe

[eileenmcnaughton]

Ok - well let's let @monishdeb review - @JoeMurray has made it clear this 7.2 review is something he wants @monishdeb to do pro bono work on as he sees it as a priority so @monishdeb should be the first point of review for all the 7.2 stuff - but there is a risk there will be too much for him. In general it is probably quite risky as changing 'each' statements is almost by definition involving 'less trodden' code

[monishdeb]

Fixed the two failures

1. api_v3_ContributionPageTest::testSubmitPledgePaymentPaymentProcessorRecurFuturePayment
   Failure in api call for contribution_page submit: The each() function is deprecated. This message will be suppressed on further calls

2. CRM_Pledge_BAO_PledgeTest::testGetPledgeStartDate
   The each() function is deprecated. This message will be suppressed on further calls

• (r-explain) Pass
• (r-test) Pass
• (r-code) Pass
• (r-doc) Pass
• (r-maint) Pass
• (r-run) Tested on local, works fine.
• (r-user) Pass
• (r-tech) Pass: As per code pov I wish if there was php native function to fetch key and value of an array in a single call like each() but there isn't any. So need to rely on foreach for now, as a possible alternative to avoid foreach iteration, would be:

$value = reset($startDate);
$field = key($startDate);
...

as I found $startDate always got single record, but that's ok as doesn't matter in terms of performance/clarity.