Skip to content

Conversation

CodeProKid
Copy link
Contributor

@CodeProKid CodeProKid commented Aug 11, 2021

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:

WP_CLI::add_hook( 'before_run_command', function() {
	$runner  = WP_CLI::get_runner();
	$command = $runner->find_command_to_run( $runner->arguments )[0];
	log( $command->get_name() );
} );

This works for commands invoked from the command line but doesn't work for commands invoked via WP_CLI::run_command with launch => false set. This is because run_command will call the Runner->run_command() method directly so the Runner 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 the before_run_command hook.

@CodeProKid CodeProKid requested a review from a team as a code owner August 11, 2021 18:21
@@ -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
Copy link
Contributor Author

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. 😆

Copy link
Member

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.

@janw-me
Copy link
Member

janw-me commented Aug 17, 2021

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.
I don't know what the best way to test those is.

@CodeProKid
Copy link
Contributor Author

@schlessera curious if you have any thoughts/feedback on this change?

@schlessera
Copy link
Member

I'm no expert on the behat tests, but now you only test the happy path not the 2 failure states.

Those are already being tested by checking for the return code. Those 2 failure states use WP_CLI::error(), which produces a non-zero return code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants