Further deprecate use of $ids array in membership functions #14887
Conversation
|
(Standard links)
|
| @@ -356,14 +349,22 @@ public static function create(&$params, &$ids = [], $skipRedirect = FALSE) { | |||
| //record contribution for this membership | |||
| if (!empty($params['contribution_status_id']) && empty($params['relate_contribution_id'])) { | |||
| $memInfo = array_merge($params, array('membership_id' => $membership->id)); | |||
| $params['contribution'] = self::recordMembershipContribution($memInfo, $ids); | |||
| $params['contribution'] = self::recordMembershipContribution($memInfo); | |||
There was a problem hiding this comment.
It seems that we are no longer passing in $ids['contribution'] & nor are we passing in $memInfo['contribution_id'] so we might need to be loading & passing that in instead?
There was a problem hiding this comment.
$ids['contribution'] can only be set if we've already called CRM_Member_BAO_Membership::create() which we haven't, as we're calling this from create()
Arguably we don't need the $params['contribution_id'] check in recordMembershipContribution() as I think it will always be empty, but it seemed sensible given the need for further refactoring in this area.
There was a problem hiding this comment.
I'm confused - if $ids['membership'] is set & it's an update the $ids['membership'] would be set so it would load $ids['contribution'] in the rows above (before your removal)
There was a problem hiding this comment.
:-( Looks like I am too... Have undone a bit of the refactor so we set $params['contribution_id'] before calling self::recordMembershipContribution. I'm still pretty sure that neither $ids['contribution' or $params['contribution_id'] will be set at this point but the code is now less "risky" for a first round.
|
I think we do have pretty good test cover here |
mattwire
force-pushed
the
membership_deprecateids
branch
from
0766c5f to
a01c34d
Compare
|
@eileenmcnaughton I've reviewed (and resolved) your comments. I've added a load more comments reflecting the work I did to track where |
|
@eileenmcnaughton Try again :-) |
|
changes look fine to me now @mattwire can you squish your commits together please |
mattwire
force-pushed
the
membership_deprecateids
branch
from
113706d to
f57cb50
Compare
|
@seamuslee001 @eileenmcnaughton Squashed! |
|
unrelated fail |
Overview
Incremental progress towards removing deprecated code.
Before
$idspassed in more places.After
$idspassed in less places.Technical Details
Comments
@eileenmcnaughton per similar work you've done