-
-
Notifications
You must be signed in to change notification settings - Fork 825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dev/core#117 Replace use of Deprecated function each in CRM/Member/BA… #12155
Conversation
CRM/Member/BAO/Membership.php
Outdated
if ($available <= 0) { | ||
break; | ||
} | ||
CRM_Member_BAO_Membership::create($params, $relMemIds); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when i was testhing this there was no value key in the $params which was causing other test failures
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, so this bit of code hasn't worked for a long time then?
@seamuslee001 It would be nice to rename the $available
variable to something with a bit more meaning as part of this PR - eg $numRelatedAvailable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can also drop the reset($queue);
on line 1517 as it's not required with foreach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is my guess but i'm not exactly sure as only looking at it in the prism of unit tests
…O/Membership.php to assit in supporting php7.2 Rename Variable to be more accurate
81abe04
to
e48744e
Compare
@mattwire @monishdeb changes have been made as per Matt's comment |
while (($available > 0) && ($params = each($queue))) { | ||
CRM_Member_BAO_Membership::create($params['value'], $relMemIds); | ||
$available--; | ||
foreach ($queue as $params) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eileenmcnaughton @monishdeb @mattwire this one is a bit more of a change however to check this i put in a print_r($queue) in L1517 locally previously this is its output
Array
(
[0] => Array
(
[is_test] => 0
[version] => 3
[debug] => 1
[join_date] => 20070121
[start_date] => 2018-05-21
[end_date] => 2019-05-21
[source] => Test suite
[membership_type_id] => 40
[contact_id] => 67
[custom] => Array
(
)
[action] => 1
[status_id] => 26
[membership_id] => 1
[skipLineItem] => 1
[owner_membership_id] => 1
[skipStatusCal] => 1
[createActivity] => 1
)
)
After the patch it looks like
Array
(
[0] => Array
(
[is_test] => 0
[version] => 3
[debug] => 1
[join_date] => 20070121
[start_date] => 2018-05-21
[end_date] => 2019-05-21
[source] => Test suite
[membership_type_id] => 40
[contact_id] => 67
[custom] => Array
(
)
[action] => 1
[status_id] => 26
[membership_id] => 1
[skipLineItem] => 1
[owner_membership_id] => 1
[skipStatusCal] => 1
[createActivity] => 1
)
)
Makes sense to me. |
…O/Membership.php to assit in supporting php7.2
Overview
When running the unit tests in php7.2 this comes up as an error due to the use of the deprecated function
Before
Deprecated function is used
After
No deprecated function is used
Technical Details
This one is a little complex as there is a while + an each, i think i have replicated the purpose of this successfully and tests are passing locally
ping @eileenmcnaughton @monishdeb