Stop passing $ids to membership::create

[mattwire]

Overview

We don't need to pass $ids to membership::create on membership form because membership ID is already set in $membershipParams.

Before

$ids passed

After

$ids not passed

Technical Details

Comments

@eileenmcnaughton this probably makes most sense to you...

[civibot]

(Standard links)

• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[colemanw]

@mattwire Jenkins says "failed to apply patches".
Solution is usually to git pull --rebase origin master&&git push -f

[mattwire]

@mattwire Jenkins says "failed to apply patches". Solution is usually to git pull --rebase origin master&&git push -f

Thanks,. fixed

[eileenmcnaughton]

Note that this actually results in a behaviour change - see

civicrm-core/CRM/Member/BAO/Membership.php

Lines 344 to 354 in f5fef76

	if (isset($ids['membership'])) {
	$contributionID = 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($contributionID)) {
	$params['contribution_id'] = $contributionID;
	}
	}

I can't recall the impact - I did remove it from a bunch of places in the past but there was something scary about this one

[mattwire]

Note that this actually results in a behaviour change - see

@eileenmcnaughton Does it actually change anything though? If you look at the calling code in CRM_Member_Form_Membership you can see that $params['contribution_id'] is always set if there is a contribution which means that the code in the BAO would never update anything anyway.

[eileenmcnaughton]

I put this up to see ... #26401

[eileenmcnaughton]

SO it seems from my test it DOES hit that line - I'm just not quite sure what happens as a result

Exception : CRM_Core_Exception: "should be unreachable Caller: ::civicrm_api3_membership_create"
#0 /srv/civi-sites/dmaster/web/sites/all/modules/civicrm/CRM/Member/Form/Membership.php(1184): civicrm_api3("Payment", "create", (Array:9))
#1 /srv/civi-sites/dmaster/web/sites/all/modules/civicrm/CRM/Member/Form/Membership.php(874): CRM_Member_Form_Membership->submit()
#2 /srv/civi-sites/dmaster/web/sites/all/modules/civicrm/tests/phpunit/CRM/Member/Form/MembershipTest.php(538): CRM_Member_Form_Membership->postProcess()
#3 /srv/civi-sites/wmff/vendor/phpunit/phpunit/src/Framework/TestCase.php(1156): CRM_Member_Form_MembershipTest->testSubmit(".")
#4 /srv/civi-sites/dmaster/web/sites/all/modules/civicrm/tests/phpunit/CiviTest/CiviUnitTestCase.php(230): PHPUnit\Framework\TestCase->runTest()
#5 /srv/civi-sites/wmff/vendor/phpunit/phpunit/src/Framework/TestCase.php(845): CiviUnitTestCase->runTest()

[mattwire]

@eileenmcnaughton Your test is checking if any code passes in $ids. There are still 4 or 5 code paths that do pass in $ids['membership'] so the failures are expected. I'm only removing $ids from a single codepath/caller where we can see by code inspection that $params['contribution_id'] will be set when it exists.

[eileenmcnaughton]

@mattwire where is it being set? I see it in the payment processor section but not sure where it is being set leading up to this line

(of course I don't know if it is really a good thing for it to be set but that is a different question)

[mattwire]

@eileenmcnaughton It's a bit confusing and the submit function probably should be split into two because there's a big if here:

civicrm-core/CRM/Member/Form/Membership.php

Line 1086 in 9afc564

if ($this->_mode) {

    if ($this->_mode) {

which says "if mode is set" (could be live or test I think) follow the "has a payment associated" flow.
Then contribution ID gets set here:

civicrm-core/CRM/Member/Form/Membership.php

Line 1164 in 9afc564

$params['contribution_id'] = $paymentParams['contributionID'];

If you follow the other "flow" ie.

civicrm-core/CRM/Member/Form/Membership.php

Line 1260 in 9afc564

else {

else

then it assumes there is no contribution associated. If a contribution is to be created it will only exist after the call to CRM_Member_BAO_Membership::create() when CRM_Member_BAO_Membership::recordMembershipContribution() is called.

[eileenmcnaughton]

OK @mattwire I think this is something of an understatement "It's a bit confusing "

But I can follow your argument now and I can't see another path