Fix for ongoing issues with static upsetting the apple cart

[eileenmcnaughton]

Overview

This is an alternate to #18218

Before

Under various edge cases - including the one in the test & in #18218 inherited memberships are not created

After

Memberships created more reliably

Technical Details

This removes a static variable which was intended to prevent a loop in https://issues.civicrm.org/jira/browse/CRM-4213

The static has caused various ongoing bugs - including https://issues.civicrm.org/jira/browse/CRM-19735 which got past it by flushing the static. Fundamentally the static is not reliable enough so I've switched to looking up & checking existing memberships.

I think it might mean more queries but they shouldn't be expensive queries & the
code is just too hard-going to skip straight from hacked but still intermittently buggy to 'good' (I won't pretend perfect was the enemy of good here- good was the enemy of 'not truly awful')

Comments

CRM-17723 was also a bug this static was interacting with

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[mattwire]

@eileenmcnaughton Checkstyle and conflict

[eileenmcnaughton]

Ah yes - this conflicted with the other

[eileenmcnaughton]

@agh1 or @MegaphoneJon or @agileware-pengyi is this something one of you would be able to review?

[MegaphoneJon]

I can have someone start reviewing today or the day following.

[eileenmcnaughton]

Thanks @MegaphoneJon

[eileenmcnaughton]

Hmm - I'd better check those test fails!

[eileenmcnaughton]

OK - test fails required me to remove old hacks....

[lalgwebdev]

@eileenmcnaughton I can confirm that this fixes the problem described in #18218.

However there is now a different fault. When the status of the primary Membership is changed to Expired from e.g. Current or Grace, this does not propagate to the related Membership(s). Other changes I have tried do propagate. I have seen this when changing the end date manually - I have not tried when running the Process Memberships scheduled job.

[eileenmcnaughton]

@lalgwebdev thanks for checking - I think that is the issue the remaining failing test might be picked up too.

You are testing through back office membership edit form?

[eileenmcnaughton]

Failing job api_v3_JobTest.testProcessMembershipUpdateStatus

[eileenmcnaughton]

I've fixed the test - I think this will fix the front end issue too...

[lalgwebdev]

@eileenmcnaughton Yes - this is now working when I test it (through the administrator's edit membership form).

[MegaphoneJon]

@eileenmcnaughton Test fails relate.

@lalgwebdev It looks like you're taking on review/testing, is that correct?

[eileenmcnaughton]

test this please

[eileenmcnaughton]

@lalgwebdev I made another change for the test fail - can you double check again?

[lalgwebdev]

@eileenmcnaughton Yes, this works OK.

[MegaphoneJon]

Ah, OK, I get it now. I thought there was some unnecessary recursion, and I didn't realize that createRelatedMemberships() is called from within create(), but now I see it all. This makes sense, I think it's good to merge.