In run-tests.sh, we are properly escaping most of the arguments, but not the path to the PHP binary. As a consequence, run-tests.sh cannot be used when the path to the PHP binary contains a space, as it is usually the case on Windows.

CommentFileSizeAuthor
#8 run-tests-escaping-752696-8.patch585 bytesTor Arne Thune
#1 05-752696-run-tests-escaping.patch836 bytesdamien tournoud

Comments

damien tournoud’s picture

damien tournoud commented
Status: Active » Needs review
StatusFileSize
new 05-752696-run-tests-escaping.patch836 bytes

Hopefully, this is easy to fix.

Tor Arne Thune’s picture

Tor Arne Thune commented
Status: Needs review » Closed (fixed)

It looks like this is fixed in Drupal 7.0.

/**
 * Run a single test (assume a Drupal bootstrapped environment).
 */
function simpletest_script_run_one_test($test_id, $test_class) {
  $test = new $test_class($test_id);
  $test->run();
  $info = $test->getInfo();

  $status = ((isset($test->results['#fail']) && $test->results['#fail'] > 0)
           || (isset($test->results['#exception']) && $test->results['#exception'] > 0) ? 'fail' : 'pass');
  simpletest_script_print($info['name'] . ' ' . _simpletest_format_summary_line($test->results) . "\n", simpletest_script_color_code($status));
}

/**
 * Execute a command to run batch of tests in separate process.
 */
function simpletest_script_command($concurrency, $test_id, $tests) {
  global $args, $php;

  $command = "$php ./scripts/{$args['script']} --url {$args['url']}";
  if ($args['color']) {
    $command .= ' --color';
  }
  $command .= " --php " . escapeshellarg($php) . " --concurrency $concurrency --test-id $test_id --execute-batch $tests";
  passthru($command);
}
Tor Arne Thune’s picture

Tor Arne Thune commented
Status: Closed (fixed) » Needs review

Sorry, I was wrong. Still a valid issue.

/**
 * Execute a command to run batch of tests in separate process.
 */
function simpletest_script_command($concurrency, $test_id, $tests) {
  global $args, $php;

  $command = "$php ./scripts/{$args['script']} --url {$args['url']}";
  if ($args['color']) {
    $command .= ' --color';
  }
  $command .= " --php " . escapeshellarg($php) . " --concurrency $concurrency --test-id $test_id --execute-batch $tests";
  passthru($command);
}
Tor Arne Thune’s picture

Tor Arne Thune commented

#1: 05-752696-run-tests-escaping.patch queued for re-testing.

Tor Arne Thune’s picture

Tor Arne Thune commented
Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7

Status: Needs review » Needs work

The last submitted patch, 05-752696-run-tests-escaping.patch, failed testing.

Tor Arne Thune’s picture

Tor Arne Thune commented
Status: Needs work » Needs review
StatusFileSize
new run-tests-escaping-752696-8.patch585 bytes

Re-roll of patch in #1.

Anonymous’s picture

Anonymous (not verified) commented
Issue tags: -Needs backport to D7

#8: run-tests-escaping-752696-8.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs backport to D7

The last submitted patch, run-tests-escaping-752696-8.patch, failed testing.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Closed (won't fix)

The file scripts/run-tests.sh doesnt exists anymore - hence the patch doesn't apply. Closing the issue.

droplet’s picture

droplet commented
Status: Closed (won't fix) » Closed (fixed)

It fixed in other issue and backported. #771448: Use proc_open() instead of pcntl_fork() in simpletest

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Closed (fixed) » Closed (duplicate)

Thanks @Christoffer Hallas.

Note that the file does actually still exist; it has been moved to core/scripts/run-tests.sh. However, @droplet found this other issue where the same bug was resolved: #771448: Use proc_open() instead of pcntl_fork() in simpletest

So, marking this as a duplicate of that issue.

xjm’s picture

xjm
she/her
Primary language English
commented

Thanks @Christoffer Hallas.

Note that the file does actually still exist; it has been moved to core/scripts/run-tests.sh. However, @droplet found this other issue where the same bug was resolved: #771448: Use proc_open() instead of pcntl_fork() in simpletest

So, marking this as a duplicate of that issue.