Skip to content

[Process][Tests] Prove process fail with chained commands #5592

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Sep 28, 2012

Conversation

lyrixx
Copy link
Member

@lyrixx lyrixx commented Sep 25, 2012

Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: no
Fixes the following tickets: -
Todo: Fix that
License of the code: MIT

This PR is against 2.1 branch. Previous PR was #5575

This PR try to hiligh a regression in Process component.

$process = new Process("echo -n 1 && echo -n 1");
// or $process = new Process("echo -n 1 ; echo -n 1");
$process->run();
var_dump('11' == $process->getOutput()); // false, 
var_dump($process->getOutput()); // '1', 

This test failed because of PR #5543 ; see 7bafc69#L0R233

@romainneutron
Copy link
Contributor

You've to revert the change that causes the fail (ie: remove https://github.com/symfony/symfony/blob/2.1/src/Symfony/Component/Process/Process.php#L233)

@romainneutron
Copy link
Contributor

BTW, removing this line re-open #5030

@stof
Copy link
Member

stof commented Sep 25, 2012

@lyrixx please add a commit reverting the addition of exec in the case of sigchild not being used (only this addition, not the full commit you linked) as it should fix your test.

@stof
Copy link
Member

stof commented Sep 25, 2012

@fabpot btw, this regression is quite important. As I said in the previous PR, it impacts composer in a bunch of places.

@romainneutron
Copy link
Contributor

You reverted too much things, you just had to remove line 233

@stof
Copy link
Member

stof commented Sep 25, 2012

@lyrixx I explicitly asked you to revert only the exec addition for the case without sigchild.

@lyrixx
Copy link
Member Author

lyrixx commented Sep 25, 2012

@stof Sorry, i fixed that.

@romainneutron
Copy link
Contributor

@lyrixx just remove the two last commit, edit Process.php and remove line 233

@lyrixx
Copy link
Member Author

lyrixx commented Sep 25, 2012

@romainneutron I think it's ok now.

@romainneutron
Copy link
Contributor

yep it's good :)

fabpot added a commit that referenced this pull request Sep 28, 2012
Commits
-------

27b2df9 [Process] Fixed bug introduced by 7bafc69.
7a955c0 [Process][Tests] Prove process fail (Add more test case)
598dcf3 [Process][Tests] Prove process fail

Discussion
----------

[Process][Tests] Prove process fail with chained commands

Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: no
Fixes the following tickets: -
Todo: Fix that
License of the code: MIT

This PR is against 2.1 branch. Previous PR was #5575

This PR try to hiligh a regression in Process component.

``` php
$process = new Process("echo -n 1 && echo -n 1");
// or $process = new Process("echo -n 1 ; echo -n 1");
$process->run();
var_dump('11' == $process->getOutput()); // false,
var_dump($process->getOutput()); // '1',
```

This test failed because of PR #5543 ; see 7bafc69#L0R233

---------------------------------------------------------------------------

by romainneutron at 2012-09-25T13:05:45Z

You've to revert the change that causes the fail (ie: remove https://github.com/symfony/symfony/blob/2.1/src/Symfony/Component/Process/Process.php#L233)

---------------------------------------------------------------------------

by romainneutron at 2012-09-25T13:06:56Z

BTW, removing this line re-open #5030

---------------------------------------------------------------------------

by stof at 2012-09-25T13:11:15Z

@lyrixx please add a commit reverting the addition of ``exec`` in the case of sigchild not being used (only this addition, not the full commit you linked) as it should fix your test.

---------------------------------------------------------------------------

by stof at 2012-09-25T13:12:21Z

@fabpot btw, this regression is quite important. As I said in the previous PR, it impacts composer in a bunch of places.

---------------------------------------------------------------------------

by romainneutron at 2012-09-25T13:30:07Z

You reverted too much things, you just had to remove line 233

---------------------------------------------------------------------------

by stof at 2012-09-25T13:42:49Z

@lyrixx I explicitly asked you to revert only the ``exec`` addition for the case without sigchild.

---------------------------------------------------------------------------

by lyrixx at 2012-09-25T13:55:57Z

@stof Sorry, i fixed that.

---------------------------------------------------------------------------

by romainneutron at 2012-09-25T13:56:26Z

@lyrixx just remove the two last commit, edit Process.php and remove line 233

---------------------------------------------------------------------------

by lyrixx at 2012-09-25T13:59:59Z

@romainneutron I think it's ok now.

---------------------------------------------------------------------------

by romainneutron at 2012-09-25T14:11:28Z

yep it's good :)
@fabpot fabpot merged commit 27b2df9 into symfony:2.1 Sep 28, 2012
georgiana-gligor pushed a commit to georgiana-gligor/Sismo that referenced this pull request Nov 3, 2015
georgiana-gligor pushed a commit to georgiana-gligor/Sismo that referenced this pull request Nov 3, 2015
This PR was merged into the master branch.

Commits
-------

64c11a8 Updated symfony/process

Discussion
----------

Updated symfony/process

See symfony/symfony#5592

Tests are ok

Before that, tests failed because of https://github.com/fabpot/Sismo/blob/master/tests/Sismo/Tests/GithubProjectTest.php#L51

Note : I had to change (temporarly) the minimum-stability to dev...
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.

4 participants