CRM-21229 improve static var management in group permission clause

[eileenmcnaughton]

Overview

Improve management of static vars

Before

Static var in CRM_Contact_BAO_Group::getPermissionClause persists over test runs

After

Static var resets

Technical Details

@jmcclelland this is the way we are eliminating those static var issues

---

• CRM-21229: Manage Group page is slow if you have smart groups

[mickadoo]

Just an idea, I wonder could you use

if ($force || !isset(Civi::$statics[__CLASS__]['permission_clause'])) {
  unset(Civi::$statics[__CLASS__]['permission_clause']);
  // fetch clause
}

Seems to be no error for unsetting a non-existing variable.

[eileenmcnaughton]

Hmm - do you think that's more readable? I could squash the first part - but really we want to be killing the whole $force

[mickadoo]

If you'd like to remove $force why not do it in this PR? I don't know the code well enough to see why it should be removed.

Re: readability, my idea was just to reduce two separate conditionals into one, but it's not really such an important change

[eileenmcnaughton]

I think I was trying to keep scope small & not have to grep everywhere that might use the $force param

[monishdeb]

FWIW in entire codebase $force = TRUE is only used in tests/

$ grep -irn "getPermissionClause(" CRM tests
CRM/Contact/BAO/Group.php:611:  public static function getPermissionClause($force = FALSE) {
CRM/Contact/BAO/Group.php:1271:    $clauses[] = self::getPermissionClause();
tests//phpunit/CiviTest/CiviUnitTestCase.php:3065:      CRM_Contact_BAO_Group::getPermissionClause(TRUE);
tests//phpunit/CiviTest/CiviUnitTestCase.php:3539:    CRM_Contact_BAO_Group::getPermissionClause(TRUE);
tests//phpunit/CRM/Group/Page/AjaxTest.php:32:    CRM_Contact_BAO_Group::getPermissionClause($force);
tests//phpunit/CRM/Group/Page/AjaxTest.php:82:    CRM_Contact_BAO_Group::getPermissionClause(TRUE);
tests//phpunit/CRM/Group/Page/AjaxTest.php:256:    $permissionClause = CRM_Contact_BAO_Group::getPermissionClause(TRUE);

[eileenmcnaughton]

yep - but it could be in extension tests too - I feel like that is how this came up

[mickadoo]

Well I don't want to be a blocker on this. My comment was more about merging two conditionals, but it's fine as it is. I don't know enough about usage of $force but if you do want to get rid of it you could put in a trigger_error call here if $force = TRUE and schedule a version for it to be removed. Again, that's just a suggestion, I don't have any issue with this change that I think need to be changed

[eileenmcnaughton]

OK - taking a look at old PRs of mine that are stalled - I've pulled out $force as a param & removed it from the tests. It is only being called from the tests in order to flush the static & we can do that in a different way if the still fail (which they likely won't if we are using a Civi\Static not an in-function static

[eileenmcnaughton]

I updated this in response to the feedback - looking to get it merged now - it's a simple fix to improve caching

[eileenmcnaughton]

Note the key goal of this is to prevent unit test pain

[colemanw]

• (r-jira) Pass
• (r-test) Pass
• (r-code) Pass
• (r-doc) Pass
• (r-maint) Pass
• (r-run) Skip: this only affects unit tests
• (r-user) N/A
• (r-tech) Pass: Improves test maintainability.