Util function to check if running plugin check command#161
Conversation
| } else { | ||
| return; | ||
| } |
There was a problem hiding this comment.
There is no other code after this return. I am 99% sure that, this is not needed.
|
@mukeshpanchal27 I played around with this ticket in https://github.com/10up/plugin-check/pull/152/files. I don't think the idea works, but it is still interesting to compare. |
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@mukeshpanchal27 Looks good so far, though I think a cleaner solution would be to keep using the CLI_Runner method we already have for that purpose.
| * | ||
| * @return bool Returns true if is a CLI request for the plugin check else false. | ||
| */ | ||
| public static function is_plugin_check() { |
There was a problem hiding this comment.
Instead of a new method in this general class (which is not specific to CLI at all), I suggest we use the CLI_Runner::is_plugin_check() method instead and make that static.
There was a problem hiding this comment.
I did this here in my testing. https://github.com/10up/plugin-check/blob/e5f89c4f8b76bfeeebd408671cbdb9fd82b1950c/cli.php
| @@ -32,15 +33,7 @@ class CLI_Runner extends Abstract_Check_Runner { | |||
| * @return bool Returns true if is an CLI request for the plugin check else false. | |||
| */ | |||
| public function is_plugin_check() { | |||
There was a problem hiding this comment.
See my other comment, I think making this method static makes more sense than introducing a similar method somewhere else that is not specific to CLI.
It's perfectly reasonable for this method to be static since it only checks something in the global scope and has nothing to do with an instance of this class.
There was a problem hiding this comment.
@mukeshpanchal27 @felixarntz should we also update the AJAX_Runner method to also be static for consistency? We would also need to update the Plugin_Request_Utility::initialize_runner method to use that static methods also.
| @@ -32,15 +33,7 @@ class CLI_Runner extends Abstract_Check_Runner { | |||
| * @return bool Returns true if is an CLI request for the plugin check else false. | |||
| */ | |||
| public function is_plugin_check() { | |||
There was a problem hiding this comment.
@mukeshpanchal27 @felixarntz should we also update the AJAX_Runner method to also be static for consistency? We would also need to update the Plugin_Request_Utility::initialize_runner method to use that static methods also.
|
Keep the static add back the public on the method. |
|
Another lint error. |
jjgrainger
left a comment
jjgrainger
left a comment
There was a problem hiding this comment.
Thanks @mukeshpanchal27 updates look good
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
Thanks @mukeshpanchal27, LGTM!
4 participants
Closes #111
Checklist: