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
#56040 site health persistent object cache #2890
#56040 site health persistent object cache #2890
Conversation
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.
I reported a few nitpicks on the PR (I didn't mention all occurrences though) 😇
Thanks for the nitpicks @audrasjb, I've fixed them. |
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.
Looks good to me.
I think this one is ready to be merged. |
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.
Had a look over, and I have some questions that I left in-line, and a quick reference for some potential improvements.
using phpcs:ignore rule for interpolation
@tillkruss @Clorith I've implemented the changes.
Thanks! |
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.
@manuelRod Excellent work, this looks solid to me. I left a few comments on minor things to improve, such as the test slug and some documentation.
I think this is good to go now! cc: @Clorith @tillkruss @felixarntz |
LGTM |
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.
@manuelRod One tiny nit-pick, however I can address that one pre-commit. This looks good to go, great work!
@Clorith Can you please give this another review this week or early next week? It now has 3 approvals, from my perspective it's ready to commit. |
|
||
$available_services = $this->available_object_cache_services(); | ||
|
||
$notes = __( 'Your hosting provider can tell you if persistent object cache can be enabled on your site.' ); |
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.
To me this sounds incorrect.
$notes = __( 'Your hosting provider can tell you if persistent object cache can be enabled on your site.' ); | |
$notes = __( 'Your hosting provider can tell you if a persistent object cache can be enabled on your site.' ); |
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.
I agree with the small string change by @tillkruss, but other than that it looks good 👍
* | ||
* @param bool $suggest Whether to suggest using a persistent object cache. | ||
*/ | ||
if ( apply_filters( 'site_status_suggest_persistent_object_cache', false ) ) { |
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.
@manuelRod A bit late, but I am just realizing that this should rather follow a more appropriate short-circuiting behavior: Right now, the only way to short-circuit would be to actually recommend a persistent object cache. But it also needs to be possible to run custom logic that would result in not recommending a persistent object cache. So I think the default value here should be null
, and any boolean result should cause the method to return early with that result.
This is a trivial change, which is compatible with the current behavior, it just expands it to also allow returning false
to short-circuit.
@manuelRod Committed in https://core.trac.wordpress.org/changeset/53955 🎉 |
Adding Persistent Object cache check to Site Health.
Trac ticket: #56040
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.