Skip to content

Commit e856f33

Browse files
njoubert-cleveragenjoubert-cleverage
authored
Merge pull request #193 from cleverage/192
#192 - Fix CommandRunnerTask options handling
2 parents e70a620 + 28a48db commit e856f33

4 files changed

Lines changed: 160 additions & 10 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,9 @@ Latest
55
* [#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
66
* [#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.
77

8+
## Fixes
9+
* [#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.
10+
811
* v5.0
912
-----
1013

‎docs/reference/tasks/command_runner_task.md‎

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,11 +27,11 @@ Options
2727

2828
| Code | Type | Required | Default | Description |
2929
|---------------|--------------------|:--------:|---------------------------|--------------------------------------------------------------------------------|
30-
| `commandline` | `string\|array` | **X** | | Command to run, as an array of arguments (recommended) or a string |
30+
| `commandline` | `string\|array` | **X** | | Command to run, as an array of arguments (recommended) or a string run by the shell (see Notes) |
3131
| `cwd` | `string\|null` | | Symfony project directory | Working directory of the command |
3232
| `env` | `array\|null` | | `null` | Environment variables of the command (`null` inherits the current environment) |
3333
| `timeout` | `int\|float\|null` | | `60` | Timeout in seconds (`null` disables it) |
34-
| `options` | `mixed` | | `null` | Unused by the task itself, see Notes |
34+
| `options` | `array\|null` | | `null` | Passed to `Process::setOptions()`: `blocking_pipes`, `create_process_group`, `create_new_console` |
3535

3636
Examples
3737
--------
@@ -48,10 +48,22 @@ count_lines:
4848
outputs: [next_task]
4949
```
5050
51+
* Use shell features (pipes, environment variables)
52+
53+
```yaml
54+
# Task configuration level
55+
count_errors:
56+
service: '@CleverAge\ProcessBundle\Task\Process\CommandRunnerTask'
57+
options:
58+
commandline: 'grep "$PATTERN" | wc -l'
59+
env:
60+
PATTERN: 'error'
61+
outputs: [next_task]
62+
```
63+
5164
Notes
5265
-----
5366
54-
The task calls `Process::setOptions()` with **all** its resolved options (`commandline`, `cwd`, `env`, `timeout` and
55-
`options`). Recent versions of `symfony/process` only accept `blocking_pipes`, `create_process_group` and
56-
`create_new_console` there and throw a `LogicException` for any other key, which makes the task fail before the
57-
command is started.
67+
An array `commandline` is run with `new Process()`: each argument is escaped and no shell is involved. A string
68+
`commandline` is run with `Process::fromShellCommandline()`: it is interpreted by the shell, so never build it from
69+
untrusted input.

‎src/Task/Process/CommandRunnerTask.php‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,14 +32,18 @@ public function __construct(
3232
public function execute(ProcessState $state): void
3333
{
3434
$options = $this->getOptions($state);
35-
$process = new Process(
36-
$options['commandline'],
35+
$arguments = [
3736
$options['cwd'],
3837
$options['env'],
3938
$state->getInput(),
4039
$options['timeout'],
41-
);
42-
$process->setOptions($options);
40+
];
41+
$process = \is_array($options['commandline'])
42+
? new Process($options['commandline'], ...$arguments)
43+
: Process::fromShellCommandline($options['commandline'], ...$arguments);
44+
if (null !== $options['options']) {
45+
$process->setOptions($options['options']);
46+
}
4347
$process->mustRun();
4448
$state->setOutput($process->getOutput());
4549
}
@@ -56,5 +60,9 @@ protected function configureOptions(OptionsResolver $resolver): void
5660
'options' => null,
5761
]
5862
);
63+
$resolver->setAllowedTypes('cwd', ['null', 'string']);
64+
$resolver->setAllowedTypes('env', ['null', 'array']);
65+
$resolver->setAllowedTypes('timeout', ['null', 'int', 'float']);
66+
$resolver->setAllowedTypes('options', ['null', 'array']);
5967
}
6068
}

‎tests/Task/Process/CommandRunnerTaskTest.php‎

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,127 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/*
6+
* This file is part of the CleverAge/ProcessBundle package.
7+
*
8+
* Copyright (c) Clever-Age
9+
*
10+
* For the full copyright and license information, please view the LICENSE
11+
* file that was distributed with this source code.
12+
*/
13+
14+
namespace CleverAge\ProcessBundle\Tests\Task\Process;
15+
16+
use CleverAge\ProcessBundle\Configuration\ProcessConfiguration;
17+
use CleverAge\ProcessBundle\Configuration\TaskConfiguration;
18+
use CleverAge\ProcessBundle\Context\ContextualOptionResolver;
19+
use CleverAge\ProcessBundle\Model\ProcessHistory;
20+
use CleverAge\ProcessBundle\Model\ProcessState;
21+
use CleverAge\ProcessBundle\Task\Process\CommandRunnerTask;
22+
use PHPUnit\Framework\TestCase;
23+
use Symfony\Component\HttpKernel\KernelInterface;
24+
use Symfony\Component\OptionsResolver\Exception\InvalidOptionsException;
25+
use Symfony\Component\Process\Exception\ProcessFailedException;
26+
27+
#[\PHPUnit\Framework\Attributes\CoversClass(CommandRunnerTask::class)]
28+
#[\PHPUnit\Framework\Attributes\CoversMethod(CommandRunnerTask::class, 'execute')]
29+
#[\PHPUnit\Framework\Attributes\CoversMethod(CommandRunnerTask::class, 'configureOptions')]
30+
#[\PHPUnit\Framework\Attributes\UsesClass(ProcessConfiguration::class)]
31+
#[\PHPUnit\Framework\Attributes\UsesClass(TaskConfiguration::class)]
32+
#[\PHPUnit\Framework\Attributes\UsesClass(ContextualOptionResolver::class)]
33+
#[\PHPUnit\Framework\Attributes\UsesClass(ProcessHistory::class)]
34+
#[\PHPUnit\Framework\Attributes\UsesClass(ProcessState::class)]
35+
class CommandRunnerTaskTest extends TestCase
36+
{
37+
public function testExecuteWithArrayCommandline(): void
38+
{
39+
$state = $this->createState(['commandline' => ['echo', 'hello']]);
40+
41+
$this->createTask()->execute($state);
42+
43+
$this->assertSame("hello\n", $state->getOutput());
44+
}
45+
46+
public function testExecuteWithStringCommandline(): void
47+
{
48+
$state = $this->createState(['commandline' => 'echo "hello world" | tr a-z A-Z']);
49+
50+
$this->createTask()->execute($state);
51+
52+
$this->assertSame("HELLO WORLD\n", $state->getOutput());
53+
}
54+
55+
public function testInputIsPassedToCommand(): void
56+
{
57+
$state = $this->createState(['commandline' => ['cat']]);
58+
$state->setInput('from input');
59+
60+
$this->createTask()->execute($state);
61+
62+
$this->assertSame('from input', $state->getOutput());
63+
}
64+
65+
public function testCwdAndEnvOptions(): void
66+
{
67+
$state = $this->createState([
68+
'commandline' => 'echo "$FOO" && pwd',
69+
'cwd' => sys_get_temp_dir(),
70+
'env' => ['FOO' => 'bar'],
71+
]);
72+
73+
$this->createTask()->execute($state);
74+
75+
$this->assertSame('bar'.\PHP_EOL.realpath(sys_get_temp_dir()).\PHP_EOL, $state->getOutput());
76+
}
77+
78+
public function testProcessOptionsArePassedToProcess(): void
79+
{
80+
$state = $this->createState([
81+
'commandline' => ['echo', 'hello'],
82+
'options' => ['create_new_console' => false],
83+
]);
84+
85+
$this->createTask()->execute($state);
86+
87+
$this->assertSame("hello\n", $state->getOutput());
88+
}
89+
90+
public function testInvalidProcessOptionsType(): void
91+
{
92+
$state = $this->createState([
93+
'commandline' => ['echo', 'hello'],
94+
'options' => 'invalid',
95+
]);
96+
97+
$this->expectException(InvalidOptionsException::class);
98+
$this->createTask()->execute($state);
99+
}
100+
101+
public function testFailingCommandThrows(): void
102+
{
103+
$state = $this->createState(['commandline' => ['false']]);
104+
105+
$this->expectException(ProcessFailedException::class);
106+
$this->createTask()->execute($state);
107+
}
108+
109+
private function createTask(): CommandRunnerTask
110+
{
111+
$kernel = $this->createStub(KernelInterface::class);
112+
$kernel->method('getProjectDir')->willReturn(sys_get_temp_dir());
113+
114+
return new CommandRunnerTask($kernel);
115+
}
116+
117+
private function createState(array $options): ProcessState
118+
{
119+
$processConfiguration = new ProcessConfiguration('test', []);
120+
$state = new ProcessState($processConfiguration, new ProcessHistory($processConfiguration));
121+
$state->setContextualOptionResolver(new ContextualOptionResolver());
122+
$state->setContext([]);
123+
$state->setTaskConfiguration(new TaskConfiguration('command', CommandRunnerTask::class, $options));
124+
125+
return $state;
126+
}
127+
}

0 commit comments

Comments
 (0)