Skip to content

Fix CommandRunnerTask options handling - #187

Closed
Tmalboeuf-CA wants to merge 1 commit into
cleverage:mainfrom
Tmalboeuf-CA:fix/command-runner-options
Closed

Tmalboeuf-CA wants to merge 1 commit into
cleverage:mainfrom
Tmalboeuf-CA:fix/command-runner-options

Conversation

@Tmalboeuf-CA

Copy link
Copy Markdown
Collaborator

Description

CommandRunnerTask could not be used at all: every execution failed.

  • The task passed its whole options array to Process::setOptions(). That method only accepts blocking_pipes, create_process_group and create_new_console, so every run threw LogicException: Invalid option "cwd" passed to "Symfony\Component\Process\Process::setOptions()".
  • A string commandline is allowed by the options resolver, but new Process() only accepts an array. It threw a TypeError.

This PR:

  • passes only the options option to Process::setOptions(), and only when it is set;
  • runs string commandlines through Process::fromShellCommandline(), so pipes and environment variables work ('grep "$PATTERN" | wc -l');
  • validates the types of cwd (null|string), env (null|array), timeout (null|int|float) and options (null|array), so a wrong value is reported when options are resolved instead of failing inside Symfony Process;
  • adds the missing reference documentation (docs/reference/tasks/command_runner_task.md) and links it from docs/index.md.

Tested with 7 new unit tests (array and string commandlines, stdin input, cwd/env, process options, invalid option type, failing command). They all fail on main and pass with this fix. The full suite, PHPStan and PHP-CS-Fixer pass. Also checked end to end in a Symfony 7.4 app.

Requirements

  • Documentation updates
    • Reference
    • Cookbooks
    • Changelog
  • Unit tests

Breaking changes

None. Every configuration of this task failed before this change, so no working setup can break. The new type validation only rejects values that already crashed.

@njoubert-cleverage

Copy link
Copy Markdown
Member

Hello @Tmalboeuf-CA , could you provide a simple example of not working code ?

@Tmalboeuf-CA

Copy link
Copy Markdown
Collaborator Author

@njoubert-cleverage
Here is a minimal process that fails on main, Symfony 7.4 / PHP 8.5:

clever_age_process:
    configurations:
        demo.command_runner:
            tasks:
                run:
                    service: '@CleverAge\ProcessBundle\Task\Process\CommandRunnerTask'
                    options:
                        commandline: ['echo', 'hello']
                    outputs: [debug]
                debug:
                    service: '@CleverAge\ProcessBundle\Task\Debug\DebugTask'
$ bin/console cleverage:process:execute demo.command_runner
Starting process 'demo.command_runner'...
CRITICAL [cleverage_process_task] Invalid option "cwd" passed to "Symfony\Component\Process\Process::setOptions()".
Supported options are "blocking_pipes", "create_process_group", "create_new_console".

The whole resolved options array (commandline, cwd, env, timeout, options) is passed to Process::setOptions() (CommandRunnerTask.php#L42), which only accepts blocking_pipes, create_process_group and create_new_console. So every configuration fails, whatever the command.

A string commandline, although allowed by setAllowedTypes('commandline', ['string', 'array']), fails even earlier:

                    options:
                        commandline: 'echo hello'
CRITICAL [cleverage_process_task] Symfony\Component\Process\Process::__construct(): Argument #1 ($command)
must be of type array, string given, called in src/Task/Process/CommandRunnerTask.php on line 35

With this PR, both processes output "hello\n" and succeed. The same cases are covered by the unit tests in tests/Task/Process/CommandRunnerTaskTest.php (testExecuteWithArrayCommandline, testExecuteWithStringCommandline).

The whole task options array was passed to Process::setOptions(), which
only accepts blocking_pipes, create_process_group and create_new_console:
every execution failed with 'Invalid option "cwd"'. A string
commandline, although allowed, also failed with a TypeError.

- Pass only the `options` option to Process::setOptions()
- Run string commandlines through Process::fromShellCommandline()
- Validate cwd, env, timeout and options types
- Update the reference documentation, add unit tests

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants