-
Notifications
You must be signed in to change notification settings - Fork 52
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 plugin debug information to the Site Health screen #108
Conversation
includes/Classifai/Plugin.php
Outdated
@@ -78,6 +78,7 @@ public function init_services() { | |||
$this->services = [ | |||
'service_manager' => new Services\ServicesManager( $classifai_services ), | |||
'admin_notifications' => new Admin\Notifications(), |
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.
Putting non-service-related classes in $this->services
doesn't seem quite right, but I followed what was already there.
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.
@johnwatkins0 this was a side-effect of the refactor work done to allow the plugin to support multiple services. At the time, there wasn't a need to handle non-services differently. This can probably be added now as we have both notices and debug information .
* @return string|array Debug info to display on the Site Health screen. Accepts a string or key-value pairs. | ||
* @since 1.4.0 | ||
*/ | ||
public function get_provider_debug_information() { |
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.
@johnwatkins0 as this class is abstract, it can never be instantiated directly so this stub is never called. It may make sense to mark this method as abstract as well to enforce that any providers implement this.
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.
@johnwatkins0 thanks for submitting this! I've added a couple of items to the PR. I think we had an issue using the null coalescing operator at one point but we should confirm that with @helen or @jeffpaul before making any changes.
Thanks, @ryanwelcher ! I've pushed the following updates: From CR
Not from CRMade booleans that show in the debug information more human-friendly -- "yes"/"no" instead of "1"/empty. Let me know if you have questions or more suggestions. |
I think what I'd like to see added here is the results of the most recent request, maybe to each provider. Could help a lot with troubleshooting. So store those (request response I guess?) in options somewhere and translate them into human-ish terms maybe. |
I'll add a separate issue to include the details on services within Site Health, that way this PR can land in 1.4 and we can enhance it in 1.5 |
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 made a couple output formatting tweaks but otherwise looks good to me. I didn't look too closely at the internationalization aspect, though given lack of actual localizations at the moment I will worry about that later.
@johnwatkins0 Next considerations for Site Health: storing and displaying debug info for the last request (or requests?) for each provider, and considering whether it makes sense to make the setup status of the plugin a part of the health tests. It may not, but we should at least consider it.
This resolves #83, which describes a need to display plugin debug information on the new Site Health screen introduced to core in 5.2.
Description of the Change
It adds a new accordion drawer to the Site Health info screen that looks like this:
We do this by hooking into the new
debug_information
filter provided by core, adding a key forclassifai
, and providing a set of plugin details that would be useful for debugging purposes. This is done via aclassifai_debug_information
filter that can be hooked into throughout the plugin.Alternate Designs
DebugInfo
class provides theclassifai_debug_information
filter and delegates the adding of debug information to the individual features in the plugin via the filter. Alternatively, we could do all the work needed to retrieve debug information in theDebugInfo
class. This might be simpler, but I liked the extensibility the filter provides.Benefits
Plugin users have a concrete, nontechnical step to share information needed for debugging.
Possible Drawbacks
Expecting debug information to be provided with all plugin features adds a small amount of overhead.
Verification Process
Using an up-to-date Version of WP:
Checklist:
Applicable Issues
#83