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

Fix cache bypass #18643

Merged
merged 2 commits into from
Oct 2, 2020
Merged

Fix cache bypass #18643

merged 2 commits into from
Oct 2, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Minor performance fix - If the contact is not impacted by any acls then ids will be an empty array. However, currently
an empty array is treated as 'uncached' rather than 'cached by empty' which
would be represented by null

Before

If the contact does not have any applicable acls the cached empty array is ignored and the acls are computed each time

After

Code only does the queries if the cache returns NULL, not an empty array

Technical Details

Comments

@civibot
Copy link

civibot bot commented Sep 30, 2020

(Standard links)

@civibot civibot bot added the master label Sep 30, 2020
@seamuslee001
Copy link
Contributor

Test fails look related @eileenmcnaughton

api_v3_UFFieldTest.testACLPermissionforProfiles
CRM_Group_Page_AjaxTest.testTraditionalACL
CRM_Group_Page_AjaxTest.testTraditionalACLFoundTitle
CRM_Group_Page_AjaxTest.testTraditionalACLDisabled
CRM_Group_Page_AjaxTest.testTraditionalACLDisabledFoundTitle
CRM_Group_Page_AjaxTest.testTraditionalACLEnabled
CRM_Group_Page_AjaxTest.testTraditionalACLAll

@eileenmcnaughton
Copy link
Contributor Author

yeah interesting - will look

If the contact is not impacted by any acls then ids will be an empty array. However, currently
an empty array is treated as 'uncached' rather than 'cached by empty' which
would be represented by null
@eileenmcnaughton
Copy link
Contributor Author

@seamuslee001 it looks like it was a hidden bug in the cachekey

@seamuslee001
Copy link
Contributor

This looks fine merging

@seamuslee001 seamuslee001 merged commit e2c512f into civicrm:master Oct 2, 2020
@seamuslee001 seamuslee001 deleted the cache branch October 2, 2020 00:19
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.

2 participants