-
-
Notifications
You must be signed in to change notification settings - Fork 814
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CRM-21229 improve static var management in group permission clause #11050
Conversation
CRM/Contact/BAO/Group.php
Outdated
static $clause = 1; | ||
static $retrieved = FALSE; | ||
if (!$retrieved || $force) { | ||
if ($force) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm - do you think that's more readable? I could squash the first part - but really we want to be killing the whole $force
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I was trying to keep scope small & not have to grep everywhere that might use the $force param
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep - but it could be in extension tests too - I feel like that is how this came up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
7631db0
to
eeb17fa
Compare
eeb17fa
to
bc95ea0
Compare
I updated this in response to the feedback - looking to get it merged now - it's a simple fix to improve caching |
Note the key goal of this is to prevent unit test pain |
CRM-21229 improve static var management in group permission clause
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