Create Abstract Check Runner & CLI_Runner classes by jjgrainger · Pull Request #73 · WordPress/plugin-check

jjgrainger · 2023-02-08T14:31:01Z

Add Abstract_Check_Runner class

Addresses the acceptance criteria in Create Abstract_Check_Runner class #63
Addresses the acceptance criteria in Create CLI_Runner class #14

Closes #63, closes #14

felixarntz

@jjgrainger I left a number of comments here for a few things that I think are worth iterating on. Most importantly, using declarative methods to return specific parts make for a more intuitive API than relying on one method that has to set multiple specific class properties. It is like that in the ACs, but looking at the code it makes for a rather clunky experience.

One higher level thing that we have not considered so far, but I think it may be relevant here is that runtime checks as a whole should only ever be eligible to run if the plugin to check is in fact active. We may want to add a clause somewhere here that early on throws an exception if the plugin is not active and any of the provided checks to run is a runtime check.

Most comments not related to the above are nit-picks, e.g. about more closely following WP docs standards.

jjgrainger · 2023-02-10T10:39:11Z

@felixarntz I've updated the PR based on your feedback, this is ready for you to review. Thanks

jjgrainger · 2023-02-10T17:03:11Z

@felixarntz I've updated this PR to include the work for #14 in order to test the approach we discussed. This is ready for review.

felixarntz

@jjgrainger Thanks for the changes, this looks much better. I left some follow up feedback.

felixarntz

@jjgrainger This looks great. Only a few nit-picks, but nothing blocking.

felixarntz · 2023-02-13T19:24:03Z

+	 *
+	 * @since n.e.x.t
+	 *
+	 * @return Check_Context


Please add a description.

felixarntz · 2023-02-13T19:26:03Z

