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

Remove wasteful double-caching of settings metadata #14259

Merged
merged 1 commit into from
May 19, 2019

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented May 16, 2019

Overview

Improves efficiency by removing redundant caching and repeated calls to the same hook.

Before

Settings metadata was being cached once per domain and one extra time for good measure.
hook_civicrm_alterSettingsMetaData() was invoked even when fetching cached data, even though the hook-altered data was stored in the cache.

After

Settings metadata cached once per domain.
hook_civicrm_alterSettingsMetaData() is invoked only once per domain; as its results were being cached anyway I don't see the need to invoke it repeatedly.

Technical details

Instead of firing hook_civicrm_alterSettingsMetaData() once and then caching the results, the hook was fired once, cached, and then fired again every time metadata was loaded (potentially hundreds of times per request). In addition to being inefficient this could cause weird bugs since the data already altered by hook was then being passed into the hook to be altered again; depending on how the hook implementation was written this could give unexpected results.

@civibot
Copy link

civibot bot commented May 16, 2019

(Standard links)

Settings metadata was being cached once per domain and one extra time for good measure.
hook_civicrm_alterSettingsMetaData() was invoked even when fetching cached data,
even though the hook-altered data was stored in the cache.

Now that the hook is invoked only once and the results cached, tests have been tweaked
to cache after setting the hook instead of before.
@colemanw
Copy link
Member Author

test this please

@colemanw
Copy link
Member Author

colemanw commented May 17, 2019

@eileenmcnaughton this is an initial cleanup toward getting the api3 Settings tests working with api4 so hopefully we can get this merged so I can keep on toward that goal.
Once this is merged I've got another REF to submit to get the 'getoptions' logic out of api3 and into core.

@eileenmcnaughton
Copy link
Contributor

Looks good & we have heavy testing so I think this should be pretty safe

@eileenmcnaughton eileenmcnaughton merged commit 91bf427 into civicrm:master May 19, 2019
@eileenmcnaughton eileenmcnaughton deleted the cacheOptions branch May 19, 2019 21:32
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