-
Notifications
You must be signed in to change notification settings - Fork 109
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
Add Site Health check module for persistent object cache #111
Conversation
@tillkruss Can you explain what I am meant to see in site health. I checked two sites and I see any new options in site health? Can you provide details to test this and maybe a screenshot? |
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.
@tillkruss LGTM, just a few last nit-picks.
modules/object-cache/persistent-object-cache-health-check/load.php
Outdated
Show resolved
Hide resolved
modules/object-cache/persistent-object-cache-health-check/load.php
Outdated
Show resolved
Hide resolved
modules/object-cache/persistent-object-cache-health-check/load.php
Outdated
Show resolved
Hide resolved
….php Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
….php Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
….php Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
@felixarntz: All done. Any filter you'd like me to remove? |
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.
Good first PR.
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.
@tillkruss I think all filters currently present make sense, except for the one I'm flagging below. Particularly the filters to modify the Site Health contents are great to have.
If you feel strongly about keeping this filter, I'd say at a minimum we should put that into the perflab_oc_health_should_persistent_object_cache()
function as a "short-circuit" filter, rather than hooking that function onto the filter.
modules/object-cache/persistent-object-cache-health-check/load.php
Outdated
Show resolved
Hide resolved
modules/object-cache/persistent-object-cache-health-check/load.php
Outdated
Show resolved
Hide resolved
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.
@tillkruss Production code lgtm, leaving a few recommendations on tests. At a minimum, we should fix the one filter usage there that's using the wrong filter.
tests/modules/object-cache/persistent-object-cache-health-check/health-check-test.php
Outdated
Show resolved
Hide resolved
tests/modules/object-cache/persistent-object-cache-health-check/health-check-test.php
Outdated
Show resolved
Hide resolved
tests/modules/object-cache/persistent-object-cache-health-check/health-check-test.php
Outdated
Show resolved
Hide resolved
modules/object-cache/persistent-object-cache-health-check/load.php
Outdated
Show resolved
Hide resolved
….php Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
@felixarntz: All resolved in 6291513. |
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.
@tillkruss Awesome stuff! 🎉
Summary
This PR adds the
object-cache/health
module containing a single health check to suggest the usage of a persistent object cache backend, if needed.The module adds the direct
persistent_object_cache
health check.site_status_suggest_persistent_object_cache
filter to inject their own criteria checks to suggest the use of an object cache, as well as bypass the default core health checksite_status_persistent_object_cache_url
filter to link to their own support docssite_status_persistent_object_cache_notes
filter to recommend their preferred object caching solutionsite_status_persistent_object_cache_notes
filter on why object caching is recommended for their pluginResolves #35.