Skip to content

Refactor to remove repeated code. #157

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
spacedmonkey wants to merge 7 commits into from
Closed

Refactor to remove repeated code. #157

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a30e873
Refactor to remove repeated code.
spacedmonkey Apr 28, 2023
9778ae2
Just create instance.
spacedmonkey May 4, 2023
f48c9df
Feedback.
spacedmonkey May 4, 2023
4fd86df
Merge branch 'trunk' into fix/repeated-code
spacedmonkey May 4, 2023
afa4fab
Merge branch 'trunk' into fix/repeated-code
spacedmonkey May 9, 2023
4e99b9b
Merge branch 'trunk' into fix/repeated-code
spacedmonkey May 18, 2023
06f1743
Merge branch 'trunk' into fix/repeated-code
spacedmonkey Jun 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 51 additions & 48 deletions includes/Admin/Admin_AJAX.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,22 +94,10 @@ public function set_up_environment() {
if ( is_wp_error( $valid_request ) ) {
wp_send_json_error( $valid_request, 403 );
}
$runner = Plugin_Request_Utility::get_runner();

if ( is_null( $runner ) ) {
$runner = new AJAX_Runner();
}

// Make sure we are using the correct runner instance.
if ( ! ( $runner instanceof AJAX_Runner ) ) {
wp_send_json_error(
new WP_Error( 'invalid-runner', __( 'AJAX Runner was not initialized correctly.', 'plugin-check' ) ),
500
);
}

$checks = filter_input( INPUT_POST, 'checks', FILTER_DEFAULT, FILTER_FORCE_ARRAY );
$plugin = filter_input( INPUT_POST, 'plugin', FILTER_SANITIZE_FULL_SPECIAL_CHARS );
$runner = $this->get_runner();
$checks = $this->get_checks();
$plugin = $this->get_plugin();

try {
$runner->set_check_slugs( $checks );
Expand Down Expand Up @@ -189,22 +177,9 @@ public function get_checks_to_run() {
wp_send_json_error( $valid_request, 403 );
}

$checks = filter_input( INPUT_POST, 'checks', FILTER_DEFAULT, FILTER_FORCE_ARRAY );
$checks = is_null( $checks ) ? array() : $checks;
$plugin = filter_input( INPUT_POST, 'plugin', FILTER_SANITIZE_FULL_SPECIAL_CHARS );
$runner = Plugin_Request_Utility::get_runner();

if ( is_null( $runner ) ) {
$runner = new AJAX_Runner();
}

// Make sure we are using the correct runner instance.
if ( ! ( $runner instanceof AJAX_Runner ) ) {
wp_send_json_error(
new WP_Error( 'invalid-runner', __( 'AJAX Runner was not initialized correctly.', 'plugin-check' ) ),
403
);
}
$runner = $this->get_runner();
$checks = $this->get_checks();
$plugin = $this->get_plugin();

try {
$runner->set_check_slugs( $checks );
Expand Down Expand Up @@ -240,23 +215,9 @@ public function run_checks() {
wp_send_json_error( $valid_request, 403 );
}

$runner = Plugin_Request_Utility::get_runner();

if ( is_null( $runner ) ) {
$runner = new AJAX_Runner();
}

// Make sure we are using the correct runner instance.
if ( ! ( $runner instanceof AJAX_Runner ) ) {
wp_send_json_error(
new WP_Error( 'invalid-runner', __( 'AJAX Runner was not initialized correctly.', 'plugin-check' ) ),
500
);
}

$checks = filter_input( INPUT_POST, 'checks', FILTER_DEFAULT, FILTER_FORCE_ARRAY );
$checks = is_null( $checks ) ? array() : $checks;
$plugin = filter_input( INPUT_POST, 'plugin', FILTER_SANITIZE_FULL_SPECIAL_CHARS );
$runner = $this->get_runner();
$checks = $this->get_checks();
$plugin = $this->get_plugin();

try {
$runner->set_check_slugs( $checks );
Expand All @@ -278,6 +239,48 @@ public function run_checks() {
);
}


Copy link
Copy Markdown
Member

@mukeshpanchal27 mukeshpanchal27 May 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

Remove extra line.

/**
* Get the AJAX_Runner runner.
*
* @since n.e.x.t
*
* @return AJAX_Runner
*/
protected function get_runner() {
$runner = Plugin_Request_Utility::get_runner();

if ( ! ( $runner instanceof AJAX_Runner ) ) {
$runner = new AJAX_Runner();
}
Copy link
Copy Markdown
Member

@felixarntz felixarntz May 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@spacedmonkey spacedmonkey May 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


return $runner;
}

/**
* Get array of checks to run.
*
* @since n.e.x.t
*
* @return array
*/
protected function get_checks() {
$checks = filter_input( INPUT_POST, 'checks', FILTER_DEFAULT, FILTER_FORCE_ARRAY );
Copy link
Copy Markdown
Member

@mukeshpanchal27 mukeshpanchal27 May 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar function present in AJAX_Runner::get_check_slugs_param()


return is_null( $checks ) ? array() : (array) $checks;
}

/**
* Get requested plugin.
*
* @since n.e.x.t
*
* @return string
*/
protected function get_plugin() {
return filter_input( INPUT_POST, 'plugin', FILTER_SANITIZE_FULL_SPECIAL_CHARS );
}

/**
* Verify the request.
*
Expand Down