[Process] fix for Process:isSuccessful()

[tgabi333]

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

If you call isSuccessful() on a running process you would get true because of 0 == null. I think that is not the expected behavior and that is not what phpdoc @return block said so.

This PR fixes another phpunit failure on testCheckTimeoutOnStartedProcess

[jakzal]

In this case I'd rather add a new test case, revealing the bug, instead of modifying an existing one.

[romainneutron]

👍 on adding a new test case. We also must ensure that the exitcode we catch is an int. I suspect that if the comparison was with a simple ==, it's because it may be an int or a string (I really dont know)

[romainneutron]

In case of $this->exitcode === null, I think we should throw a LogicException as a succesful state is neither true nor false before the process finished.

[romainneutron]

last but not least, I think this PR should be open against 2.2

[tgabi333]

Exit code is an int https://github.com/php/php-src/blob/master/ext/standard/proc_open.c#L384

[tgabi333]

New unit test case added. I'm afraid if we throw an exception in isSuccessful() it would be a BC, i would rebase this PR to 2.2 and if you like i would create another PR and put a throw in this method.

[romainneutron]

Be careful, the exitcode might be caught by proc_close or with a hack in case of sigchild enabled environment.

@fabpot, @stof, do you consider throwing a LogicException when calling this method on a running / non-run process a BC break ? Should it be added in 2.2 ? I'd rather consider not, what is your opinion ?

[fabpot]

Throwing an exception is a BC break. I think it makes sense to throw an exception on a running / non-run process , but this needs to be done on master instead.

[tgabi333]

Replaced with #8815