-
-
Notifications
You must be signed in to change notification settings - Fork 827
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
Extract out remaining usage of MembershipPayment #32222
base: master
Are you sure you want to change the base?
Changes from all commits
9eb1ea7
497aa4f
b4a4b8c
4392be3
ef203d5
693f501
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1507,12 +1507,7 @@ protected function postProcessMembership( | |
|
||
if (!empty($membershipContribution)) { | ||
// Next line is probably redundant. Checks prevent it happening twice. | ||
$membershipPaymentParams = [ | ||
'membership_id' => $membership->id, | ||
'membership_type_id' => $membership->membership_type_id, | ||
'contribution_id' => $membershipContribution->id, | ||
]; | ||
civicrm_api3('MembershipPayment', 'create', $membershipPaymentParams); | ||
CRM_Member_BAO_MembershipPayment::legacyMembershipPaymentCreate($membership->id, $membershipContribution->id); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we could check if this does anything & maybe remove it. I'm pretty sure creating the line item does this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, the extracted function is only called in one place. But my thinking is that it was easier to extract everything related to MembershipPayment first and then it should be easier to justify just removing stuff. Eg. if |
||
} | ||
if ($membership) { | ||
CRM_Core_BAO_CustomValueTable::postProcess($this->_params, 'civicrm_membership', $membership->id, 'Membership'); | ||
|
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.
Oh wow - I think we could ditch the fallback & just use the line item - this is a slightly obscure screen & there are enough other places that rely on people having good data re line item entity_id that I don't think this is the place we need to bend over backwards for bad data
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.
Yeah, we probably could. Except that this getFieldValue logic pops up in various places and causes obscure bugs. I've come across sites missing lineitems and sites missing MembershipPayment records and a mixture of both... so I feel like the safer option is to extract out this logic and leave the fallback in initially. We might add some kind of upgrader/check before we can completely remove the MembershipPayment fallback. See #32244 for the extraction of just this function for easier review!