From f57cb50c96c23077fd567688f1bf527972036a66 Mon Sep 17 00:00:00 2001 From: "Matthew Wire (MJW Consulting)" Date: Fri, 26 Jul 2019 16:32:12 +0100 Subject: [PATCH] Remove more use of ids array on CRM_Member_BAO_Membership::create --- CRM/Batch/Form/Entry.php | 1 + CRM/Member/BAO/Membership.php | 38 ++++++++++++++++++------- CRM/Member/Form/Membership.php | 5 ++-- CRM/Member/Form/MembershipView.php | 3 +- CRM/Member/Import/Parser/Membership.php | 10 +++---- api/v3/Membership.php | 5 ++-- tests/phpunit/api/v3/MembershipTest.php | 1 + 7 files changed, 42 insertions(+), 21 deletions(-) diff --git a/CRM/Batch/Form/Entry.php b/CRM/Batch/Form/Entry.php index 46555a290b..d710f550b4 100644 --- a/CRM/Batch/Form/Entry.php +++ b/CRM/Batch/Form/Entry.php @@ -860,6 +860,7 @@ class CRM_Batch_Form_Entry extends CRM_Core_Form { unset($value['membership_start_date']); unset($value['membership_end_date']); $ids = []; + // @todo stop passing empty $ids $membership = CRM_Member_BAO_Membership::create($value, $ids); } diff --git a/CRM/Member/BAO/Membership.php b/CRM/Member/BAO/Membership.php index e7d3adae81..b59eb1a4cf 100644 --- a/CRM/Member/BAO/Membership.php +++ b/CRM/Member/BAO/Membership.php @@ -325,6 +325,7 @@ class CRM_Member_BAO_Membership extends CRM_Member_DAO_Membership { $transaction = new CRM_Core_Transaction(); + // @todo remove $ids from here - $ids['contribution'] is not used or set by add. $ids['userId'] is used. $ids['membership'] is used if $params['id'] is not set $membership = self::add($params, $ids); if (is_a($membership, 'CRM_Core_Error')) { @@ -339,15 +340,20 @@ class CRM_Member_BAO_Membership extends CRM_Member_DAO_Membership { } $params['membership_id'] = $membership->id; + // @todo further cleanup required to remove use of $ids['contribution'] from here if (isset($ids['membership'])) { $ids['contribution'] = CRM_Core_DAO::getFieldValue('CRM_Member_DAO_MembershipPayment', $membership->id, 'contribution_id', 'membership_id' ); + // @todo this is a temporary step to removing $ids['contribution'] completely + if (empty($params['contribution_id']) && !empty($ids['contribution'])) { + $params['contribution_id'] = $ids['contribution']; + } } - // This code ensures a line item is created but it is recomended you pass in 'skipLineItem' or 'line_item' + // This code ensures a line item is created but it is recommended you pass in 'skipLineItem' or 'line_item' if (empty($params['line_item']) && !empty($params['membership_type_id']) && empty($params['skipLineItem'])) { CRM_Price_BAO_LineItem::getLineItemArray($params, NULL, 'membership', $params['membership_type_id']); } @@ -356,14 +362,14 @@ class CRM_Member_BAO_Membership extends CRM_Member_DAO_Membership { //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); } if (!empty($params['lineItems'])) { $params['line_item'] = $params['lineItems']; } - //do cleanup line items if membership edit the Membership type. + // do cleanup line items if membership edit the Membership type. if (empty($ids['contribution']) && !empty($ids['membership'])) { CRM_Price_BAO_LineItem::deleteLineItems($ids['membership'], 'civicrm_membership'); } @@ -1437,9 +1443,8 @@ WHERE civicrm_membership.contact_id = civicrm_contact.id $relMembership = new CRM_Member_DAO_Membership(); $relMembership->contact_id = $contactId; $relMembership->owner_membership_id = $membership->id; - $relMemIds = array(); if ($relMembership->find(TRUE)) { - $params['id'] = $relMemIds['membership'] = $relMembership->id; + $params['id'] = $relMembership->id; } $params['contact_id'] = $contactId; $params['owner_membership_id'] = $membership->id; @@ -1481,15 +1486,18 @@ WHERE civicrm_membership.contact_id = civicrm_contact.id // CRM-20966: Do not create membership_payment record for inherited membership. unset($params['relate_contribution_id']); + $ids = []; if (($params['status_id'] == $deceasedStatusId) || ($params['status_id'] == $expiredStatusId)) { // related membership is not active so does not count towards maximum - CRM_Member_BAO_Membership::create($params, $relMemIds); + // @todo stop passing empty $ids + CRM_Member_BAO_Membership::create($params, $ids); } else { // related membership already exists, so this is just an update if (isset($params['id'])) { if ($numRelatedAvailable > 0) { - CRM_Member_BAO_Membership::create($params, $relMemIds); + // @todo stop passing empty $ids + CRM_Member_BAO_Membership::create($params, $ids); $numRelatedAvailable--; } else { @@ -1508,7 +1516,8 @@ WHERE civicrm_membership.contact_id = civicrm_contact.id if ($numRelatedAvailable <= 0) { break; } - CRM_Member_BAO_Membership::create($params, $relMemIds); + // @todo stop passing $ids - at this point it may be set by reference from earlier calls to CRM_Member_BAO_Membership::create + CRM_Member_BAO_Membership::create($params, $ids); $numRelatedAvailable--; } } @@ -1864,7 +1873,7 @@ INNER JOIN civicrm_contact contact ON ( contact.id = membership.contact_id AND if ($contributionRecurID) { $memParams['contribution_recur_id'] = $contributionRecurID; } - + // @todo stop passing $ids - it is empty $membership = self::create($memParams, $ids, FALSE); return array($membership, $renewalMode, $dates); } @@ -2038,6 +2047,7 @@ INNER JOIN civicrm_contact contact ON ( contact.id = membership.contact_id AND // Load all line items & process all in membership. Don't do in contribution. // Relevant tests in api_v3_ContributionPageTest. $memParams['line_item'] = $lineItems; + // @todo stop passing $ids (membership and userId may be set by this point) $membership = self::create($memParams, $ids, FALSE); // not sure why this statement is here, seems quite odd :( - Lobo: 12/26/2010 @@ -2414,11 +2424,14 @@ WHERE civicrm_membership.is_test = 0 * @param array $params * Array of submitted params. * @param array $ids - * (param in process of being removed - try to use params) array of ids. + * (@deprecated) array of ids. * * @return CRM_Contribute_BAO_Contribution */ public static function recordMembershipContribution(&$params, $ids = array()) { + if (!empty($ids)) { + CRM_Core_Error::deprecatedFunctionWarning('no $ids array'); + } $membershipId = $params['membership_id']; $contributionParams = array(); $config = CRM_Core_Config::singleton(); @@ -2454,6 +2467,9 @@ WHERE civicrm_membership.is_test = 0 $contributionParams[$f] = CRM_Utils_Array::value($f, $params); } + if (!empty($params['contribution_id'])) { + $contributionParams['id'] = $params['contribution_id']; + } // make entry in batch entity batch table if (!empty($params['batch_id'])) { $contributionParams['batch_id'] = $params['batch_id']; @@ -2470,7 +2486,7 @@ WHERE civicrm_membership.is_test = 0 $contributionParams['line_item'] = CRM_Utils_Array::value('lineItems', $params, NULL); } - $contribution = CRM_Contribute_BAO_Contribution::create($contributionParams, $ids); + $contribution = CRM_Contribute_BAO_Contribution::create($contributionParams); //CRM-13981, create new soft-credit record as to record payment from different person for this membership if (!empty($contributionSoftParams)) { diff --git a/CRM/Member/Form/Membership.php b/CRM/Member/Form/Membership.php index cf5bf72ffc..3169844fdc 100644 --- a/CRM/Member/Form/Membership.php +++ b/CRM/Member/Form/Membership.php @@ -1418,7 +1418,7 @@ class CRM_Member_Form_Membership extends CRM_Member_Form { $paymentParams['contributionTypeID'] = $contribution->financial_type_id; $paymentParams['contributionPageID'] = $contribution->contribution_page_id; $paymentParams['contributionRecurID'] = $contribution->contribution_recur_id; - $ids['contribution'] = $contribution->id; + $params['contribution_id'] = $paymentParams['contributionID']; $params['contribution_recur_id'] = $paymentParams['contributionRecurID']; } $paymentStatus = NULL; @@ -1522,6 +1522,7 @@ class CRM_Member_Form_Membership extends CRM_Member_Form { unset($membershipParams['lineItems']); } $membershipParams['payment_instrument_id'] = $paymentInstrumentID; + // @todo stop passing $ids (membership and userId only are set above) $membership = CRM_Member_BAO_Membership::create($membershipParams, $ids); $params['contribution'] = CRM_Utils_Array::value('contribution', $membershipParams); unset($params['lineItems']); @@ -1618,7 +1619,7 @@ class CRM_Member_Form_Membership extends CRM_Member_Form { if (!empty($softParams)) { $membershipParams['soft_credit'] = $softParams; } - + // @todo stop passing $ids (membership and userId only are set above) $membership = CRM_Member_BAO_Membership::create($membershipParams, $ids); $params['contribution'] = CRM_Utils_Array::value('contribution', $membershipParams); unset($params['lineItems']); diff --git a/CRM/Member/Form/MembershipView.php b/CRM/Member/Form/MembershipView.php index 05520fb1d3..ceca87f219 100644 --- a/CRM/Member/Form/MembershipView.php +++ b/CRM/Member/Form/MembershipView.php @@ -121,7 +121,6 @@ class CRM_Member_Form_MembershipView extends CRM_Core_Form { break; case 'create': - $ids = []; $params = [ 'contact_id' => CRM_Utils_Request::retrieve('rid', 'Positive', $this), 'membership_type_id' => $owner['membership_type_id'], @@ -136,6 +135,8 @@ class CRM_Member_Form_MembershipView extends CRM_Core_Form { 'skipStatusCal' => TRUE, 'createActivity' => TRUE, ]; + // @todo stop passing $ids here (we are only doing so because of passbyreference) + $ids = []; CRM_Member_BAO_Membership::create($params, $ids); $relatedDisplayName = CRM_Contact_BAO_Contact::displayName($params['contact_id']); CRM_Core_Session::setStatus(ts('Related membership for %1 has been created.', [1 => $relatedDisplayName]), diff --git a/CRM/Member/Import/Parser/Membership.php b/CRM/Member/Import/Parser/Membership.php index 66147109a9..2501ba9d01 100644 --- a/CRM/Member/Import/Parser/Membership.php +++ b/CRM/Member/Import/Parser/Membership.php @@ -395,15 +395,15 @@ class CRM_Member_Import_Parser_Membership extends CRM_Member_Import_Parser { 'Membership' ); if ($dao->find(TRUE)) { - $ids = [ - 'membership' => $formatValues['membership_id'], - 'userId' => $session->get('userID'), - ]; - if (empty($params['line_item']) && !empty($formatted['membership_type_id'])) { CRM_Price_BAO_LineItem::getLineItemArray($formatted, NULL, 'membership', $formatted['membership_type_id']); } + // @todo stop passing $ids array (and put details in $formatted if required) + $ids = [ + 'membership' => $formatValues['membership_id'], + 'userId' => $session->get('userID'), + ]; $newMembership = CRM_Member_BAO_Membership::create($formatted, $ids, TRUE); if (civicrm_error($newMembership)) { array_unshift($values, $newMembership['is_error'] . ' for Membership ID ' . $formatValues['membership_id'] . '. Row was skipped.'); diff --git a/api/v3/Membership.php b/api/v3/Membership.php index 5b06b0820f..42f5249895 100644 --- a/api/v3/Membership.php +++ b/api/v3/Membership.php @@ -134,10 +134,10 @@ function civicrm_api3_membership_create($params) { } // Fixme: This code belongs in the BAO + $ids = []; if (empty($params['id'])) { $params['action'] = CRM_Core_Action::ADD; // we need user id during add mode - $ids = []; if (!empty($params['contact_id'])) { $ids['userId'] = $params['contact_id']; } @@ -145,10 +145,11 @@ function civicrm_api3_membership_create($params) { else { // edit mode $params['action'] = CRM_Core_Action::UPDATE; - // $ids['membership'] is required in CRM_Price_BAO_LineItem::processPriceSet + // @todo remove $ids['membership'] is required in CRM_Price_BAO_LineItem::processPriceSet $ids['membership'] = $params['id']; } + // @todo stop passing $ids (membership and userId may be set above) $membershipBAO = CRM_Member_BAO_Membership::create($params, $ids, TRUE); if (array_key_exists('is_error', $membershipBAO)) { diff --git a/tests/phpunit/api/v3/MembershipTest.php b/tests/phpunit/api/v3/MembershipTest.php index 8074902f2a..07ea117346 100644 --- a/tests/phpunit/api/v3/MembershipTest.php +++ b/tests/phpunit/api/v3/MembershipTest.php @@ -1206,6 +1206,7 @@ class api_v3_MembershipTest extends CiviUnitTestCase { 'skipStatusCal' => 1, 'is_for_organization' => 1, ]; + // @todo stop passing empty $ids $ids = []; $membership = CRM_Member_BAO_Membership::create($params, $ids); -- 2.25.1