Create Plugin_Context class #47

jjgrainger · 2022-12-19T12:27:38Z

Adds the Plugin_Context class

Addresses acceptance criteria in Create Plugin_Context class #5

Closes #5

felixarntz

@jjgrainger Left a few comments on the code here that isn't in #44 or #46 (mostly similar feedback though).

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

includes/Plugin_Context.php

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


See my comments on #46, let's use n.e.x.t.

tests/Plugin_Context_Tests.php

felixarntz · 2022-12-19T21:33:29Z

tests/Plugin_Context_Tests.php

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


See my comments on #44 and #46 regarding usage of test_ prefix (applies throughout this class). The convention is to just call the method like the tested method (i.e. here test_basename()), unless there are multiple test methods for the same method in which case the method should be more descriptive (e.g. test_basename_returns_basename(), though here that is a pointless example).

felixarntz · 2022-12-19T21:35:04Z

tests/Plugin_Context_Tests.php

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


See above, this could be test_path().

felixarntz · 2022-12-19T21:35:43Z

tests/Plugin_Context_Tests.php

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


And this test_path_with_parameter().

felixarntz · 2022-12-19T21:35:58Z

tests/Plugin_Context_Tests.php

+	 * @test
+	 */
+	public function it_can_return_the_path() {
+		$basename = $this->plugin_context->path();


Variable name here and below is misleading, likely copy-paste oversight :)

mukeshpanchal27

Thanks @jjgrainger, Left nit-pick feedback.

mukeshpanchal27 · 2022-12-20T15:18:42Z

includes/Plugin_Context.php

