Skip to content
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

[REF] Remove unused params from function signature for getACLs #16175

Merged
merged 1 commit into from
Jan 2, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jan 1, 2020

Overview

Improves code readability by removing unnecessary parameters

Before

public static function getACLs($contact_id = NULL, $group_id = NULL, $aclRoles = FALSE)
  • this is only called once with the values for group_id being NULL & aclRoles being TRUE

After

public static function getACLs($contact_id = NULL) {

Technical Details

It turns out this function is only called once & the second 2 parameters are hard-coded so we don't need them

Comments

This also makes the groupID unnecessary for getACLRoles but will wait on #16174 to fix.

It turns out this function is only called once & the second 2 acls are hard-coded so we don't need them
@civibot
Copy link

civibot bot commented Jan 1, 2020

(Standard links)

@civibot civibot bot added the master label Jan 1, 2020
@seamuslee001
Copy link
Contributor

Change looks good i confirmed by grep that this was the only place it was being called from merging

@seamuslee001 seamuslee001 merged commit 730f41f into civicrm:master Jan 2, 2020
@seamuslee001 seamuslee001 deleted the get_acls branch January 2, 2020 00:10
@totten
Copy link
Member

totten commented Jan 2, 2020

I also did a read-through and the refactoring does look to be equivalent to inlining the typical inputs.

@eileenmcnaughton
Copy link
Contributor Author

Yeah - there is some bad stuff in the ACL code (ie bad join on option group ) that contributes to the deadlocks but the code is so unreadable it has survived. I have been thinking about the caching of metadata & apiv4 though. We are increasingly caching some metadata entities like membershipType - but I have been wondering if that could also cover option values & if apiv4 could use the cached data where available or appropriate (rather than create constructs like the getMembershipType function)

eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Jan 2, 2020
Following on from civicrm#16175 we now know that group_id is not
passed into this function as the only place that calls this no longer passes group id, eliminate.
@eileenmcnaughton
Copy link
Contributor Author

Follow up to remove from the downstream fn #16180

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants