[Process] Added support for processes that need a TTY to run. #7566

Merged
merged 1 commit into from Apr 20, 2013

3 participants

@mcuadros

Added support for processes that need a TTY to run. This can be useful in scenarios where we need to open an editor and wait for the user (like "crontab -e" or "git commit"). The new methos "setTTY" can be used to control this.

Regards

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR
@canni canni commented on an outdated diff Apr 5, 2013
src/Symfony/Component/Process/Process.php
@@ -283,6 +256,12 @@ public function start($callback = null)
stream_set_blocking($pipe, false);
}
+
+ if ( $this->tty ) {
@canni
canni added a line comment Apr 5, 2013

We don't put whitespace around operands in if statement

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@canni canni commented on an outdated diff Apr 5, 2013
src/Symfony/Component/Process/Process.php
+ //Fix for PHP bug #51800: reading from STDOUT pipe hangs forever on Windows if the output is too big.
+ //Workaround for this problem is to use temporary files instead of pipes on Windows platform.
+ //@see https://bugs.php.net/bug.php?id=51800
+ if (defined('PHP_WINDOWS_VERSION_BUILD')) {
+ $this->fileHandles = array(
+ self::STDOUT => tmpfile(),
+ );
+ if (false === $this->fileHandles[self::STDOUT]) {
+ throw new RuntimeException('A temporary file could not be opened to write the process output to, verify that your TEMP environment variable is writable');
+ }
+ $this->readBytes = array(
+ self::STDOUT => 0,
+ );
+ $descriptors = array(array('pipe', 'r'), $this->fileHandles[self::STDOUT], array('pipe', 'w'));
+ } else {
+ if ( $this->tty ) {
@canni
canni added a line comment Apr 5, 2013

Same here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@canni canni commented on an outdated diff Apr 5, 2013
src/Symfony/Component/Process/Process.php
@@ -1092,4 +1136,4 @@ private function processFileHandles($callback, $closeEmptyHandles = false)
}
}
}
-}
+}
@canni
canni added a line comment Apr 5, 2013

Missing newline

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Apr 5, 2013
src/Symfony/Component/Process/Process.php
@@ -982,6 +985,47 @@ public function setEnhanceSigchildCompatibility($enhance)
return $this;
}
+ protected function getDescriptors() {
@stof
Symfony member
stof added a line comment Apr 5, 2013

the curly brace should be on its own line

@stof
Symfony member
stof added a line comment Apr 5, 2013

btw, why moving this code to a protected method ?

  • you use it in a single place, so it is not really useful and simply makes it harder to merge fixes between 2.2 and master.
  • helper methods should be private by default in Symfony (a protected method is an extension point for which we need to take BC into account)
@mcuadros
mcuadros added a line comment Apr 5, 2013

Should be private, i moved the code due the main function is too large and complex and hard to read, but yes will be hard to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Apr 5, 2013
src/Symfony/Component/Process/Process.php
+ protected function getDescriptors() {
+ //Fix for PHP bug #51800: reading from STDOUT pipe hangs forever on Windows if the output is too big.
+ //Workaround for this problem is to use temporary files instead of pipes on Windows platform.
+ //@see https://bugs.php.net/bug.php?id=51800
+ if (defined('PHP_WINDOWS_VERSION_BUILD')) {
+ $this->fileHandles = array(
+ self::STDOUT => tmpfile(),
+ );
+ if (false === $this->fileHandles[self::STDOUT]) {
+ throw new RuntimeException('A temporary file could not be opened to write the process output to, verify that your TEMP environment variable is writable');
+ }
+ $this->readBytes = array(
+ self::STDOUT => 0,
+ );
+ $descriptors = array(array('pipe', 'r'), $this->fileHandles[self::STDOUT], array('pipe', 'w'));
+ } else {
@stof
Symfony member
stof added a line comment Apr 5, 2013

Instead of using else here, you should return the array at the end of the if IMO

@mcuadros
mcuadros added a line comment Apr 5, 2013

Sure, you have reason. I just move the code without think twice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@stof stof and 1 other commented on an outdated diff Apr 5, 2013
...mfony/Component/Process/Tests/AbstractProcessTest.php
@@ -162,6 +162,17 @@ public function testExitCodeCommandFailed()
$this->assertGreaterThan(0, $process->getExitCode());
}
+ public function testTTYCommand()
+ {
+ if (defined('PHP_WINDOWS_VERSION_BUILD')) {
+ $this->markTestSkipped('Windows does have /dev/tty support');
+ }
+
+ $process = $this->getProcess('echo "a" >> /dev/null');
+ $process->setTTY(true);
+ $process->run();
@stof
Symfony member
stof added a line comment Apr 5, 2013

A test should have assertions (running phpunit in strict mode would mark the test as failing)

@mcuadros
mcuadros added a line comment Apr 5, 2013

Yep, this test just check the command works, because is very hard to test a interaction with the tty.
I just added the correct assertion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fabpot
Symfony member

Can you squash your commits please? As you merged master, it's not mergeable cleanly as is. Thanks.

@mcuadros

Done, i hope was enough, the diff looks ok, i made some "--force" commits.

Regards and thanks!

@fabpot fabpot added a commit that referenced this pull request Apr 20, 2013
@fabpot fabpot merged branch mcuadros/master (PR #7566)
This PR was merged into the master branch.

Discussion
----------

[Process] Added support for processes that need a TTY to run.

Added support for processes that need a TTY to run. This can be useful in scenarios where we need to open an editor and wait for the user (like "crontab -e" or "git commit"). The new methos "setTTY" can be used to control this.

Regards

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

Commits
-------

2d30fb3 [Process] Added support for processes that need a TTY to run.
ab43728
@fabpot fabpot merged commit 2d30fb3 into symfony:master Apr 20, 2013

1 check passed

Details default Scrutinizer: No Comments
@romainneutron

Hey @mcuadros

I propose to revert this in #10412
do you remember if you set this on specific purpose ?

After almost 1 year i dont remember why is marked as terminated.
But for sure has a reason :/

Anyway if all tests pass and the behaviour is better 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment