Refactor proc_open() implementation

[Girgias]

Not sure all of the commits are sensible, and I still don't quite grasp why I segfault on an unknown address if I convert the command struct member from char* to zend_string*

[twose]

I think you should use add_assoc_str(return_value, "command", zend_string_copy(proc->command)); instead, this may be the cause of the memory error :)

[Girgias]

I think you should use add_assoc_str(return_value, "command", zend_string_copy(proc->command)); instead, this may be the cause of the memory error :)

That seems to have been indeed the issue, thanks for spotting this. :-)

[nikic]

Looks mostly okay. I'm uncertain about:

• a112c2d The types used here look rather arbitrary to me, why is this an improvement?
• 1d05bba IMHO this should've just changed the return types to zend_result. Or do we have some general policy that bool is preferred over zend_result?

[Girgias]

Looks mostly okay. I'm uncertain about:

* [a112c2d](https://github.com/php/php-src/commit/a112c2d35624f414558f6a1b01b24e522d34d417) The types used here look rather arbitrary to me, why is this an improvement?

That's the one I really wasn't sure about, just tried to align some types to prevent the int cast, and thought well the array offsets used here should only be unsigned so let's go with size_t, but it doesn't really do anything.

* [1d05bba](https://github.com/php/php-src/commit/1d05bba8e88b24bb70c069679f48e55bc889a7b4) IMHO this should've just changed the return types to zend_result. Or do we have some general policy that bool is preferred over zend_result?

We don't have a general policy but I do think bool is usually better than zend_result because you cannot do if (!func_return_zend_result()) {} to check for failure, one option would be to change zend_result to match bool, but the -1 for FAILURE is probably used (if not within php-src, in external extensions) to return -1 for failure as is typical for C APIs.
And because those are static functions restrained to this file, I prefer using bool to make a future audit of FAILURE/SUCCESS easier.

But I could change it to zend_result.

[nikic]

We don't have a general policy but I do think bool is usually better than zend_result because you cannot do if (!func_return_zend_result()) {} to check for failure, one option would be to change zend_result to match bool, but the -1 for FAILURE is probably used (if not within php-src, in external extensions) to return -1 for failure as is typical for C APIs.
And because those are static functions restrained to this file, I prefer using bool to make a future audit of FAILURE/SUCCESS easier.

Why is being able to write if (!func()) a goal though? The == SUCCESS / == FAILURE check seems pretty clear for a function where the return value indicates success/failure. bool is good for predicate functions (is_* and friends), but it doesn't seems like the natural choice for a success indicator, if we already have zend_result that is explicitly intended for that purpose?

[Girgias]

It is not a goal, maybe it's just because zend_result is in my mind also a "boolean"-like enum that I think it's better to just use the "native" type directly, especially as one needs to "audit" the functions anyway to check the int return value is legitimate or one can change it to zend_result. Also it can be a bit confusing at times that some return zend_result and other bool.

But, frankly either zend_result and bool are fine with me.

[Girgias]

I dropped the int to size_t type changes and changed the bool returns to zend_result