Fix treating callable array as string

[cjhaas]

Short introduction

Initially posted in #384 , health test data exists as either an array-based callable or a string, but cron invoking assumes the latter

Description of what the PR accomplishes

The health test data exists as either a callable using array syntax, or a string that gets prepended with get_test_. The cron job that actually runs this code, however, first blindly assumes that it is a string and uses it as such in a sprintf statement which raises a notice:

PHP Notice: Array to string conversion in wp-content\plugins\health-check\includes\class-health-check-site-status.php on line 2014

This PR fixes the logic to check if the current test is a callable array and if so, use it, and then falls back to the original logic.

To reproduce the bug, install and activate the plugin on a regular WP site (tested against WP 5.5.1 running PHP 7.4.2) and try manually running all cron jobs:

wp cron event run --all

Screenshots

n/a

PR Checklist

These are things to ensure are covered by your PR.

• [x] This PR is created against the develop branch
• [x] This PR is feature ready and can be reviewed in its entirety

[mbtools]

LGTM. When will this be merged?

[Clorith]

Good catch, yes the logic here is definitely backwards and leading to notices for any custom registered checks, thanks!

[TimothyBJacobs]

This should probably be encapsulated into a separate method, because this also doesn't look right to me.

health test data exists as either an array-based callable or a string

I don't think this is accurate. It can be any callable, not an array based callable. The check in enqueue_scripts looks if test is a string and if it corresponds to a method on the class. Otherwise, it just does a simple is_callable check.

This looks to me that it breaks non array callables. For instance a closure, object implementing __invoke or a function name that doesn't match against the site health class.

[cjhaas]

@TimothyBJacobs , just to be clear, when I said array-based callable I didn't mean [$obj, 'method'] syntax, although in hindsight it really sounds like I did. What I meant was that the test key of the provided array could either be a string, or it could actually be a callable (closure, __invoke, whatever). The bug is that the sprintf happens before any type checks and assumes for former (a string).

Looking longer at this, I think this statement:

if ( is_array( $test['test'] ) && is_callable( $test['test'] ) ) {

could be simplified further to:

if ( is_callable( $test['test'] ) ) {

although I haven't run that through any tests yet.