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

(dev/core#174) systemStatusCheckResult - Migrate from settings to cache #12336

Merged
merged 2 commits into from
Jun 19, 2018

Conversation

totten
Copy link
Member

@totten totten commented Jun 18, 2018

Overview

This is fundamentally an ephemeral value, and it's apt to being recalculated at arbitrary times. Storing this in the settings table limits the efficacy of read-only database.

See also: https://lab.civicrm.org/dev/core/issues/174

Before

  • The footer of every admin page shows a pre-computed general status. This pre-computed value is periodically updated. The value is stored and read using Civi::settings().

After

  • Same as before, but hte value is stored and read using Civi::cache('checks').

Technical Details

  • The checks cache may be stored in memory-based service, or in a civicrm_cache SQL group, or in a static array. (It uses the best-available storage.)
  • You may concerned about how the lifecycle of this cache compares to the old settings-based lifecycle. In the settings-based lifecycle, the value is separate and immune from normal cache-flushing. Do we get the same immunity when it's a cache? Yes and no. (For simplicity, I'm going to assume a related PR (dev/core#174) CRM_Utils_Cache - Always use a prefix. Standardize delimiter #12331 is merged.)
    • If you have a memory-backed cache, then it's separate by virtue of using different prefixes.
    • If you have a SQL-backed cache, then it can be cleared... but only when one of the following is triggered:
      • General system flush (System.flush aka cv flush aka rebuildMenuAndCaches -- which is done manually or as part of a system-lifecycle task, like installing new extensions)
      • CRM_Core_Config::cleanupCaches() (which can be triggered by the above -- or by major DB upgrade)
      • CRM_Core_Config::clearDBCache() (which can be triggered by the above -- or by Job.cleanup dbCache=1, CRM/Utils/Migrate/Import*, or the GUI page "Settings - Cleanup Caches and Update Paths", or any page based on CRM_Admin_Form_Setting)

Most of those seem like reasonable (and infrequent) occasions to clear it. I suppose CRM_Admin_Form_Setting is more debatable:

  • Usually, after editing a setting, the status-check results don't change. During the early days of setting up a new system, you may edit settings frequently.
  • Sometimes, after editing a setting, the status-check results do change.

If the last one is worth arguing over, then I assume someone will argue over it. ;)

totten added 2 commits June 18, 2018 16:28
This is fundamentally an ephemeral value. Storing this in the settings
table prevents one from accessing the system with a read-only database.
@civibot
Copy link

civibot bot commented Jun 18, 2018

(Standard links)

'community_messages' => 'community_messages',
'checks' => 'checks',
);
foreach ($basicCaches as $cacheSvc => $cacheGrp) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@totten any reason why we are needing the 2 elements rather than just an array like ['js_strings','community_messages','checks']

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a cherry-pick from #11896, which has a few more draft patches which update the same array. Some of those use-cases could work with ['js_strings','community_messages','checks'], and one wouldn't. It's easier to juggle the patches (rebasing, re-ordering, cherry-picking, avoiding conflicts, etc) if they all use the use the same multi-line notation.

@eileenmcnaughton
Copy link
Contributor

I took a look at this & it makes a lot of sense - merging

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

Successfully merging this pull request may close these issues.

4 participants