mnapoli / silly

Silly CLI micro-framework based on Symfony Console
MIT License
922 stars 52 forks source link

Using ArrayInput instead of StringInput breaks sub-command arguments #69

Closed mcaskill closed 1 year ago

mcaskill commented 1 year ago

Tested in mnapoli/silly v1.8.2, with symfony/console v6.2.5, via laravel/valet v4.1.2.

While working on a fix for Valet, I've discovered that sub-command arguments no longer work as a result of #66. A comment on #17 appears to be the same issue I've discovered.

For context, Valet has among others the following commands link and isolate. When executing the link command, if the --isolate argument is provided, Valet will run the isolate command via Silly's Application::runCommand method:

$app->command('link [name] [--secure] [--isolate]', function ($name, $secure, $isolate) {
    # ...

    if ($isolate) {
        if (Site::phpRcVersion($name)) {
            $this->runCommand('isolate');
        } else {

If a custom name is provided for the link, it is not passed along to the isolate command which ends up using the current directory's name, which ends up causing an error because it won't find a matching link:

$app->command('isolate [phpVersion] [--site=]', function (OutputInterface $output, $phpVersion, $site = null) {
    if (! $site) {
        $site = basename(getcwd());
    }

This is what I'm attempting to fix in Valet. The solution to this is fairly simple: provide the --site= option to the isolate sub-command with the given link name:

 if (Site::phpRcVersion($name)) {
-    $this->runCommand('isolate');
+    $this->runCommand('isolate --site="'.$name.'"');
 } else {

Unfortunately, this does not work. The isolate command is executed but with no arguments.

In Silly's Application::runCommand() method, the return value of $input->getArguments() is an empty array. The reason the arguments are empty is because the input is only parsed if it has a definition. The definition is assigned to the input by the command in Symfony's Command::run() method.

The Symfony documentation, cited in #66, shows that ArrayInput is used for explicitly defined arguments. Here our arguments are defined via a string input (of tokens) which need a definition to be correctly parsed as arguments.

Reading again the quoted error message in #66, its messaging is suspiciously formulated:

Too many arguments to "xxx" command, expected arguments "yyy".

Looking up the logic behind it in Symfony's ArgvInput::parseArgument() method gives me the impression their sub-command only accepts a single argument but they are passing multiple arguments (maybe an unquoted string). That's what triggers the above error message. By adding new ArrayInput($input->getArguments(), you are skipping all arguments which "fixes" that error message.

In conclusion, Silly should revert the changes introduced by #66 in v1.8.1:

- return $command->run(new ArrayInput($input->getArguments()), $output ?: new NullOutput());
+ return $command->run($input, $output ?: new NullOutput());

The sub-command will correctly parse the given arguments from the string input.