Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

[Process] Add ability to force unescaping + tweaks (code & test) #6793

Closed
wants to merge 7 commits into from

3 participants

@vicb
Q A
Bug fix? no
New feature? yes
BC breaks? yes
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR Not sure if we want to document this ? If yes, I'll update if accepted.

Rationale:
'ogr2ogr' '-f GeoJSON' would not work
'ogr2ogr' -f GeoJSON would work

The first commit has the interesting updates, other commits are tweaks only. Everything should be squashed before the merge.

vicb added some commits
@vicb vicb [Process] Ability to prevent escaping in the ProcessBuilder
'ogr2ogr' 'out' 'in' '-f GeoJSON' would not work
'ogr2ogr' 'out' 'in' -f GeoJSON would work
2a0a6fb
@vicb vicb [Process] Tweaks 5547e9b
@vicb vicb [Process] Update the CHANGELOG 71b6dbe
@stof

missing closing curly brace for the previous method (a bad conflict resolution ?)

Well everything is fine globaly but the commit are splitted to make it easier to understand and the split is not perfect !

Collaborator

yeah, I saw it when going back to the PR instead of individual commits

@vicb

@stof how could you have missed this one, tired ? ;)

@vicb

What about using 0instead of null to disable the timeout, it would be more consistent with other things (ie cache).
We could support BC with (integer) $timeout.

@vicb

@fabpot could this be in 2.2 ?

@vicb

pong @fabpot, it would really help one of my projects !

@romainneutron
Collaborator

@vicb you have to consider -f and GeoJSON has two arguments. Running 'ogr2ogr' '-f' 'GeoJSON' should work fine.

@vicb

@romainneutron you are absolutely right, thanks.

@vicb vicb closed this
@vicb vicb deleted the branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 18, 2013
  1. @vicb

    [Process] Ability to prevent escaping in the ProcessBuilder

    vicb authored
    'ogr2ogr' 'out' 'in' '-f GeoJSON' would not work
    'ogr2ogr' 'out' 'in' -f GeoJSON would work
  2. @vicb

    [Process] Tweaks

    vicb authored
  3. @vicb

    [Process] Update the CHANGELOG

    vicb authored
  4. @vicb

    [Process] self-stofing

    vicb authored
Commits on Jan 22, 2013
  1. @vicb
  2. @vicb

    [Process] Cleanup

    vicb authored
Commits on Jan 31, 2013
  1. @vicb
This page is out of date. Refresh to see the latest.
View
4 src/Symfony/Component/Process/CHANGELOG.md
@@ -4,6 +4,10 @@ CHANGELOG
2.2.0
-----
+ * [BC BREAK] modified the timeout methods: you should now set the timeout to 0
+ instead of null to disable it. Using null for setters still works, however the
+ getters will return 0 instead of null when the timeout is disabled.
+ * [BC BREAK] added a second argument to ProcessBuilder::add() to force unescaping.
* added ProcessBuilder::setArguments() to reset the arguments on a builder
* added a way to retrieve the standard and error output incrementally
* added Process:restart()
View
42 src/Symfony/Component/Process/Process.php
@@ -136,8 +136,6 @@ public function __construct($commandline, $cwd = null, array $env = null, $stdin
foreach ($env as $key => $value) {
$this->env[(binary) $key] = (binary) $value;
}
- } else {
- $this->env = null;
}
$this->stdin = $stdin;
$this->setTimeout($timeout);
@@ -292,7 +290,7 @@ public function start($callback = null)
$w = $writePipes;
$e = null;
- $n = @stream_select($r, $w, $e, $this->timeout);
+ $n = @stream_select($r, $w, $e, 0 === $this->timeout ? null : $this->timeout);
if (false === $n) {
break;
@@ -340,15 +338,15 @@ public function start($callback = null)
*
* @return Process The new process
*
- * @throws \RuntimeException When process can't be launch or is stopped
- * @throws \RuntimeException When process is already running
+ * @throws RuntimeException When process can't be launch or is stopped
+ * @throws RuntimeException When process is already running
*
* @see start()
*/
public function restart($callback = null)
{
if ($this->isRunning()) {
- throw new \RuntimeException('Process is already running');
+ throw new RuntimeException('Process is already running');
}
$process = clone $this;
@@ -384,7 +382,7 @@ public function wait($callback = null)
$w = null;
$e = null;
- if (false === $n = @stream_select($r, $w, $e, $this->timeout)) {
+ if (false === $n = @stream_select($r, $w, $e, 0 === $this->timeout ? null : $this->timeout)) {
$lastError = error_get_last();
// stream_select returns false when the `select` system call is interrupted by an incoming signal
@@ -407,7 +405,7 @@ public function wait($callback = null)
if (strlen($data) > 0) {
// last exit code is output and caught to work around --enable-sigchild
if (3 == $type) {
- $this->fallbackExitcode = (int) $data;
+ $this->fallbackExitcode = (integer) $data;
} else {
call_user_func($callback, $type == 1 ? self::OUT : self::ERR, $data);
}
@@ -424,9 +422,7 @@ public function wait($callback = null)
throw new RuntimeException(sprintf('The process stopped because of a "%s" signal.', $this->processInformation['stopsig']));
}
- $time = 0;
- while ($this->isRunning() && $time < 1000000) {
- $time += 1000;
+ for ($time = 0; $this->isRunning() && $time < 1000000; $time += 1000) {
usleep(1000);
}
@@ -703,12 +699,10 @@ public function getStatus()
*/
public function stop($timeout = 10)
{
- $timeoutMicro = (int) $timeout*10E6;
+ $timeoutMicro = (integer) $timeout * 10E6;
if ($this->isRunning()) {
proc_terminate($this->process);
- $time = 0;
- while (1 == $this->isRunning() && $time < $timeoutMicro) {
- $time += 1000;
+ for ($time = 0; $this->isRunning() && $time < $timeoutMicro; $time += 1000) {
usleep(1000);
}
@@ -779,7 +773,7 @@ public function setCommandLine($commandline)
/**
* Gets the process timeout.
*
- * @return integer|null The timeout in seconds or null if it's disabled
+ * @return integer The timeout in seconds or 0 if it's disabled
*/
public function getTimeout()
{
@@ -789,9 +783,7 @@ public function getTimeout()
/**
* Sets the process timeout.
*
- * To disable the timeout, set this value to null.
- *
- * @param integer|null $timeout The timeout in seconds
+ * @param integer $timeout The timeout in seconds (0 to disable)
*
* @return self The current Process instance
*
@@ -799,20 +791,12 @@ public function getTimeout()
*/
public function setTimeout($timeout)
{
- if (null === $timeout) {
- $this->timeout = null;
+ $this->timeout = (integer) $timeout;
- return $this;
- }
-
- $timeout = (integer) $timeout;
-
- if ($timeout < 0) {
+ if ($this->timeout < 0) {
throw new InvalidArgumentException('The timeout value must be a valid positive integer.');
}
- $this->timeout = $timeout;
-
return $this;
}
View
105 src/Symfony/Component/Process/ProcessBuilder.php
@@ -29,10 +29,14 @@ class ProcessBuilder
private $options;
private $inheritEnv;
+ /**
+ * @param string[] $arguments The command line arguments. They will always get escaped.
+ *
+ * @see add
+ */
public function __construct(array $arguments = array())
{
- $this->arguments = $arguments;
-
+ $this->setArguments($arguments);
$this->timeout = 60;
$this->options = array();
$this->env = array();
@@ -45,31 +49,59 @@ public static function create(array $arguments = array())
}
/**
- * Adds an unescaped argument to the command string.
+ * Adds an argument to the command string.
+ *
+ * The argument will get escaped unless you instruct to do so by setting the
+ * $escape flag to false. Setting the $escape flag to false is not recommended
+ * unless you understand the implications.
*
- * @param string $argument A command argument
+ * @param string $argument A command argument
+ * @param Boolean $escape Whether the argument should be escaped (default)
*
* @return ProcessBuilder
+ *
+ * @throws InvalidArgumentException When the argument is not a string
*/
- public function add($argument)
+ public function add($argument, $escape = true)
{
- $this->arguments[] = $argument;
+ if (!is_string($argument)) {
+ throw new InvalidArgumentException('The argument must be a string.');
+ }
+
+ $this->arguments[] = array($argument, $escape);
return $this;
}
/**
- * @param array $arguments
+ * @param string[] $arguments
*
* @return ProcessBuilder
+ *
+ * @throws InvalidArgumentException When any argument is not a string
*/
public function setArguments(array $arguments)
{
- $this->arguments = $arguments;
+ $this->arguments = array_map(function($arg) {
+ if (!is_string($arg)) {
+ throw new InvalidArgumentException('Arguments must be strings.');
+ }
+
+ return array($arg, true);
+ },
+ $arguments
+ );
return $this;
}
+ /**
+ * Sets the working directory for the process.
+ *
+ * @param string $cwd The working directory
+ *
+ * @return ProcessBuilder
+ */
public function setWorkingDirectory($cwd)
{
$this->cwd = $cwd;
@@ -77,13 +109,28 @@ public function setWorkingDirectory($cwd)
return $this;
}
+ /**
+ * Sets whether to inherit from the environment variables.
+ *
+ * @param Boolean $inheritEnv
+ *
+ * @return ProcessBuilder
+ */
public function inheritEnvironmentVariables($inheritEnv = true)
{
- $this->inheritEnv = $inheritEnv;
+ $this->inheritEnv = (Boolean) $inheritEnv;
return $this;
}
+ /**
+ * Sets the value of an environment variable.
+ *
+ * @param string $name
+ * @param string $value
+ *
+ * @return ProcessBuilder
+ */
public function setEnv($name, $value)
{
$this->env[$name] = $value;
@@ -91,6 +138,13 @@ public function setEnv($name, $value)
return $this;
}
+ /**
+ * Sets the content for STDIN.
+ *
+ * @param string $stdin The content for STDIN
+ *
+ * @return ProcessBuilder
+ */
public function setInput($stdin)
{
$this->stdin = $stdin;
@@ -101,33 +155,31 @@ public function setInput($stdin)
/**
* Sets the process timeout.
*
- * To disable the timeout, set this value to null.
- *
- * @param integer|null
+ * @param integer The timeout in seconds, 0 to disable.
*
* @return ProcessBuilder
*
- * @throws InvalidArgumentException
+ * @throws InvalidArgumentException When the timeout value is invalid (<0)
*/
public function setTimeout($timeout)
{
- if (null === $timeout) {
- $this->timeout = null;
-
- return $this;
- }
+ $this->timeout = (integer) $timeout;
- $timeout = (integer) $timeout;
-
- if ($timeout < 0) {
+ if ($this->timeout < 0) {
throw new InvalidArgumentException('The timeout value must be a valid positive integer.');
}
- $this->timeout = $timeout;
-
return $this;
}
+ /**
+ * Sets an option for proc_open
+ *
+ * @param string $name
+ * @param string $value
+ *
+ * @return ProcessBuilder
+ */
public function setOption($name, $value)
{
$this->options[$name] = $value;
@@ -135,6 +187,11 @@ public function setOption($name, $value)
return $this;
}
+ /**
+ * @return Process
+ *
+ * @throws Exception\LogicException When no arguments have been specified
+ */
public function getProcess()
{
if (!count($this->arguments)) {
@@ -143,7 +200,7 @@ public function getProcess()
$options = $this->options;
- $script = implode(' ', array_map('escapeshellarg', $this->arguments));
+ $script = implode(' ', array_map(function($arg) { return $arg[1] ? escapeshellarg($arg[0]) : $arg[0]; }, $this->arguments));
if ($this->inheritEnv) {
$env = $this->env ? $this->env + $_ENV : null;
View
6 src/Symfony/Component/Process/Tests/AbstractProcessTest.php
@@ -37,13 +37,13 @@ public function testNegativeTimeoutFromSetter()
$p->setTimeout(-1);
}
- public function testNullTimeout()
+ public function testDisableTimeout()
{
$p = $this->getProcess('');
$p->setTimeout(10);
- $p->setTimeout(null);
+ $p->setTimeout(0);
- $this->assertNull($p->getTimeout());
+ $this->assertEquals(0, $p->getTimeout());
}
/**
View
105 src/Symfony/Component/Process/Tests/ProcessBuilderTest.php
@@ -15,69 +15,62 @@
class ProcessBuilderTest extends \PHPUnit_Framework_TestCase
{
- /**
- * @test
- */
- public function shouldInheritEnvironmentVars()
+ public function testShouldInheritEnvironmentVars()
{
$snapshot = $_ENV;
$_ENV = $expected = array('foo' => 'bar');
- $pb = new ProcessBuilder();
- $pb->add('foo')->inheritEnvironmentVariables();
- $proc = $pb->getProcess();
+ $proc = ProcessBuilder::create()
+ ->add('foo')
+ ->inheritEnvironmentVariables()
+ ->getProcess()
+ ;
$this->assertNull($proc->getEnv(), '->inheritEnvironmentVariables() copies $_ENV');
$_ENV = $snapshot;
}
- /**
- * @test
- */
- public function shouldInheritAndOverrideEnvironmentVars()
+ public function testShouldInheritAndOverrideEnvironmentVars()
{
$snapshot = $_ENV;
$_ENV = array('foo' => 'bar', 'bar' => 'baz');
$expected = array('foo' => 'foo', 'bar' => 'baz');
- $pb = new ProcessBuilder();
- $pb->add('foo')->inheritEnvironmentVariables()
- ->setEnv('foo', 'foo');
- $proc = $pb->getProcess();
+ $proc= ProcessBuilder::create()
+ ->add('foo')
+ ->inheritEnvironmentVariables()
+ ->setEnv('foo', 'foo')
+ ->getProcess()
+ ;
$this->assertEquals($expected, $proc->getEnv(), '->inheritEnvironmentVariables() copies $_ENV');
$_ENV = $snapshot;
}
- /**
- * @test
- */
- public function shouldInheritEnvironmentVarsByDefault()
+ public function testShouldInheritEnvironmentVarsByDefault()
{
- $pb = new ProcessBuilder();
- $proc = $pb->add('foo')->getProcess();
+ $proc = ProcessBuilder::create()
+ ->add('foo')
+ ->getProcess()
+ ;
$this->assertNull($proc->getEnv());
}
- /**
- * @test
- */
- public function shouldNotReplaceExplicitlySetVars()
+ public function testShouldNotReplaceExplicitlySetVars()
{
$snapshot = $_ENV;
$_ENV = array('foo' => 'bar');
$expected = array('foo' => 'baz');
- $pb = new ProcessBuilder();
- $pb
+ $proc = ProcessBuilder::create()
->setEnv('foo', 'baz')
->inheritEnvironmentVariables()
->add('foo')
+ ->getProcess()
;
- $proc = $pb->getProcess();
$this->assertEquals($expected, $proc->getEnv(), '->inheritEnvironmentVariables() copies $_ENV');
@@ -89,30 +82,58 @@ public function shouldNotReplaceExplicitlySetVars()
*/
public function testNegativeTimeoutFromSetter()
{
- $pb = new ProcessBuilder();
- $pb->setTimeout(-1);
+ ProcessBuilder::create()->setTimeout(-1);
}
public function testNullTimeout()
{
- $pb = new ProcessBuilder();
- $pb->setTimeout(10);
- $pb->setTimeout(null);
-
- $r = new \ReflectionObject($pb);
- $p = $r->getProperty('timeout');
- $p->setAccessible(true);
+ $proc = ProcessBuilder::create()
+ ->add('foo')
+ ->setTimeout(10)
+ ->setTimeout(0)
+ ->getProcess()
+ ;
- $this->assertNull($p->getValue($pb));
+ $this->assertEquals(0, $proc->getTimeout());
}
public function testShouldSetArguments()
{
- $pb = new ProcessBuilder(array('initial'));
- $pb->setArguments(array('second'));
+ $proc = ProcessBuilder::create(array('initial'))
+ ->setArguments(array('second'))
+ ->getProcess()
+ ;
+
+ $this->assertContains('second', $proc->getCommandLine());
+ }
+
+ /**
+ * @expectedException \Symfony\Component\Process\Exception\InvalidArgumentException
+ */
+ public function testAddAcceptStringOnly()
+ {
+ ProcessBuilder::create()->add(array());
+ }
- $proc = $pb->getProcess();
+ /**
+ * @expectedException \Symfony\Component\Process\Exception\InvalidArgumentException
+ */
+ public function testSetArgumentsAcceptStringOnly()
+ {
+ ProcessBuilder::create()->setArguments(array(array()));
+ }
+
+ public function testAddUnescapedArguments()
+ {
+ $proc = ProcessBuilder::create(array('fooEscaped'))
+ ->add('barEscaped')
+ ->add('FooUnescaped', false)
+ ->getProcess()
+ ;
- $this->assertContains("second", $proc->getCommandLine());
+ $this->assertContains(
+ escapeshellarg('fooEscaped') . ' ' . escapeshellarg('barEscaped') . ' FooUnescaped',
+ $proc->getCommandLine()
+ );
}
}
Something went wrong with that request. Please try again.