Refactor to remove repeated code. by spacedmonkey · Pull Request #157 · WordPress/plugin-check

spacedmonkey · 2023-04-28T11:39:13Z

Description of the Change

While reviewing code, I noticed a lot of repeated code in Admin_Ajax. Simple refactor to tidy up the code.

Closes #

How to test the Change

Changelog Entry

Added - New feature
Changed - Existing functionality
Deprecated - Soon-to-be removed feature
Removed - Feature
Fixed - Bug fix
Security - Vulnerability

Credits

Props @username, @username2, ...

Checklist:

I agree to follow this project's Code of Conduct.
I have updated the documentation accordingly.
I have added tests to cover my change.
All new and existing tests pass.

jjgrainger

Thanks @spacedmonkey looks good, left a couple of comments based on previous feedback in another PR

jjgrainger · 2023-04-28T15:17:35Z

 	public function set_up_environment() {
 		// Verify the nonce before continuing.
-		$valid_request = $this->verify_request( filter_input( INPUT_POST, 'nonce', FILTER_SANITIZE_FULL_SPECIAL_CHARS ) );
+		$this->validate_request();


I had originally approached this that way before but it was recommended to decouple this and handle the response outside of the verify/validate method. (see #97 (comment))

I don't think those two things are the same. This does change any of the logic, just moves it around.

Moving the logic into reusable functions makes sense, but to @jjgrainger's point we shouldn't couple the new utility methods to send JSON response data. The response should be handled separately by the AJAX callbacks, the new utility functions should rather throw an Exception or return WP_Error that can then be returned as JSON.

jjgrainger · 2023-04-28T15:20:19Z

+			Plugin_Request_Utility::initialize_runner();
+			$runner = Plugin_Request_Utility::get_runner();


The initialize_runner method handles other logic alongside creating the correct runner (and could handle more in the future). As we know we're in the AJAX context I think we should simply instantiate an AJAX_Runner as before rather than use the Plugin_Request_Utility methods here.

Then what is the point of Plugin_Request_Utility::get_runner(); then. Why not just create a AJAX_Runner here?

+1 to what @jjgrainger said, this is changing the logic from what it was before. We should create an AJAX_Runner here, not call Plugin_Request_Utility::initialize_runner() and Plugin_Request_Utility::get_runner(), as that also runs other logic.

jjgrainger · 2023-04-28T15:34:41Z

+			wp_send_json_error(
+				new WP_Error( 'invalid-runner', __( 'AJAX Runner was not initialized correctly.', 'plugin-check' ) ),
+				500
+			);


Similar to my other comment, we may want to return the WP_Error rather than send the JSON response here. (see #97 (comment))

Alternatively, we could throw an Exception instead and call get_runner in the try/catch block?

This is just the same logic that was there before. Why change it?

See https://github.com/10up/plugin-check/pull/157/files#r1180537801, the utility methods should have the responsibility of what their name is, not return a JSON response. For example this method should return the runner, or throw an exception if there is an error. The AJAX callbacks can then use try { ... } catch and return a JSON error response in case of an exception.

If we do that, that could mean repeated the same code every time this method is called. I am trying avoid lots of boilerplate code here.

felixarntz

@spacedmonkey Moving this logic to reusable methods is a great idea, however I agree with @jjgrainger that we should decouple those methods from sending JSON response data. The methods should just return what they're named for, or throw an exception if something goes wrong.

The AJAX callbacks can then catch the exception and return it as a JSON error response.

felixarntz · 2023-04-28T17:21:36Z

 	public function set_up_environment() {
 		// Verify the nonce before continuing.
-		$valid_request = $this->verify_request( filter_input( INPUT_POST, 'nonce', FILTER_SANITIZE_FULL_SPECIAL_CHARS ) );
+		$this->validate_request();


Moving the logic into reusable functions makes sense, but to @jjgrainger's point we shouldn't couple the new utility methods to send JSON response data. The response should be handled separately by the AJAX callbacks, the new utility functions should rather throw an Exception or return WP_Error that can then be returned as JSON.

felixarntz · 2023-04-28T17:23:46Z

+			Plugin_Request_Utility::initialize_runner();
+			$runner = Plugin_Request_Utility::get_runner();


+1 to what @jjgrainger said, this is changing the logic from what it was before. We should create an AJAX_Runner here, not call Plugin_Request_Utility::initialize_runner() and Plugin_Request_Utility::get_runner(), as that also runs other logic.

felixarntz · 2023-04-28T17:25:42Z

+			wp_send_json_error(
+				new WP_Error( 'invalid-runner', __( 'AJAX Runner was not initialized correctly.', 'plugin-check' ) ),
+				500
+			);


See https://github.com/10up/plugin-check/pull/157/files#r1180537801, the utility methods should have the responsibility of what their name is, not return a JSON response. For example this method should return the runner, or throw an exception if there is an error. The AJAX callbacks can then use try { ... } catch and return a JSON error response in case of an exception.

felixarntz · 2023-04-28T17:26:52Z

+		if ( is_wp_error( $valid_request ) ) {
+			wp_send_json_error( $valid_request, 403 );
+		}
+	}


See above. This method shouldn't be necessary at all. I would suggest we rather modify the verify_request() method to throw an exception instead of returning WP_Error. That way we can catch a potential exception with the same code that we would use also for the other methods, like get_runner() above.

spacedmonkey · 2023-05-04T10:42:54Z

+		$runner = Plugin_Request_Utility::get_runner();
+
+		if ( is_null( $runner ) ) {
+			$runner = new AJAX_Runner();


@felixarntz @jjgrainger By doing this, should we not also be calling the prepare method on this runner?

felixarntz

This looks almost good to go, however one thing needs to be addressed.

felixarntz · 2023-05-04T16:20:36Z

+
+		if ( ! ( $runner instanceof AJAX_Runner ) ) {
+			$runner = new AJAX_Runner();
+		}


This is a functional change we shouldn't make: This method should only instantiate a new AJAX_Runner if Plugin_Request_Utility::get_runner() returns null. Otherwise, if the runner returned from that method itself is not an AJAX_Runner, we should throw an exception and then the underlying AJAX callback should terminate with a JSON error, similar to how that works today.

Why would we want to do that? If it returns null, it is okay to create a new runner right? If get_runner returns a CLI_Runner, then error? Why error here? What is the benefit of erroring when we can just create an instance?

mukeshpanchal27

Thanks @spacedmonkey, Can we make util function for filter_input( INPUT_POST, 'nonce', FILTER_SANITIZE_FULL_SPECIAL_CHARS ) also as it used four time in this file also I left some feedbacks.

mukeshpanchal27 · 2023-05-17T09:12:39Z

 		);
 	}

+


Suggested change

Remove extra line.

mukeshpanchal27 · 2023-05-17T09:22:44Z

+	 * @return array
+	 */
+	protected function get_checks() {
+		$checks = filter_input( INPUT_POST, 'checks', FILTER_DEFAULT, FILTER_FORCE_ARRAY );


Similar function present in AJAX_Runner::get_check_slugs_param()

Refactor to remove repeated code.

a30e873

spacedmonkey self-assigned this Apr 28, 2023

spacedmonkey requested review from felixarntz, jjgrainger and mukeshpanchal27 April 28, 2023 11:40

jjgrainger requested changes Apr 28, 2023

View reviewed changes

felixarntz requested changes Apr 28, 2023

View reviewed changes

Just create instance.

9778ae2

spacedmonkey commented May 4, 2023

View reviewed changes

spacedmonkey and others added 2 commits May 4, 2023 11:54

Feedback.

f48c9df

Merge branch 'trunk' into fix/repeated-code

4fd86df

spacedmonkey requested review from felixarntz and jjgrainger May 4, 2023 14:42

felixarntz reviewed May 4, 2023

View reviewed changes

Merge branch 'trunk' into fix/repeated-code

afa4fab

mukeshpanchal27 reviewed May 17, 2023

View reviewed changes

spacedmonkey added 2 commits May 18, 2023 17:16

Merge branch 'trunk' into fix/repeated-code

4e99b9b

Merge branch 'trunk' into fix/repeated-code

06f1743

spacedmonkey closed this Jun 28, 2023

		Plugin_Request_Utility::initialize_runner();
		$runner = Plugin_Request_Utility::get_runner();

Conversation

spacedmonkey commented Apr 28, 2023

Description of the Change

How to test the Change

Changelog Entry

Credits

Checklist:

Uh oh!

jjgrainger 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!

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants