Don't use random parameters to detect if we should create MembershipPayment

[mattwire]

Overview

Follow on from #14886

There are 3 places where a MembershipPayment may be created. This bit uses 2 unrelated parameters to check if we've already hit one of those other places. Instead, just check directly for a MembershipPayment using the API and don't rely on parameters which come from elsewhere and may change.

Before

Uses random parameters to decide whether a MembershipPayment should be created.

After

Use a proper check to decide whether a MembershipPayment should be created.

Technical Details

Described above.

Comments

@eileenmcnaughton Follow on from #14886 which should be merged first

[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).

[eileenmcnaughton]

@mattwire needs a rebase

[mattwire]

@eileenmcnaughton Now rebased :-)

[eileenmcnaughton]

@MegaphoneJon I wonder if this is a piece of your puzzle on paypal

[MegaphoneJon]

Yes, this may have been the underlying bug that led to the issue.

[mattwire]

@MegaphoneJon Awesome if it is! I just came across it and thought, hmm that doesn't look very reliable..

[eileenmcnaughton]

I guess a contribution could relate to more than one membership - so this might fail to create some

[eileenmcnaughton]

@mattwire @MegaphoneJon I think the IF should go altogether - in fact - perhaps by accident -- in current master $ids['contribution'] is ALWAYS empty & also the membershipPayment.create code handles being called twice for the same pair

[mattwire]

@eileenmcnaughton When we reach this point we are working with a single specific membership ID, the entire function would be called once for each membership so contributions linked to multiple memberships would be ok.
If we remove the IF statement then we call hooks multiple times for membershippayment for no good reason, as if it already exists nothing has changed. So I'd prefer that the code checks before calling create in this instance.