Make sure subcommand @alias definition respects @when definition #5730

mrsdizzie · 2023-01-19T22:19:40Z

Currently subcommand @alias value is not passed to register_early_invoke. This fixes #5724

…finition

danielbachhuber

This is great, @mrsdizzie — thank you!

Following up on your comment, I think the best place for @alias and @when parsing is inside of add_command():

wp-cli/php/class-wp-cli.php

Line 480 in a584b19

public static function add_command( $name, $callable, $args = [] ) {

In fact, CompositeCommand probably shouldn't call register_early_invoke() directly:

wp-cli/php/WP_CLI/Dispatcher/CompositeCommand.php

Line 44 in a2f0250

WP_CLI::get_runner()->register_early_invoke( $when_to_invoke, $this );

@alias is a very old feature introduced for backcompat when renaming commands f467e3c #186

The broken behavior you're experiencing is simply because no one ever thought of this particular scenario.

mrsdizzie · 2023-01-26T01:01:54Z

Thanks!

There is a lot of abstraction im not familiar with here so I'm a bit lost, but it doesn't look like add_command() processes any of the DocBlock directly. It looks like it calls CommandFactory::create which calls create_subcommand or create_composite_command where that can happen.

You can pass $args to add_command() which can contain those same DocBlock definitions, but most uses of add::command don't do that they just pass a callable which has a DocBlock (like the example test).

I wouldn't know where to move that functionally into add_command

danielbachhuber · 2023-01-26T22:59:44Z

@mrsdizzie No worries, sounds good. I'll dive into this when I have some time, but can't make any commitments to when it might be. I've assigned the issue to myself though.

danielbachhuber · 2023-02-21T00:07:06Z

Here's some code that works, but I don't think is correct:

diff --git a/features/command.feature b/features/command.feature
index 23b1a255..3bc320f4 100644
--- a/features/command.feature
+++ b/features/command.feature
@@ -1519,6 +1519,7 @@ Feature: WP-CLI Commands
     wp custom
     """
 
+  @daniel
   Scenario: subcommand alias should respect @when definition
     Given an empty directory
     And a custom-cmd.php file:
diff --git a/php/WP_CLI/Dispatcher/CompositeCommand.php b/php/WP_CLI/Dispatcher/CompositeCommand.php
index fc711a9e..912270ca 100644
--- a/php/WP_CLI/Dispatcher/CompositeCommand.php
+++ b/php/WP_CLI/Dispatcher/CompositeCommand.php
@@ -41,7 +41,7 @@ class CompositeCommand {
 
 		$when_to_invoke = $docparser->get_tag( 'when' );
 		if ( $when_to_invoke ) {
-			WP_CLI::get_runner()->register_early_invoke( $when_to_invoke, $this );
+			WP_CLI::get_runner()->register_early_invoke( $when_to_invoke, $this, $docparser->get_tag( 'alias' ) );
 		}
 	}
 
diff --git a/php/WP_CLI/Dispatcher/Subcommand.php b/php/WP_CLI/Dispatcher/Subcommand.php
index 837a0071..f4dd9c10 100644
--- a/php/WP_CLI/Dispatcher/Subcommand.php
+++ b/php/WP_CLI/Dispatcher/Subcommand.php
@@ -25,6 +25,10 @@ class Subcommand extends CompositeCommand {
 		$this->when_invoked = $when_invoked;
 
 		$this->alias = $docparser->get_tag( 'alias' );
+		if ( 'foo' === $name ) {
+			var_dump( $docparser );
+			var_dump( $docparser->get_tag( 'alias' ) );
+		}
 
 		$this->synopsis = $docparser->get_synopsis();
 		if ( ! $this->synopsis && $this->longdesc ) {
diff --git a/php/WP_CLI/Runner.php b/php/WP_CLI/Runner.php
index afa9bc51..2c29b17c 100644
--- a/php/WP_CLI/Runner.php
+++ b/php/WP_CLI/Runner.php
@@ -89,9 +89,19 @@ class Runner {
 	 *
 	 * @param string $when Named execution hook
 	 * @param Subcommand $command
+	 * @param string $alias
 	 */
-	public function register_early_invoke( $when, $command ) {
-		$this->early_invoke[ $when ][] = array_slice( Dispatcher\get_path( $command ), 1 );
+	public function register_early_invoke( $when, $command, $alias = '' ) {
+		$cmd_path                      = array_slice( Dispatcher\get_path( $command ), 1 );
+		$this->early_invoke[ $when ][] = $cmd_path;
+		if ( 'test' === $cmd_path[0] ) {
+			var_dump( $command );
+		}
+		if ( $alias ) {
+			array_pop( $cmd_path );
+			$cmd_path[] = $alias;
+			$this->early_invoke[ $when ][] = $cmd_path;
+		}
 	}
 
 	/**

$ wp --require=test-cmd.php test bar
object(WP_CLI\Dispatcher\Subcommand)#18 (9) {
  ["alias":"WP_CLI\Dispatcher\Subcommand":private]=>
  NULL
  ["when_invoked":"WP_CLI\Dispatcher\Subcommand":private]=>
  NULL
  ["name":protected]=>
  string(3) "foo"
  ["shortdesc":protected]=>
  string(4) "test"
  ["longdesc":protected]=>
  string(0) ""
  ["synopsis":protected]=>
  NULL
  ["docparser":protected]=>
  object(WP_CLI\DocParser)#16 (1) {
    ["doc_comment":protected]=>
    string(39) "test

@alias bar

@when before_wp_load
"
  }
  ["parent":protected]=>
  object(WP_CLI\Dispatcher\CompositeCommand)#14 (7) {
    ["name":protected]=>
    string(4) "test"
    ["shortdesc":protected]=>
    string(0) ""
    ["longdesc":protected]=>
    string(0) ""
    ["synopsis":protected]=>
    NULL
    ["docparser":protected]=>
    object(WP_CLI\DocParser)#13 (1) {
      ["doc_comment":protected]=>
      string(0) ""
    }
    ["parent":protected]=>
    object(WP_CLI\Dispatcher\RootCommand)#10 (7) {
      ["name":protected]=>
      string(2) "wp"
      ["shortdesc":protected]=>
      string(42) "Manage WordPress through the command-line."
      ["longdesc":protected]=>
      NULL
      ["synopsis":protected]=>
      NULL
      ["docparser":protected]=>
      NULL
      ["parent":protected]=>
      bool(false)
      ["subcommands":protected]=>
      array(0) {
      }
    }
    ["subcommands":protected]=>
    array(0) {
    }
  }
  ["subcommands":protected]=>
  array(0) {
  }
}
object(WP_CLI\DocParser)#16 (1) {
  ["doc_comment":protected]=>
  string(39) "test

@alias bar

@when before_wp_load
"
}
string(3) "bar"
Hello

The alias the subcommand object is somehow empty when it's passed into register_early_invoke:

danielbachhuber · 2023-02-21T00:11:17Z

Ah, this does the trick:

diff --git a/features/command.feature b/features/command.feature
index 23b1a255..3bc320f4 100644
--- a/features/command.feature
+++ b/features/command.feature
@@ -1519,6 +1519,7 @@ Feature: WP-CLI Commands
     wp custom
     """
 
+  @daniel
   Scenario: subcommand alias should respect @when definition
     Given an empty directory
     And a custom-cmd.php file:
diff --git a/php/WP_CLI/Dispatcher/Subcommand.php b/php/WP_CLI/Dispatcher/Subcommand.php
index 837a0071..9ad47de3 100644
--- a/php/WP_CLI/Dispatcher/Subcommand.php
+++ b/php/WP_CLI/Dispatcher/Subcommand.php
@@ -20,12 +20,13 @@ class Subcommand extends CompositeCommand {
 	private $when_invoked;
 
 	public function __construct( $parent, $name, $docparser, $when_invoked ) {
+
+		$this->alias = $docparser->get_tag( 'alias' );
+
 		parent::__construct( $parent, $name, $docparser );
 
 		$this->when_invoked = $when_invoked;
 
-		$this->alias = $docparser->get_tag( 'alias' );
-
 		$this->synopsis = $docparser->get_synopsis();
 		if ( ! $this->synopsis && $this->longdesc ) {
 			$this->synopsis = self::extract_synopsis( $this->longdesc );
diff --git a/php/WP_CLI/Runner.php b/php/WP_CLI/Runner.php
index afa9bc51..c2e3f455 100644
--- a/php/WP_CLI/Runner.php
+++ b/php/WP_CLI/Runner.php
@@ -91,7 +91,13 @@ class Runner {
 	 * @param Subcommand $command
 	 */
 	public function register_early_invoke( $when, $command ) {
-		$this->early_invoke[ $when ][] = array_slice( Dispatcher\get_path( $command ), 1 );
+		$cmd_path                      = array_slice( Dispatcher\get_path( $command ), 1 );
+		$this->early_invoke[ $when ][] = $cmd_path;
+		if ( $command->get_alias() ) {
+			array_pop( $cmd_path );
+			$cmd_path[] = $command->get_alias();
+			$this->early_invoke[ $when ][] = $cmd_path;
+		}
 	}
 
 	/**

The problem was that register_early_invoke() is called in parent::__construct(), and $this->alias` hadn't been defined yet.

Move the docparser call before parent::__construct() so we detect and later handle any alias properly.

mrsdizzie · 2023-02-21T14:57:04Z

@danielbachhuber cool! that is pretty similar to what I had started with ( moving the docparser call above the parent::__construct() call). Looks good to me thanks for looking into it

mrsdizzie · 2023-04-06T18:38:52Z

@danielbachhuber could this be considered for 2.8.0 milestone since it is coming up?

danielbachhuber · 2023-04-06T19:33:49Z

@mrsdizzie Yeah, I'm fine to land this. There's probably a much broader refactor that could happen to make this more "correct", but I don't think it's worth the effort if the existing tests pass just fine.

Add test to verify both a subcommand and @alias use the same @when de…

40ffa92

…finition

mrsdizzie requested a review from a team as a code owner January 19, 2023 22:19

danielbachhuber reviewed Jan 23, 2023

View reviewed changes

Merge branch 'main' into fix-5724

72a31ce

Properly handle @alias tag in register_early_invoke

4103929

Move the docparser call before parent::__construct() so we detect and later handle any alias properly.

danielbachhuber requested a review from schlessera February 27, 2023 13:52

Merge branch 'main' into fix-5724

1f790dc

danielbachhuber added this to the 2.8.0 milestone Apr 6, 2023

danielbachhuber added scope:framework bug labels Apr 6, 2023

Merge branch 'main' into fix-5724

a17976a

danielbachhuber self-requested a review April 6, 2023 20:19

danielbachhuber approved these changes Apr 6, 2023

View reviewed changes

danielbachhuber merged commit 22badb1 into wp-cli:main Apr 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make sure subcommand @alias definition respects @when definition #5730

Make sure subcommand @alias definition respects @when definition #5730

Uh oh!

mrsdizzie commented Jan 19, 2023 •

edited

Loading

Uh oh!

danielbachhuber left a comment

Uh oh!

mrsdizzie commented Jan 26, 2023

Uh oh!

danielbachhuber commented Jan 26, 2023

Uh oh!

danielbachhuber commented Feb 21, 2023

Uh oh!

danielbachhuber commented Feb 21, 2023

Uh oh!

mrsdizzie commented Feb 21, 2023

Uh oh!

mrsdizzie commented Apr 6, 2023

Uh oh!

danielbachhuber commented Apr 6, 2023

Uh oh!

Uh oh!

Make sure subcommand @alias definition respects @when definition #5730

Make sure subcommand @alias definition respects @when definition #5730

Uh oh!

Conversation

mrsdizzie commented Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danielbachhuber left a comment

Choose a reason for hiding this comment

Uh oh!

mrsdizzie commented Jan 26, 2023

Uh oh!

danielbachhuber commented Jan 26, 2023

Uh oh!

danielbachhuber commented Feb 21, 2023

Uh oh!

danielbachhuber commented Feb 21, 2023

Uh oh!

mrsdizzie commented Feb 21, 2023

Uh oh!

mrsdizzie commented Apr 6, 2023

Uh oh!

danielbachhuber commented Apr 6, 2023

Uh oh!

Uh oh!

mrsdizzie commented Jan 19, 2023 •

edited

Loading