Update Runner classes to validate checks #95
Conversation
jjgrainger
added
[Type] Enhancement
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@jjgrainger This looks close, there are a few things that need to be fixed though around sanitization and validation. Also a few method renaming suggestions that just came up in my review based on the logic you're using, I like your idea of using the abstract methods only to get the parameter value but not more.
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@jjgrainger Thanks, the code looks great. A few last naming quirks, but I'll already approve this as following the renaming it can be merged.
| * | ||
| * @throws Exception Thrown if the plugin main file does not match the original request. | ||
| */ | ||
| public function set_plugin_slug( $plugin_slug ) { |
There was a problem hiding this comment.
Currently the logic in this checks against the plugin parameter, which can be either a plugin slug or a plugin basename, so the name of this method, the parameter, and also the internal property name is misleading.
We should probably align with the rest of the new method names here to clarify that this is simply the plugin param. In other words, let's call this set_plugin( $plugin ), and let's clarify on both the method parameter and the $this->plugin property declaration that the value can be either a plugin slug or a plugin basename.
This effectively means wherever we use this value we need to run it through Plugin_Request_Utility::get_plugin_basename_from_input to sanitize it to a basename. I think that's okay since it only happens in one place anyway.
| return $this->checks; | ||
| } | ||
|
|
||
| $plugin_slug = isset( $this->plugin_slug ) ? $this->plugin_slug : $this->get_plugin_param(); |
There was a problem hiding this comment.
See above regarding naming, let's use $plugin instead of $plugin_slug to account for the ambiguity of what this value can contain (it's not necessarily a slug).
includes/Checker/CLI_Runner.php
Outdated
| $plugin_slug = isset( $_SERVER['argv'][3] ) ? $_SERVER['argv'][3] : ''; | ||
|
|
||
| $this->checks = new Checks( WP_PLUGIN_DIR . '/' . $plugin_file ); | ||
| if ( empty( $plugin_slug ) ) { | ||
| throw new Exception( | ||
| __( 'Invalid plugin slug: Plugin slug must not be empty.', 'plugin-check' ) | ||
| ); | ||
| } | ||
|
|
||
| return $this->checks; | ||
| return $plugin_slug; |
There was a problem hiding this comment.
See above regarding naming of this. Also can you similarly adjust the exception message to refer to "plugin parameter" rather than "plugin slug"?
includes/Checker/AJAX_Runner.php
Outdated
| protected function get_plugin_param() { | ||
| if ( ! isset( $_REQUEST['plugin'] ) ) { | ||
| throw new Exception( | ||
| __( 'Invalid plugin slug: Plugin slug must not be empty.', 'plugin-check' ) |
There was a problem hiding this comment.
See other similar comment, can you adjust the exception message to refer to "plugin parameter" rather than "plugin slug"?
Addresses acceptance criteria in #93
Closes #93