+	}
+
+	/**
+	 * Returns the full URL for a pth relative to the plugin directory.


Suggested change

* Returns the full URL for a pth relative to the plugin directory.

* Returns the full URL for a path relative to the plugin directory.

mukeshpanchal27 · 2022-12-20T15:22:30Z

tests/Plugin_Context_Tests.php

+		$basename = $this->plugin_context->basename();
+
+		$this->assertSame( $basename, 'plugin-check/plugin-check.php' );


Suggested change

$basename = $this->plugin_context->basename();

$this->assertSame( $basename, 'plugin-check/plugin-check.php' );

$this->assertSame( 'plugin-check/plugin-check.php', $this->plugin_context->basename() );

As per the assertSame documentation the expected should be in first place. Also remove variable.

Do similar change for other tests.

mukeshpanchal27

Thanks @jjgrainger, LGTM!

felixarntz

@jjgrainger Overall LGTM, however I have a few suggestions for making the tests more reliable, as they currently use hard-coded strings which I suspect will fail depending on how the test environment is set up. (They work here, but they may not work if someone sets up tests in another way, e.g. without Docker.)

felixarntz · 2022-12-21T21:59:11Z

tests/Plugin_Context_Tests.php

+	public function test_path() {
+		$this->assertSame( 'plugin-check/', $this->plugin_context->path() );
+	}
+
+	public function test_path_with_parameter() {
+		$this->assertSame( 'plugin-check/another/folder', $this->plugin_context->path( '/another/folder' ) );
+	}


To make these two checks more stable, I would suggest not hard-coding an assumed path here. It'd be more reliable to use dirname( __DIR__ ) instead of plugin-check.

felixarntz · 2022-12-21T22:01:31Z

tests/Plugin_Context_Tests.php

+	public function test_url() {
+		$this->assertSame( site_url( '/wp-content/plugins/plugin-check/' ), $this->plugin_context->url() );
+	}
+
+	public function test_url_with_parameter() {
+		$this->assertSame( site_url( '/wp-content/plugins/plugin-check/folder/file.css' ), $this->plugin_context->url( '/folder/file.css' ) );
+	}


Same here, how about using WP_PLUGIN_DIR . '/' . basename( dirname( __DIR__ ) ) instead of site_url( '/wp-content/plugins/plugin-check' )?

@felixarntz Here we have to use WP_PLUGIN_URL instead of WP_PLUGIN_DIR

felixarntz · 2022-12-21T22:02:26Z

tests/Plugin_Context_Tests.php

+	public function test_basename() {
+		$this->assertSame( 'plugin-check/plugin-check.php', $this->plugin_context->basename() );
+	}


Technically, even here we could make it more reliable by using basename( dirname( __DIR__ ) ) instead of the plugin-check directory. We know for sure the main file is called plugin-check.php, but the directory name depends on the WordPress setup and where the plugin is installed to.

mukeshpanchal27 · 2022-12-22T04:04:56Z

tests/Plugin_Context_Tests.php

+	public function set_up() {
+		parent::set_up();
+
+		$this->plugin_context = new Plugin_Context( 'plugin-check/plugin-check.php' );


Can we apply #47 (comment) changes here also?

mukeshpanchal27

@jjgrainger Over to you for review of the changes and merging.

mukeshpanchal27 · 2022-12-22T04:07:54Z

tests/Plugin_Context_Tests.php

+	public function set_up() {
+		parent::set_up();
+
+		$this->plugin_context = new Plugin_Context( 'plugin-check/plugin-check.php' );


Suggested change

$this->plugin_context = new Plugin_Context( 'plugin-check/plugin-check.php' );

$this->plugin_name = basename( dirname( __DIR__ ) );

$this->plugin_context = new Plugin_Context( $this->plugin_name . '/plugin-check.php' );

mukeshpanchal27 · 2022-12-22T04:08:18Z

tests/Plugin_Context_Tests.php

+	}
+
+	public function test_basename() {
+		$this->assertSame( 'plugin-check/plugin-check.php', $this->plugin_context->basename() );


Suggested change

$this->assertSame( 'plugin-check/plugin-check.php', $this->plugin_context->basename() );

$this->assertSame( $this->plugin_name . '/plugin-check.php', $this->plugin_context->basename() );

mukeshpanchal27 · 2022-12-22T04:08:38Z

tests/Plugin_Context_Tests.php

+	}
+
+	public function test_path() {
+		$this->assertSame( 'plugin-check/', $this->plugin_context->path() );


Suggested change

$this->assertSame( 'plugin-check/', $this->plugin_context->path() );

$this->assertSame( $this->plugin_name . '/', $this->plugin_context->path() );

mukeshpanchal27 · 2022-12-22T04:11:23Z

tests/Plugin_Context_Tests.php

+	}
+
+	public function test_path_with_parameter() {
+		$this->assertSame( 'plugin-check/another/folder', $this->plugin_context->path( '/another/folder' ) );


Suggested change

$this->assertSame( 'plugin-check/another/folder', $this->plugin_context->path( '/another/folder' ) );

$this->assertSame( $this->plugin_name . '/another/folder', $this->plugin_context->path( '/another/folder' ) );

mukeshpanchal27 · 2022-12-22T04:11:44Z

tests/Plugin_Context_Tests.php

+	}
+
+	public function test_url() {
+		$this->assertSame( site_url( '/wp-content/plugins/plugin-check/' ), $this->plugin_context->url() );


Suggested change

$this->assertSame( site_url( '/wp-content/plugins/plugin-check/' ), $this->plugin_context->url() );

$this->assertSame( WP_PLUGIN_URL . '/' . $this->plugin_name . '/', $this->plugin_context->url() );

mukeshpanchal27 · 2022-12-22T04:12:10Z

tests/Plugin_Context_Tests.php

+	}
+
+	public function test_url_with_parameter() {
+		$this->assertSame( site_url( '/wp-content/plugins/plugin-check/folder/file.css' ), $this->plugin_context->url( '/folder/file.css' ) );


Suggested change

$this->assertSame( site_url( '/wp-content/plugins/plugin-check/folder/file.css' ), $this->plugin_context->url( '/folder/file.css' ) );

$this->assertSame( WP_PLUGIN_URL . '/' . $this->plugin_name . '/folder/file.css', $this->plugin_context->url( '/folder/file.css' ) );

mukeshpanchal27 · 2022-12-22T04:31:18Z

tests/Plugin_Main_Tests.php

 		$context = $this->plugin_main->context();



Suggested change

$context = $this->plugin_main->context();

Remove the variable.

mukeshpanchal27 · 2022-12-22T04:31:38Z

tests/Plugin_Main_Tests.php

 		$context = $this->plugin_main->context();

-		$this->assertIsObject( $context );
 		$this->assertInstanceOf( Plugin_Context::class, $context );


Suggested change

$this->assertInstanceOf( Plugin_Context::class, $context );

$this->assertInstanceOf( Plugin_Context::class, $this->plugin_main->context() );

felixarntz · 2022-12-22T17:14:08Z

tests/Plugin_Context_Tests.php

+	public function test_path() {
+		$this->assertSame( $this->plugin_name . '/', $this->plugin_context->path() );
+	}
+
+	public function test_path_with_parameter() {
+		$this->assertSame( $this->plugin_name . '/another/folder', $this->plugin_context->path( '/another/folder' ) );
+	}


@jjgrainger Just spotting this post-merge: This still doesn't fully address my original feedback: I would think we need to use WP_PLUGIN_DIR . '/' . $this->plugin_name instead of just $this->plugin_name. Have you tried that, were there any problems? At least in theory this still looks unstable to me without that bit.

Let me know what you think. If my concerns are valid, we can fix this throughout in a quick follow up PR.

cc @mukeshpanchal27

The plugin_dir_path( $this->main_file ) return plugin directory slug instead of the full relative path.

Thanks both, I've addressed these comments in #51

mukeshpanchal27 · 2023-01-03T09:11:46Z

includes/Plugin_Context.php

+	 * @return string Absolute path.
+	 */
+	public function path( $relative_path = '/' ) {
+		return plugin_dir_path( $this->main_file ) . ltrim( $relative_path, '/' );


Should update this line with return plugin_dir_path( WP_PLUGIN_CHECK_PLUGIN_DIR_PATH ) . $this->main_file . ltrim( $relative_path, '/' ); to get full relative path. If we use this and run the unit test it gives Use of undefined constant WP_PLUGIN_CHECK_PLUGIN_DIR_PATH - assumed 'WP_PLUGIN_CHECK_PLUGIN_DIR_PATH' (this will throw an Error in a future version of PHP) notice that needs to figure out.

@jjgrainger I'd like to get your feedback on this.

Thanks @mukeshpanchal27

I've addressed this in #51. We're unable to use the WP_PLUGIN_CHECK_PLUGIN_DIR_PATH as it's created in the plugin-check.php file which is not bootstrapped as part of the tests (nor should it be).

The line here works correctly as the full path is expected to be passed in the constructor (see the `plugin-check.php file here).

I've updated the tests based on the feedback in #51 which I believe to be correct.

* Add an initial Trademark check. This is based entirely upon the code currently used within the Plugin Directory upload. * Add unit test * Fix the tests; use a singular needle. * Tests: Test the slugs. * Correct the portmanteau check, add test. * Tests: Fully install all dependencies. * Typo * Ensure that for-use exceptions don't miss it being used earlier in the name * Even if a plugin is using the valid for-use exception, check for other trademarks they may not be observing. * Reduce cognitive load for is_valid_for_use_exception and include the rest of the docblock * Remove verifications against the email since this data is not public + Code Standards and Docblocks for the whole file. * Add back into the code references to code that will only run when used in the Plugins Repository site * Include validation to only happen if the plugin is being reviewed. --------- Co-authored-by: Gustavo Bordoni <[email protected]>

jjgrainger added 2 commits December 19, 2022 11:47

update Plugin_Context class

eb75be8

add Plugin_Context tests

0cea27d

jjgrainger requested review from felixarntz and mukeshpanchal27 December 19, 2022 12:27

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

felixarntz reviewed Dec 19, 2022

View reviewed changes

jjgrainger added 2 commits December 20, 2022 14:01

use n.e.x.t for versioning

d04f871

update tests

fea62b3

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

jjgrainger mentioned this pull request Dec 20, 2022

Create Check_Context class #48

Merged

mukeshpanchal27 reviewed Dec 20, 2022

View reviewed changes

jjgrainger added 5 commits December 20, 2022 16:00

update tests

ae18605

fix tests

5e0e34b

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

4a752c1

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

98ad5a2

remove unecessary assertion in tests

8d8a6d9

jjgrainger requested a review from mukeshpanchal27 December 21, 2022 09:11

jjgrainger marked this pull request as ready for review December 21, 2022 09:12

mukeshpanchal27 approved these changes Dec 21, 2022

View reviewed changes

felixarntz approved these changes Dec 21, 2022

View reviewed changes

mukeshpanchal27 reviewed Dec 22, 2022

View reviewed changes

mukeshpanchal27 approved these changes Dec 22, 2022

View reviewed changes

update tests to use more reliable context

985bcb3

jjgrainger merged commit 8561820 into trunk Dec 22, 2022

jjgrainger deleted the feature/create-plugin-context-class branch December 22, 2022 11:32

felixarntz reviewed Dec 22, 2022

View reviewed changes

mukeshpanchal27 reviewed Jan 3, 2023

View reviewed changes

jjgrainger mentioned this pull request Jan 4, 2023

Fix context class test stability #51

Merged

	* Returns the full URL for a pth relative to the plugin directory.
	* Returns the full URL for a path relative to the plugin directory.

		$basename = $this->plugin_context->basename();

		$this->assertSame( $basename, 'plugin-check/plugin-check.php' );

	$this->plugin_context = new Plugin_Context( 'plugin-check/plugin-check.php' );
	$this->plugin_name = basename( dirname( __DIR__ ) );
	$this->plugin_context = new Plugin_Context( $this->plugin_name . '/plugin-check.php' );

	$this->assertSame( 'plugin-check/plugin-check.php', $this->plugin_context->basename() );
	$this->assertSame( $this->plugin_name . '/plugin-check.php', $this->plugin_context->basename() );

	$this->assertSame( 'plugin-check/', $this->plugin_context->path() );
	$this->assertSame( $this->plugin_name . '/', $this->plugin_context->path() );

	$this->assertSame( 'plugin-check/another/folder', $this->plugin_context->path( '/another/folder' ) );
	$this->assertSame( $this->plugin_name . '/another/folder', $this->plugin_context->path( '/another/folder' ) );

	$this->assertSame( site_url( '/wp-content/plugins/plugin-check/' ), $this->plugin_context->url() );
	$this->assertSame( WP_PLUGIN_URL . '/' . $this->plugin_name . '/', $this->plugin_context->url() );

	$this->assertInstanceOf( Plugin_Context::class, $context );
	$this->assertInstanceOf( Plugin_Context::class, $this->plugin_main->context() );

Create Plugin_Context class #47

Create Plugin_Context class #47

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!

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!

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!

mukeshpanchal27 left a comment

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

jjgrainger commented Dec 19, 2022 •

edited

Loading