-
Notifications
You must be signed in to change notification settings - Fork 2
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 Health Checks #36
Conversation
…d external to build the links automatically.
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.
Just a few minor things, @bradp. Everything else looks good to me! 😃
includes/HealthChecks.php
Outdated
/** | ||
* Add health checks. | ||
*/ | ||
public function addHealthChecks() { |
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 think this works well for now. But if this file grows into a large monolith in the future, do you think it might make sense to create an abstract class for new health checks to inherit from?
For instance, we could have a HealthChecks
directory with an AbstractHealthCheck.php
file that defines and documents all the necessary fields. A class like LinkPrefetchHealthCheck.php
could then inherit from it and include all tests related to link prefetching.
In this file, we can simply call $manager->addHealthCheck( new LinkPrefetchHealthCheck() );
. This approach would provide better validation and typing in PHP, prevent files from becoming too large, group related functionality, and avoid people adding random data directly into the manager. What do you think?
includes/HealthChecks.php
Outdated
|
||
/* // phpcs:ignore Squiz.PHP.CommentedOutCode | ||
// Enable when https://github.com/newfold-labs/wp-module-performance/pull/26 is merged. | ||
// PRESS7-120: Link Prefetching. |
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.
The Link Prefetch and Jetpack Boost stuff has been merged. I'm working on getting the Image Optimization changes merged as soon as possible after a review! 😃
@bradp can we get these comments addressed and the conflict resolved? Looks like there's a lint check issue(s) too. |
…s for each check for more organization
I've refactored this, added unit testing, and gotten everything cleaned up! |
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.
@bradp Just one comment regarding the translations namespace. Everything else looks good to me. I'll approve and merge as soon as you update that namespace everywhere. I'll also get this QA'ed meanwhile. Thanks!
Proposed changes
Implements a variety of Health Checks to the Site Health page.
Jira tickets: PRESS7-108, PRESS7-109, PRESS7-110, PRESS7-111, PRESS7-118, PRESS7-112, PRESS7-113, PRESS7-121, PRESS7-107, PRESS7-119, PRESS7-120, PRESS7-114, PRESS7-115, PRESS7-116, PRESS7-117.
Type of Change
Screenshot
Checklist
Further comments
This PR adds
HealthCheckManager.php
which is a helper class to set up and add health checks to the Site Health page. This is used inHealthChecks.php
to add the health checks to the Site Health page.This makes it so adding a new health check is as simple as:
This PR also adds Health Checks that are commented out for the following PRs