Skip to content

Conversation

coccoinomane
Copy link
Contributor

Hello!
Why: For __invoke commands, the before_invoke and after_invoke hooks run twice, creating potential unexpected behavior.
Solution: I propose an easy fix: pass the command name to the callback, so that it can choose when to run.
If you think it is a viable solution, I'll add tests :-)
Cheers,
Cocco

@coccoinomane coccoinomane requested a review from a team as a code owner December 8, 2022 17:17
@coccoinomane
Copy link
Contributor Author

PS: Passing args and assoc_args to the callback would add even more context, making it possible to do validation of arguments in the before_invoke hook.

I haven't done so in the PR in order to avoid too many modifications, but if there's interest I'll make another PR or edit this one.

Thanks,
Cocco

Copy link
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, @coccoinomane !

It certainly seems relatively straightforward. Can you share a bit more detail on how you discovered this issue?

@coccoinomane
Copy link
Contributor Author

Ciao @danielbachhuber, and thank you for your reply!

I had a command structured in this way:

<?php

class TestHook
{
    public function __invoke( $args )
    {
        WP_CLI::line( "In command" );
    }
}

WP_CLI::add_command(
    'test hook', // two words
    'TestHook',
    [
        'before_invoke' => function() {
            WP_CLI::line( "In before_invoke" );
        },
        'after_invoke' => function() {
            WP_CLI::line( "In after_invoke" );
        }
    ]
);

When I invoked the command, I was suprised getting the following output:

$ wp test my hook
In before_invoke
In before_invoke
In command
In after_invoke
In after_invoke

Please note that by changing the command's name to a single word (e.g. testhook instead of test hook), it works fine.

The problem originates here. The easiest fix would be to just remove the WP_CLI::do_hook( "before_invoke:{$parent}" ); line, but I am not sure what the side effects would be.

@danielbachhuber
Copy link
Member

Thanks for the clarification, @coccoinomane !

I think the backwards-compat approach approach makes most sense too. The downside risk of unexpected explosions outweighs any upside of making this absolutely correct, in my opinion.

I'll flag for a review from @schlessera too.

@coccoinomane
Copy link
Contributor Author

Sure, thanks 👍

To add context, I am using the hooks in this new project > https://github.com/Idearia/wp-cli-command

The idea is to register hooks by simply overriding a method 🙂

@schlessera schlessera added this to the 2.8.0 milestone Dec 9, 2022
@danielbachhuber danielbachhuber self-requested a review December 9, 2022 11:38
@danielbachhuber danielbachhuber merged commit 4b1d84e into wp-cli:main Dec 9, 2022
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