Add Plugin_Check_Command class #72
Conversation
vishalkakadiya
changed the title
|
@felixarntz @jjgrainger The unit tests are not yet completed, but the command's class is a bit large, so that you can review the CLI command code. I will complete the unit test for this soon. Thank you! 🙂 |
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@vishalkakadiya Left some feedback. Overall this looks solid.
There are a few things that we need to reconsider and discuss outside of this PR, e.g. how to better handle setup of the runner classes. But we can address that in a separate follow up PR later once we have figured out the approach.
vishalkakadiya
changed the title
jjgrainger
left a comment
jjgrainger
left a comment
There was a problem hiding this comment.
Thanks @vishalkakadiya a couple of small changes here but there is 1 part of this work I want to discuss with @felixarntz on next steps too.
joemcgill
left a comment
joemcgill
left a comment
There was a problem hiding this comment.
One suggestion, otherwise this is looking pretty good.
|
@joemcgill I have addressed your feedback on this PR and left one comment. You can review it now, thanks! |
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@jjgrainger This is almost good to go, there are just a few critical things missing around the runner setup, which we recently prepared for via #95.
mukeshpanchal27
left a comment
mukeshpanchal27
left a comment
There was a problem hiding this comment.
Thanks @jjgrainger, I left some minor update feedback, one of which related to the option in the command not working.
| /** | ||
| * Run plugin check. | ||
| * | ||
| * ## OPTIONS |
There was a problem hiding this comment.
@jjgrainger Is the options argument working for you? or will it be in an upcoming PR?
npm run wp-env run cli wp plugin check akismet --format=json it give me an error Error: Too many positional arguments: json
There was a problem hiding this comment.
Thanks @mukeshpanchal27 I experienced the same issue here but am uncertain what the cause is. I noticed that the docblock comment for the command wasn't correct for these options, however updating these has not resolved the issue. I'll keep looking into this.
There was a problem hiding this comment.
Since you're invoking WP-CLI in the container, I think you're missing an extra --. You need to use npm run wp-env run cli -- wp plugin check akismet --format=json
felixarntz
left a comment
felixarntz
left a comment
There was a problem hiding this comment.
@jjgrainger One more thing here, and one nit-pick, but should be quick to address. I'll give this my approval already since the tweaks should be trivial. Excited to see this shape!
| $runner->set_plugin( $plugin ); | ||
| $runner->set_check_slugs( $checks ); |
There was a problem hiding this comment.
These calls need to happen unconditionally, even if you're using the runner returned by Plugin_Request_Utility::get_runner. That is the entire reason why we implemented the validation in Abstract_Check_Runner.
| $checks = wp_parse_list( $options['checks'] ); | ||
|
|
||
| // Get the CLI Runner. | ||
| $runner = Plugin_Request_Utility::get_runner(); |
There was a problem hiding this comment.
Nit-pick: Not sure if this can realistically ever happen, but we should probably cater for it just in case: If this returns any object that is not a CLI_Runner, we should error out. For example, imagine something goes wrong in the object-cache.php invoked logic and then somehow this returns an AJAX_Runner here. Extremely unlikely, but possible based on the method implementation, so we may want to cater for that.
joemcgill
left a comment
joemcgill
left a comment
There was a problem hiding this comment.
I'm happy with the current iteration once @felixarntz's comments have been addressed.
|
Thanks all, I've updated the PR based on the latest feedback. Will merge this now |
Add Plugin_Check_Command class
Closes #30