Create Checks class

[mehulkaklotar]

Description

The Checks class will be the main class for running checks against a plugin. This class contains all the logic to collect the Plugin and Check contexts, run Preparations ahead of checks, loop over and run all Checks and collect and return the results.

Containing this logic within a single class decoupled from I/O allows this class to be used in the different contexts in which checks can run, such as WP-CLI or in the WordPress admin, without duplicating efforts and logic which is described more in detail in the Check Orchestration and Execution Approach.

The class will contain a method to return all available checks that can be run against the plugin, which is filterable so developers can extend/modify which checks should run.

Additionally, a method to run a set of checks (by default all checks) should be provided, so the entire suite of checks can be used or a plugin can be tested against one check alone as checks are provided as a list. The method can iterate through the list of checks and execute them. For example, it allows running a single check when a CLI command is executed with one check to test for a plugin and it also allows running a single check as part of running all checks for a plugin when a CLI command or even AJAX requests is initiated to test a plugin for all checks.

The Checks class is also responsible for preparation work ahead of running tests and cleanup after tests have run.

Acceptance Criteria

• Checks class is created and exists at includes/Checker
• Protected property $check_context should be there which is an instance of Check_Context
• Constructor receives the $plugin_main_file as an argument
• Method get_checks should be created as public and returns the list of checks
• Method run_check_with_result should be created as protected and receives Check $check and Check_Result $check_result as an arguments.
• Method run_checks should be created as public and accepts a single parameter array $checks which is an array of the check class names to run. The method will loop over the $checks array and run each check by calling run_check_with_result.

Tests Coverage

• get_checks method returns the list of expected checks
• run_check_with_result runs expected check and result is stored as expected
• run_checks runs the expected checks based on the parameter passed

[felixarntz]

@jjgrainger

• Protected property $plugin_context should be there which is an instance of Plugin_Context

For what would that be used? Related is my feedback on #50, do we really need it?

• Method prepare should be created as public and prepares checks for running and returns cleanups

How should it prepare the checks, or what concretely should it prepare for the checks?

• Method run_checks should be created as public and runs the list of checks by checking if $prepare is true and uses run_check_with_result

Nit-pick, typo in $prepare (should be $prepared).

[jjgrainger]

Thanks @felixarntz

I agree based on your other feedback that we shouldn't pass the Plugin_Context unless it's needed and will remove it for now.

As for preparing checks I think this class should be simplified to only contain a list of available checks and methods for returning all checks and returning specific checks based on options passed. My reasoning is that we have the Check_Runner classes (#13 #14 #15) which also have AC's containing similar logic.

So the updated AC's would look more like the following:

• A Checks class is created and exists at includes/Checker
• A public method all_checks() returns an array of all available checks as Check instances
• A public method get_checks( array $args ) returns an array of checks as Check instances based on the $args passed.
  • $args is an array of options that determine what checks are passed. The array can include one of the following options:
    • include - an array of Checks only to be run, filtered from available checks returned by all_checks().
    • exclude - an array of Checks to exclude. This is used to filter out specific checks from all available checks returned by all_checks().

This is simple for now but $args could be expanded to include other useful options to filter checks, for example by type (runtime or static) or severity (warning or error).

The array of checks returned by the Checks class can then be passed to a Check Runners run() method along with the Plugin_Context e.g ($runner->run( $checks, $check_context )).

Let me know your thoughts here and I can update the AC's to reflect the changes.

[jjgrainger]

Thanks @felixarntz

After reading your comments on Slack I agree we can keep the check runs within this class.

I've updated the AC's for 2 of the points you've raised but have some thoughts/questions on the prepare() method in this class.

• Part of the Check_Runners responsibilities is to orchestrate checks and preparations to be run in an optimal way, especially for when the checks are run via AJAX.
• Looking at the POC the Checks class was preparing universal preparations ahead of checks being run (Activating the plugin and an empty theme).
• Considering some checks are static analysis, I believe these checks would not need these preparations.
• Should we remove the prepare() method from the Checks class so that the Check_Runners can handle these universal preparations as part of the orchestration?
• The Checks class can still handle individual check preparations when running the check.

Let me know your thoughts here, happy to discuss on a call if needed.

[felixarntz]

@jjgrainger

Considering some checks are static analysis, I believe these checks would not need these preparations.

That's a great point. I agree it suggests that we shouldn't have a prepare method in the Checks class.

However, we do need to consider that the Minimal_Theme_Preparation is universal when it comes to runtime checks at least. The same way that we will have to ensure no plugins except for the plugin to test is loaded when doing runtime checks.

How about the following:

• We remove prepare from Checks (would require an update in #57).
• We introduce a new preparation class like Force_Single_Plugin_Preparation. This class would use a filter on the active_plugins option to ensure only the one plugin to be tested is loaded.
• We introduce another class like Universal_Runtime_Preparation which itself is a preparation, but wraps both the Force_Single_Plugin_Preparation and Minimal_Theme_Preparation.
• Later, this would be used as follows in CLI and AJAX:
  • For CLI, the CLI runner class will need to conditionally use the Universal_Runtime_Preparation, but only if any of the requested checks to run are runtime checks.
  • For AJAX, the AJAX runner class will need to conditionally use the Universal_Runtime_Preparation, but only if any of the checks to do in the request are runtime checks.

We will eventually need to conditionally instantiate the CLI runner or AJAX runner class in our object-cache.php drop-in, so that the preparations run early enough. That whole file is going to end up quite a hack :D

WDYT? We can create a few new issues to implement the above if we want to go with such an approach.

[jjgrainger]

Thanks @felixarntz

I agree with your points here and have been putting together my own thoughts on #37 as I've been thinking about the orchestration side of things.

I don't believe #37 will become a concrete thing that will need engineering so we could use this issue to discuss the wider, high-level thoughts on how Checks are orchestrated, prepared and executed. Then when we come to a solution I can update the impacted issues with changes.

[felixarntz]

@jjgrainger Regarding the AC updates, why go with run_single_check and run_all_checks instead of run_checks that we had previously aligned on? That seems unrelated to the overarching architecture changes we had talked about.

[jjgrainger]

Thanks @felixarntz I've updated the AC's and PR with the run_checks method like we've discussed.