-
Notifications
You must be signed in to change notification settings - Fork 998
Pass in args to before_run_command hook #5554
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.
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
Pass in args to before_run_command hook #5554
Conversation
@@ -71,3 +71,34 @@ Feature: Tests `WP_CLI::add_hook()` | |||
`add_hook()` to the `before_invoke` is working. | |||
""" | |||
And the return code should be 0 | |||
|
|||
Scenario: Add callback to the `before_run_command` with args |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm definitely open to a better testing approach here to make sure the hook args are flowing through to the callback properly. 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing looks good to me.
Love the fix, I'm always up for better hooks. I'm no expert on the behat tests, but now you only test the happy path not the 2 failure states. |
@schlessera curious if you have any thoughts/feedback on this change? |
Those are already being tested by checking for the return code. Those 2 failure states use |
This PR passes in some args to the
before_run_command
hook so callbacks have some more context to base their logic off of.The reason for proposing this change is because of a particular use case I have where I'm implementing some logging to track CLI commands that are being run in our codebase. I currently have an implementation that looks something like:
This works for commands invoked from the command line but doesn't work for commands invoked via
WP_CLI::run_command
withlaunch => false
set. This is becauserun_command
will call theRunner->run_command()
method directly so theRunner
instance doesn't get bootstrapped again with the proper context.To get around this, I'm proposing we pass the params from the
run_command
method into thebefore_run_command
hook.