+		if ( ! isset( $this->checks ) ) {
+			// TODO: Add checks once implemented.
+			$checks = array(
+				'i18n-usage' => new Checks\I18n_Usage_Check(),


Using underscores is a bit of a better convention than using hyphens. For example, underscores are allowed in variable names and in JS object properties, while hyphens are not.

felixarntz · 2023-02-13T19:27:54Z

+	 *
+	 * @since n.e.x.t
+	 *
+	 * @return array An array of Check instances.


This should probably be changed to describe the same map that is now returned by the Checks::get_checks() method.

felixarntz · 2023-02-13T19:31:03Z

+	 * @return array {
+	 *      An array of Preparations to run where each item is an array with the class name and args.
+	 *
+	 *      @type string $class The full class name of the Preparation.
+	 *      @type array  $args  An array of parameters to pass to the class constructor.
+	 * }


While this is correct as a human-readable description, technically speaking in terms of PHPDoc this is not correct, as the current shape would suggest the return value is an associative array that includes class and args keys. However, the return value is an indexed array where each entry is that above array.

I think for simplicity sake we should therefore stick with just a precise description, otherwise the correct return type annotation would become somewhat complex.

Suggested change

* @return array {

* An array of Preparations to run where each item is an array with the class name and args.

*

* @type string $class The full class name of the Preparation.

* @type array $args An array of parameters to pass to the class constructor.

* }

* @return array An array of Preparations to run where each item is an array with keys `class` and `args`.

felixarntz · 2023-02-13T19:34:09Z

+		require_once ABSPATH . 'wp-admin/includes/plugin.php';
+
+		if ( empty( $plugin_slug ) ) {
+			throw new Exception( 'Missing positional argument. Please provide the plugin slug as first positional argument.' );


Please update the message here to be context-independent. This message worked well when it was directly scoped to the CLI command, but now referring to a "positional argument" no longer makes sense.

Same applies to the other messages below.

Suggested change

throw new Exception( 'Missing positional argument. Please provide the plugin slug as first positional argument.' );

throw new Exception( 'Invalid plugin slug: Plugin slug must not be empty.' );

felixarntz · 2023-02-13T19:34:21Z

+		if ( strpos( $plugin_slug, '/' ) ) {
+			throw new Exception(
+				sprintf(
+					'Invalid positional argument. Plugin with basename %s is not installed.',


Suggested change

'Invalid positional argument. Plugin with basename %s is not installed.',

'Invalid plugin basename: Plugin with basename %s is not installed.',

felixarntz · 2023-02-13T19:35:02Z

+
+		throw new Exception(
+			sprintf(
+				'Invalid positional argument. Plugin with slug %s is not installed.',


Suggested change

'Invalid positional argument. Plugin with slug %s is not installed.',

'Invalid plugin slug: Plugin with slug %s is not installed.',

mukeshpanchal27

Thanks @jjgrainger, LGTM!

jjgrainger added 4 commits February 5, 2023 21:36

setup abstract check runner class

a341672

Merge branch 'trunk' into feature/create-abstract-check-runner

967d77e

update get_shared_preparations method

a2c7420

minor fixes

69b0d66

jjgrainger requested review from felixarntz and vishalkakadiya February 8, 2023 14:31

jjgrainger added [Type] Enhancement A suggestion for improvement of an existing feature Infrastructure Issues for the overall plugin infrastructure Milestone 1 labels Feb 8, 2023

felixarntz requested changes Feb 8, 2023

View reviewed changes

jjgrainger added 4 commits February 9, 2023 18:01

simplify get_shared_preparations logic

0b0b4ae

replace properties with methods

a5e9f6e

update imports

fff26bc

update docblock comments

86d6930

jjgrainger requested a review from felixarntz February 9, 2023 18:47

jjgrainger added 3 commits February 10, 2023 15:12

create CLI_Runner class

319a8e0

add tests

bac30bb

remove imports

b6ccecb

jjgrainger commented Feb 10, 2023

View reviewed changes

Comment thread includes/Checker/Checks.php Outdated

fix tests

b90860c

jjgrainger changed the title ~~Create Abstract Check Runner class~~ Create Abstract Check Runner & CLI_Runner classes Feb 10, 2023

felixarntz reviewed Feb 10, 2023

View reviewed changes

This was referenced Feb 10, 2023

Create test DB tables using object-cache.php to generate demo data #42

Closed

Create AJAX_Runner class #15

Closed

jjgrainger added 4 commits February 13, 2023 09:15

remove get_context method

63de1b3

update checks methods

24e0d58

create Plugin_Request_Utility class

151ba82

update checks

a0b32c0

Merge branch 'trunk' into feature/create-abstract-check-runner

9626911

jjgrainger requested review from felixarntz and mukeshpanchal27 February 13, 2023 12:33

felixarntz approved these changes Feb 13, 2023

View reviewed changes

jjgrainger and others added 3 commits February 14, 2023 11:08

update comments and exception messages

ee26850

update tests

011c8a4

Merge branch 'trunk' into feature/create-abstract-check-runner

69a52ad

mukeshpanchal27 approved these changes Feb 14, 2023

View reviewed changes

mukeshpanchal27 merged commit 960756b into trunk Feb 14, 2023

mukeshpanchal27 deleted the feature/create-abstract-check-runner branch February 14, 2023 14:02

	throw new Exception( 'Missing positional argument. Please provide the plugin slug as first positional argument.' );
	throw new Exception( 'Invalid plugin slug: Plugin slug must not be empty.' );

	'Invalid positional argument. Plugin with basename %s is not installed.',
	'Invalid plugin basename: Plugin with basename %s is not installed.',

	'Invalid positional argument. Plugin with slug %s is not installed.',
	'Invalid plugin slug: Plugin with slug %s is not installed.',

Conversation

jjgrainger commented Feb 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felixarntz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jjgrainger commented Feb 10, 2023

Uh oh!

jjgrainger commented Feb 10, 2023

Uh oh!

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

felixarntz Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

felixarntz Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

felixarntz Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

felixarntz Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

felixarntz Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

felixarntz Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

felixarntz Feb 13, 2023

Choose a reason for hiding this comment

Uh oh!

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jjgrainger commented Feb 8, 2023 •

edited

Loading

felixarntz left a comment •

edited

Loading