Fix CommandRunnerTask options handling

CHANGELOG.md

@@ -5,6 +5,9 @@ Latest
 * [#190](https://github.com/cleverage/process-bundle/issues/190) Update quality stack: use Rector `withComposerBased()` sets (removed `SYMFONY_64` / `PHPUNIT_100` sets), declare used Symfony packages and PHPUnit range in composer.json, apply quality tools fixes
 * [#145](https://github.com/cleverage/process-bundle/issues/145) Add missing documentations: reference pages for every Task & Transformer, ConditionTrait & GenericTransformer, complete guides and cookbooks. Harmonize and fix existing documentation.
 
+## Fixes
+* [#192](https://github.com/cleverage/process-bundle/issues/192) Fix CommandRunnerTask: only pass the `options` option to `Process::setOptions()`, support string `commandline` through `Process::fromShellCommandline()`, validate option types. Update documentation, add tests.
+
 * v5.0
 -----
 

docs/reference/tasks/command_runner_task.md

@@ -27,11 +27,11 @@ Options
 
 | Code          | Type               | Required | Default                   | Description                                                                    |
 |---------------|--------------------|:--------:|---------------------------|--------------------------------------------------------------------------------|
-| `commandline` | `string\|array`    |  **X**   |                           | Command to run, as an array of arguments (recommended) or a string             |
+| `commandline` | `string\|array`    |  **X**   |                           | Command to run, as an array of arguments (recommended) or a string run by the shell (see Notes) |
 | `cwd`         | `string\|null`     |          | Symfony project directory | Working directory of the command                                               |
 | `env`         | `array\|null`      |          | `null`                    | Environment variables of the command (`null` inherits the current environment) |
 | `timeout`     | `int\|float\|null` |          | `60`                      | Timeout in seconds (`null` disables it)                                        |
-| `options`     | `mixed`            |          | `null`                    | Unused by the task itself, see Notes                                           |
+| `options`     | `array\|null`      |          | `null`                    | Passed to `Process::setOptions()`: `blocking_pipes`, `create_process_group`, `create_new_console` |
 
 Examples
 --------
@@ -48,10 +48,22 @@ count_lines:
   outputs: [next_task]
 ```
 
+* Use shell features (pipes, environment variables)
+
+```yaml
+# Task configuration level
+count_errors:
+  service: '@CleverAge\ProcessBundle\Task\Process\CommandRunnerTask'
+  options:
+    commandline: 'grep "$PATTERN" | wc -l'
+    env:
+      PATTERN: 'error'
+  outputs: [next_task]
+```
+
 Notes
 -----
 
-The task calls `Process::setOptions()` with **all** its resolved options (`commandline`, `cwd`, `env`, `timeout` and
-`options`). Recent versions of `symfony/process` only accept `blocking_pipes`, `create_process_group` and
-`create_new_console` there and throw a `LogicException` for any other key, which makes the task fail before the
-command is started.
+An array `commandline` is run with `new Process()`: each argument is escaped and no shell is involved. A string
+`commandline` is run with `Process::fromShellCommandline()`: it is interpreted by the shell, so never build it from
+untrusted input.

src/Task/Process/CommandRunnerTask.php

@@ -32,14 +32,18 @@ public function __construct(
     public function execute(ProcessState $state): void
     {
         $options = $this->getOptions($state);
-        $process = new Process(
-            $options['commandline'],
+        $arguments = [
             $options['cwd'],
             $options['env'],
             $state->getInput(),
             $options['timeout'],
-        );
-        $process->setOptions($options);
+        ];
+        $process = \is_array($options['commandline'])
+            ? new Process($options['commandline'], ...$arguments)
+            : Process::fromShellCommandline($options['commandline'], ...$arguments);
+        if (null !== $options['options']) {
+            $process->setOptions($options['options']);
+        }
         $process->mustRun();
         $state->setOutput($process->getOutput());
     }
@@ -56,5 +60,9 @@ protected function configureOptions(OptionsResolver $resolver): void
                 'options' => null,
             ]
         );
+        $resolver->setAllowedTypes('cwd', ['null', 'string']);
+        $resolver->setAllowedTypes('env', ['null', 'array']);
+        $resolver->setAllowedTypes('timeout', ['null', 'int', 'float']);
+        $resolver->setAllowedTypes('options', ['null', 'array']);
     }
 }

tests/Task/Process/CommandRunnerTaskTest.php

@@ -0,0 +1,127 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the CleverAge/ProcessBundle package.
+ *
+ * Copyright (c) Clever-Age
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace CleverAge\ProcessBundle\Tests\Task\Process;
+
+use CleverAge\ProcessBundle\Configuration\ProcessConfiguration;
+use CleverAge\ProcessBundle\Configuration\TaskConfiguration;
+use CleverAge\ProcessBundle\Context\ContextualOptionResolver;
+use CleverAge\ProcessBundle\Model\ProcessHistory;
+use CleverAge\ProcessBundle\Model\ProcessState;
+use CleverAge\ProcessBundle\Task\Process\CommandRunnerTask;
+use PHPUnit\Framework\TestCase;
+use Symfony\Component\HttpKernel\KernelInterface;
+use Symfony\Component\OptionsResolver\Exception\InvalidOptionsException;
+use Symfony\Component\Process\Exception\ProcessFailedException;
+
+#[\PHPUnit\Framework\Attributes\CoversClass(CommandRunnerTask::class)]
+#[\PHPUnit\Framework\Attributes\CoversMethod(CommandRunnerTask::class, 'execute')]
+#[\PHPUnit\Framework\Attributes\CoversMethod(CommandRunnerTask::class, 'configureOptions')]
+#[\PHPUnit\Framework\Attributes\UsesClass(ProcessConfiguration::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(TaskConfiguration::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ContextualOptionResolver::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ProcessHistory::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ProcessState::class)]
+class CommandRunnerTaskTest extends TestCase
+{
+    public function testExecuteWithArrayCommandline(): void
+    {
+        $state = $this->createState(['commandline' => ['echo', 'hello']]);
+
+        $this->createTask()->execute($state);
+
+        $this->assertSame("hello\n", $state->getOutput());
+    }
+
+    public function testExecuteWithStringCommandline(): void
+    {
+        $state = $this->createState(['commandline' => 'echo "hello world" | tr a-z A-Z']);
+
+        $this->createTask()->execute($state);
+
+        $this->assertSame("HELLO WORLD\n", $state->getOutput());
+    }
+
+    public function testInputIsPassedToCommand(): void
+    {
+        $state = $this->createState(['commandline' => ['cat']]);
+        $state->setInput('from input');
+
+        $this->createTask()->execute($state);
+
+        $this->assertSame('from input', $state->getOutput());
+    }
+
+    public function testCwdAndEnvOptions(): void
+    {
+        $state = $this->createState([
+            'commandline' => 'echo "$FOO" && pwd',
+            'cwd' => sys_get_temp_dir(),
+            'env' => ['FOO' => 'bar'],
+        ]);
+
+        $this->createTask()->execute($state);
+
+        $this->assertSame('bar'.\PHP_EOL.realpath(sys_get_temp_dir()).\PHP_EOL, $state->getOutput());
+    }
+
+    public function testProcessOptionsArePassedToProcess(): void
+    {
+        $state = $this->createState([
+            'commandline' => ['echo', 'hello'],
+            'options' => ['create_new_console' => false],
+        ]);
+
+        $this->createTask()->execute($state);
+
+        $this->assertSame("hello\n", $state->getOutput());
+    }
+
+    public function testInvalidProcessOptionsType(): void
+    {
+        $state = $this->createState([
+            'commandline' => ['echo', 'hello'],
+            'options' => 'invalid',
+        ]);
+
+        $this->expectException(InvalidOptionsException::class);
+        $this->createTask()->execute($state);
+    }
+
+    public function testFailingCommandThrows(): void
+    {
+        $state = $this->createState(['commandline' => ['false']]);
+
+        $this->expectException(ProcessFailedException::class);
+        $this->createTask()->execute($state);
+    }
+
+    private function createTask(): CommandRunnerTask
+    {
+        $kernel = $this->createStub(KernelInterface::class);
+        $kernel->method('getProjectDir')->willReturn(sys_get_temp_dir());
+
+        return new CommandRunnerTask($kernel);
+    }
+
+    private function createState(array $options): ProcessState
+    {
+        $processConfiguration = new ProcessConfiguration('test', []);
+        $state = new ProcessState($processConfiguration, new ProcessHistory($processConfiguration));
+        $state->setContextualOptionResolver(new ContextualOptionResolver());
+        $state->setContext([]);
+        $state->setTaskConfiguration(new TaskConfiguration('command', CommandRunnerTask::class, $options));
+
+        return $state;
+    }
+}