[REF] Simplify membership status date handling

[eileenmcnaughton]

Overview

Reduce use of variable variables

Before

Variable variables not only for each but each part of each date

After

Use date function on the date variable, not aa variable variable

Technical Details

We have a test error coming out of this code. I find it unreadable - I think this would help....

There are some obvious next steps if this is all good

Comments

[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]

I've seen this error popping up on a few PRs:

CRM_Contribute_BAO_ContributionRecurTest::testAutoRenewalWhenOneMemberIsDeceased
Failure in api call for Contribution repeattransaction:  failed to load related objectsOops, it looks like there is no valid membership status corresponding to the membership start and end dates for this membership. Contact the site administrator for assistance.

[mattwire]

@eileenmcnaughton I realise from your comment that the test error is probably the one you mention in the description..

Marking concept approved but would appreciate someone else taking a quick look before (and ideally r-run) before merge.

[demeritcowboy]

I'll give it a go. This looks $$fun() ...

where $notfun = function() { echo 'not fun'; }; $fun = 'notfun';

[demeritcowboy]

General note: it's hard to follow what the nested foreach is doing because it ends up overwriting any previous calculation it did of $startEvent and $endEvent, but that seems to be the algorithm. So it would be good to add a comment in somewhere about what the algorithm is, which seems something like:

Loop through all the "membership status" definitions, ordered by their weight. For each, we loop through all possible variations of the given start, end, and join dates and adjust the starts and ends based on that membership status's rules, where the last computed set of adjusted start and end becomes a candidate. Then we compare that candidate to either "today" or some other given date, and if it falls between the adjusted start and end we have a match and we stop looping through status definitions. Then we call a hook in case that wasn't enough loops.

Beyond that I think this is an ok replacement, because:

• The replacement of 0,0,0 for hour/minute/second is fine.
• The $membershipStatus->{$eve stuff is just a distraction - there's nothing changing there so can ignore.
• Walking thru for one of the dates, let's say startDate:
  • There's an assumption near the top in the previous code that the dates are in ISO format and we can say the intent is to assign e.g. startMonth to the 2-digit month of startDate.
  • So then in the new code startMonth is being replaced with $month = 2-digit month of startDate, but not assuming ISO format. So that seems fine.
  • The rest is just repeat for Year and Day.
  • Then repeat again for endDate and joinDate.
  • There is a difference if e.g. startDate gets set to blank when the original was null. It ends up not mattering though because the inner for loop doesn't bother evaluating anything if it's blank because of its first if. But for the record with the original code startMonth would be null, which then is 0 if it were to be used in mktime(), whereas in the new code date('m', strtotime('')) gives, of course, 12. So before you could get 1999-11-30 (+adjustments) and now you get 1969-12-30 (+adjustments), except as noted this never happens.
    • Since it never happens, I'd suggest moving lines 269-271 down one line so it's inside the first if, since there's no point calculating them when ${$dat . 'Date'} is blank, as per the if. But that's a minor thing.

Asides:

• Lines 222-230 is the paid-by-lines-of-code way of doing $statusDate = date('Ymd');.
• $membershipTypeID is a required param for some reason but only used at the end to pass to the hook.

I did click on some things but mostly ran this by putting in some debug output in the code and calling cv ev on this function with some varied params and looked at the output before and after the patch. Fortunately except for status definitions it's self-contained.

So I'd say this PR is cv ev "$outs = ['Ok to merge', 'Needs work']; $OK = 0; $NOK = 1; $s = (new GuzzleHttp\Client(['base_uri' => 'https://civicrm.org']))->get('/')->getReasonPhrase(); echo $outs[${$s . ($status ?? '')}];"

(Ok to merge)

[eileenmcnaughton]

@demeritcowboy variable variable loops hurt my brain in places where I didn't even know I had brain.

I was thinking about following this up with a next baby step - but maybe you would like to & I can bruise my brain on that - since you seem to have thought through the obvious follow up with looks to be summarised as

Lines 222-230 is the paid-by-lines-of-code way of doing $statusDate = date('Ymd');.

[eileenmcnaughton]

test this please

[mattwire]

Tests passing - merging based on @demeritcowboy review