#56040 site health persistent object cache#2890
#56040 site health persistent object cache#2890manuelRod wants to merge 9 commits intoWordPress:trunkfrom
Conversation
audrasjb
left a comment
audrasjb
left a comment
There was a problem hiding this comment.
I reported a few nitpicks on the PR (I didn't mention all occurrences though) 😇
|
Thanks for the nitpicks @audrasjb, I've fixed them. |
|
I think this one is ready to be merged. |
Clorith
left a comment
Clorith
left a comment
There was a problem hiding this comment.
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! |
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@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 |
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@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.
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.' ); |
Clorith
left a comment
Clorith
left a comment
There was a problem hiding this comment.
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.
@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 🎉 |
6 participants
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.