CRM-19594 Wrong Membership Updated

[ginkgomzd]

remove bad assumption of what entity table should be.

---

• CRM-19594: Wrong Membership Updated

[ginkgomzd]

@eileenmcnaughton Do you think you can lend any support to the theory that this code is not needed? I think I have proven it to be harmful, but I don't know if simply removing it is an acceptable solution.

https://issues.civicrm.org/jira/browse/CRM-19594

[monishdeb]

Also it causes three related unit tests to fail https://test.civicrm.org/job/CiviCRM-Core-PR/12492/

[ginkgomzd]

I'm not seeing yet how to make this code smarter. I think calling code needs to be updated to not rely on this default. I did find that this function is called twice but I couldn't see how to change that execution pattern.

[eileenmcnaughton]

From your JIRA comment "HOWEVER, when the LineItem BAO is invoked the first time, the entity_id is that of the contribution"

In other words entity_id is being (incorrectly) conflated with contribution id.

The issue is presumably that because the calling function is not setting entity_id to be the membership id it is incorrectly being set to be the contribution id here

 if (empty($line['entity_id'])) {
          $line['entity_id'] = $entityId;
        }

Is this happening in multiple form flows?

[ginkgomzd]

Whoops, I missed any notification of a reply...
I have been thinking of it as incorrectly setting the entity table, not incorrectly setting the entity_id. I think I've seen that regardless of what is set for the entity_table, it will be overridden based on the presence of a membership_type_id.
I think the failing unit tests depend on this magical default for entity_table, and I'm not convinced they reflect real world dependencies on this magic.

Given that a call that handles the membership does eventually get made, I'm not convinced that setting the correct entity_id is correct. Perhaps it is necessary to call this twice, once for the contribution and once for the membership? I feel out of my depth on this point. I was hoping you would know more about why, if not how, this gets invoked twice.

I think a membership contribution page using price sets is the only form flow that can get us in this situation.

[eileenmcnaughton]

@ginkgomzd I managed to replicate in a test - see #9390 - however I think we really need more tests to check what happens if there is a complex price set with contribution AND membership rows

[eileenmcnaughton]

Closing in favour of #9390