CRM-21816 Fix Relative dates in search bug

[aydun]

Overview

Fix a bug in Advanced Search whereby using a relative date search condition (eg 'Next 60 days') could result in some other search conditions being ignored.

Before

You need to do a little setup work to see this:

• setup a membership type with related contacts
• setup 2 contacts with the specified relationship
• create a membership expiring within 60 days

You should have 2 related contacts with a membership expiring within 60 days. One is primary, one is not.

• In Advanced Search, open Membership, search for 'Primary Members' - should only show the primary member.
• Now search for 'Primary Members' and set the Membership End Date to 'Next 60 days'. If this shows you both contacts it demonstrates the bug - the 'primary member' condition is being ignored.

Note that this appears to be related to the version of PHP so you may not see it. The problem was observed with PHP 5.6.32

After

The search should work correctly.

Technical Details

Using unset() within a foreach loop can cause problems and should be avoided. See https://wiki.php.net/rfc/php7_foreach#introduction for a simple example.

Comments

The problem is not limited to memberships or to 'primary member'. It it potentially triggered by any of the relative date fields. Which conditions are skipped depends on the ordering within $formValues

---

• CRM-21816: Relative dates in searches cause some other conditions to be ignored

[eileenmcnaughton]

I think some of the tests in CRM_Contact_BAO_QueryTest would provide a reasonable starting point for a test for this....

[jitendrapurohit]

@aydun I just picked this for QA and wasn't able to replicate this bug on my setup or dmaster. The description seems like we can create an inherited membership and use Advance search to re-create this issue. Steps I followed -

• Create an inherited membership which expire next month.
• Navigate to Advanced Search -> Select end date as "Next 60 days" -> Search
• Both contacts listed (Correct).
• Select Primary Member? to yes.
• The secondary contact was removed from the result as expected.

Also, I see the unit test added here passes successfully without the "fix" in place. Maybe, this is already handled in the latest version? If yes, can you pls check and update? Thanks.

[aydun]

@jitendrapurohit I've just tested this again on the system that originally encountered the problem but is now upgraded to 5.1.2. Under php 5.6 it shows the same issues with the Primary Members Only clause being lost.

I have now upgraded that system to php 7.1 and Advanced Search shows the correct results.

As the example in https://wiki.php.net/rfc/php7_foreach#introduction for unset shows, apparently irrelevant code differences can effect the results on php < 7 so there may be more factors / code paths that need to be isolated to determine exactly when the problem manifests in Advanced Search.

Since the 'unset in foreach' problem is resolved in php 7, the simple solution to anyone encountering this is to upgrade php. But since we still support lesser versions of php, maybe this should be included anyway since it avoids a recognised problem in php.

[eileenmcnaughton]

@aydun I just tried this under 5.6.30 & did not hit the issue. My testing showed it working with and without this patch

However, on looking at the test & reading the code I think that this is worth merging - it DOES move the unsetting out of a foreach which seems logical and I agree with the removing of $val

FWIW - I would have preferred to see the chunk of code extracted out into a function rather than moved.

[eileenmcnaughton]

Also - the unit test is compelling :-)

[aydun]

Thanks @eileenmcnaughton & @jitendrapurohit for reviewing and merging.