Create Plugin_Main class #46

jjgrainger · 2022-12-19T11:12:50Z

Adds the Plugin_Main class.

This PR contains commits from Add Test Infrastructure #44 which should be reviewed/merged first
The Plugin_Main class accepts the plugin main file in the constructor
The Plugin_Main class creates a Plugin_Context object passing the main file parameter
The Plugin_Context class is a basic scaffold and will be addressed as part of Create Plugin_Context class #5
The Plugin_Main class has an empty add_hooks method that will be addressed as part of Create the WP-CLI subcommand wp plugin check #30

Closes #4

…-main-class

felixarntz

@jjgrainger I realize this include #44, but already left a few comments on the things that are new compared to the other PR.

felixarntz · 2022-12-19T21:22:06Z

includes/Plugin_Context.php

+/**
+ * Class representing the context in which the plugin is running.
+ *
+ * @since 0.1.0


Maybe use n.e.x.t throughout, like we do in Performance Lab? Since we don't know what the first version will be.

felixarntz · 2022-12-19T21:23:10Z

plugin-check.php

 * @package plugin-check
 */
+
+define( 'WP_PLUGIN_CHECK_VERSION', '0.1.0' );


felixarntz · 2022-12-19T21:25:09Z

plugin-check.php

+	if ( version_compare( phpversion(), WP_PLUGIN_CHECK_MINIMUM_PHP, '<' ) ) {
+		return;
+	}
+
+	// Check Composer autoloader exists.
+	if ( ! file_exists( plugin_dir_path( __FILE__ ) . 'vendor/autoload.php' ) ) {
+		return;
+	}


Instead of silently cancelling here, maybe add an action to trigger an admin notice, to indicate to the end user what went wrong? See the POC https://github.com/felixarntz/plugin-check/blob/main/plugin-check.php#L26 for reference.

felixarntz · 2022-12-19T21:27:57Z

includes/Plugin_Main.php

+	 *
+	 * @since 0.1.0
+	 *
+	 * @param string $main_file Absolute path of the plugin main file.
+	 * @return void
+	 */
+	public function __construct( $main_file ) {
+		$this->context = new Plugin_Context( $main_file );


Should we maybe add a public method context() or get_context() to be able to access this instance in a read-only way? That would also give us something a bit more appropriate (yet still simple) to unit test.

felixarntz · 2022-12-19T21:28:28Z

includes/Plugin_Main.php

+	 * @since 0.1.0
+	 *
+	 * @param string $main_file Absolute path of the plugin main file.
+	 * @return void


@return void should be omitted, per WP doc standards.

felixarntz · 2022-12-19T21:28:37Z

includes/Plugin_Main.php

+	 *
+	 * @since 0.1.0
+	 *
+	 * @return void


tests/Plugin_Main_Tests.php

felixarntz · 2022-12-19T21:31:31Z

tests/Plugin_Main_Tests.php

+	 *
+	 * @test
+	 */
+	public function it_can_be_instantiated() {


See my comment on #44 for using the test_ method prefix and avoiding doc-blocks (or more specifically the @test annotation) where not necessary.

mukeshpanchal27

Thanks @jjgrainger Left some feedback.

Question: Is there any reason to use the first capital in the class file name? If not, then we can use lowercase.

mukeshpanchal27 · 2022-12-20T14:45:44Z

includes/Plugin_Main.php

+namespace WordPress\Plugin_Check;
+
+/**
+ * Main class for the plugin


Suggested change

* Main class for the plugin

* Main class for the plugin.

As per Inline document CS, Use a period at the end.

mukeshpanchal27 · 2022-12-20T14:50:40Z

includes/Plugin_Main.php

+	}
+
+	/**
+	 * Register WordPress hooks for the plugin.


Suggested change

* Register WordPress hooks for the plugin.

* Registers WordPress hooks for the plugin.

Use third-person singular verb. See Language

mukeshpanchal27 · 2022-12-20T14:52:44Z

plugin-check.php

+ */
+function wp_plugin_check_load() {
+	// Check for supported PHP version.
+	if ( version_compare( phpversion(), WP_PLUGIN_CHECK_MINIMUM_PHP, '<' ) ) {


Why phpversion()? Is there any specific reason? The constant PHP_VERSION is used by WordPress.

@mukeshpanchal27 I think they both work the same, so I have left it as using phpversion()

mukeshpanchal27 · 2022-12-20T14:55:20Z

plugin-check.php

+	}
+
+	// Load the Composer autoloader.
+	require_once plugin_dir_path( __FILE__ ) . 'vendor/autoload.php';


Similar to Performance Lab lets define some constant that we can use in other places.

https://github.com/WordPress/performance/blob/trunk/load.php#L19-L20

felixarntz

@jjgrainger LGTM, just one nit-pick.

felixarntz · 2022-12-20T17:23:38Z

tests/Plugin_Main_Tests.php

+	public function test_context() {
+		$context = $this->plugin_main->context();
+
+		$this->assertIsObject( $context );


Nit-pick, this is unnecessary since the assertion below by definition covers that already.

mukeshpanchal27

@jjgrainger looks great.

jjgrainger added 5 commits December 15, 2022 17:19

add Plugin_Main class

6c94443

add Plugin_Context class

64bfd75

setup plugin

381b997

add Plugin_Main tests

fab71e2

Merge branch 'feature/test-infrastructure' into feature/create-plugin…

80b864f

…-main-class

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

jjgrainger requested review from felixarntz and mukeshpanchal27 December 19, 2022 11:13

jjgrainger added 2 commits December 19, 2022 11:26

update PHPCS rules to allow for psr-4 autoloading

58f7745

fix phpcs issues

da50b3a

jjgrainger mentioned this pull request Dec 19, 2022

Create Plugin_Context class #47

Merged

felixarntz reviewed Dec 19, 2022

View reviewed changes

jjgrainger added 5 commits December 20, 2022 11:06

update all version to next

4cf2f8f

add admin notices if requirements are not met

78741f8

add context method

a3e7d7d

update tests

fd3ccb3

remove void docblock comment

96215e5

jjgrainger requested a review from felixarntz December 20, 2022 14:13

mukeshpanchal27 reviewed Dec 20, 2022

View reviewed changes

jjgrainger added 3 commits December 20, 2022 15:30

fix comments

77112a6

add plugin directory constant

4fb7769

Merge branch 'trunk' into feature/create-plugin-main-class

425a069

jjgrainger marked this pull request as ready for review December 20, 2022 17:01

jjgrainger requested a review from mukeshpanchal27 December 20, 2022 17:02

felixarntz approved these changes Dec 20, 2022

View reviewed changes

mukeshpanchal27 approved these changes Dec 21, 2022

View reviewed changes

jjgrainger merged commit f5e87b6 into trunk Dec 21, 2022

jjgrainger deleted the feature/create-plugin-main-class branch December 21, 2022 07:45

	* Register WordPress hooks for the plugin.
	* Registers WordPress hooks for the plugin.

Create Plugin_Main class #46

Create Plugin_Main class #46

Uh oh!

Conversation

jjgrainger commented Dec 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

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!

4 participants

jjgrainger commented Dec 19, 2022 •

edited

Loading