Migrate resource returned by proc_open() to opaque object

[kocsismate]

As part of php/php-tasks#6

[Girgias]

Couple of questions and issue I can spot in the existing implementation.

Also for when, if ever, we do the stream resource to opaque object conversion is it noted that proc_open() stores an array of streams?

[Girgias]

Also seems there is some issue on Windows.

[dstogov]

It looks like, this is not a simple change that introduces compatibility breaks.
I think this should be better designed first. We also need to know all the introduced breaks.
Probably this needs an RFC.

[Girgias]

It looks like, this is not a simple change that introduces compatibility breaks. I think this should be better designed first. We also need to know all the introduced breaks. Probably this needs an RFC.

As far as I can see the only BC break is using is_resource() and the Process name becoming reserved, considering the global namespace, as far as I know, is "reserved" by PHP this shouldn't be an issue. Also we have performed other resource to opaque object conversions in 8.1.

[dstogov]

As far as I can see the only BC break is using is_resource() and the Process name becoming reserved, considering the global namespace, as far as I know, is "reserved" by PHP this shouldn't be an issue.

I was a bit confused by changes in PHPT tests. But now I see, that the usage of resource returned from proc_open() in different internal functions was just to demonstrate the errors.

I think reserving Process name is a problem, as this may break user code. It's better to put it into some internal namespace.

I still ask to minimize the patch.

[kocsismate]

@dstogov @Girgias I appreciate your reviews, thank you! I'll get back to address your comments in the following days/week.

Regarding the Process class name, I agree with the BC concern, and the class name is up for debate :) Regarding minimizing the diff, I'm not sure why GitHub shows the diff for _php_array_to_envp()... My guess would be that I put the new code before it... I'll try to get rid of this apparent change though, as it's annoying indeed.

[bukka]

I think this should get some discussion on internals - specifically the Process class name... Please also reduce the diff - it makes review much harder.

[bukka]

Also Windows failure seems related.

[bukka]

@kocsismate btw I didn't see your comment as it happened during my review :)

[dstogov]

There is a related test failure.

========DIFF========
--
       [1]=>
       string(14) "AssertionError"
       [2]=>
007-   string(7) "Process"
007+   string(16) "Standard\Process"
       [3]=>
       string(15) "php_user_filter"
       [4]=>
--
========DONE========
FAIL ReflectionExtension::getClassNames() method on an extension which actually returns some information [ext/reflection/tests/ReflectionExtension_getClassNames_basic.phpt] 

[kocsismate]

I'm not sure exactly why, but now Windows tests stopped hanging.

[kocsismate]

Can I have a review round again please?

[dstogov]

I don't see problems.
I'm not complteley sure about name "Standard\Process" but anyway it's better than "Process".

[bukka]

As I said this should be discussed on internals on internals. Especailly the naming and introducing Standard namespace is definitely for dicsussion and maybe RFC if there is no agreement. I would personally prefer to delay it to 9.0 as that is_resource on proc might be used quite often. I think it would be good to reconsider the whole approach and maybe repurpose is_resource to also return true for some special objects. That would be useful for possible stream migration in the future.

Btw. the fact that something has been done without discussion / RFC before is not a valid argument. As soon as there are objections, RFC or at least proper discussion is necessary whatever the previous changes are.

[bukka]

And I know that repurposing is_resource still leaves gettype but that should be fine IMHO.

[Girgias]

And I know that repurposing is_resource still leaves gettype but that should be fine IMHO.

I don't think repurposing is_resource() is a good idea. There is a backwards compatible way of checking if the function returns a valid handler or has failed, by checking $v === false. Especially as a lot of code that uses is_resource() will also use gettype() to check if the handler is closed or not.

[bukka]

I know that you can change it and compare it with false but you need to change code for that which is the main issue here. It require potentially lots of code changes which is especially problem in legacy code bases and less maintained code. We should not be doing this in minor versions as this is certainly a significant BC break for some users. The fact that we did that in past was a mistake IMHO and IIRC people were complaining about that.

I have seen lots of code using the is_resource pattern for checking valid output and I don't see reason why it should be changed when we can easily change semantic of is_resource. Remember that is_... functions are not all for single type checks already as we have got is_numeric so I don't see a problem with that. I think breaking gettype checks is not really such a big issue because it's like much less used - at least I have not really seen it used that much.

[arokettu]

@Girgias That check won't be enough if you accept resources in a public api in a lib

@bukka What's wrong with userland solutions like mine? https://github.com/arokettu/is-resource

[Girgias]

We should not be doing this in minor versions as this is certainly a significant BC break for some users. The fact that we did that in past was a mistake IMHO and IIRC people were complaining about that.

PHP does not follow semver and has always made some BC breaks in minor versions.

Moreover, the consensus around the release of PHP 8.0, was clearly to focus on converting E_WARNINGs to TypeErrors/ValueErrors as converting warnings in a minor release was not something that had ever been done; whereas converting resources to object had been done in minor versions (e.g. GMP in 5.6, HashContext in 7.2).

Thus, we deprioritized converting resources to opaque objects in 8.0 as it was expected we could continue this work in minor releases (considering the quantity of resource there were to convert). Now deciding that those changes are not doable and that one should have committed time to it at an arbitrary point in the past, or at an arbitrary point in the future does not lead to a productive way for contributing to this project.

This is not to say that changing proc_open() to return opaque objects cannot be delayed to PHP 9.0, because it may have a larger impact. But this should not prevent other resources to be converted in minor versions.

Especially as the writing has been on the wall for resources, especially since 8.0.0, and it should be clear how to deal with those conversions nowadays.

Remember that is_... functions are not all for single type checks already as we have got is_numeric so I don't see a problem with that.

But that's, IMHO, incorrect, is_numeric() does a type check, the type being checked is not a pure atomic type check but rather if it is possible to use arithmetic operations on the value. Same with is_callable() which checks if you can call the value as a function, or is_iterable() if you can sensibly use foreach on the value.

@Girgias That check won't be enough if you accept resources in a public api in a lib

Public APIs can guard the call to is_resource() with if ($v instanceof OpaqueObject) and still be compatible across versions.

[Girgias]

I will also say, if we want PHP to follow semver, then all the extensions should be unbundled and moved to PECL, so they can evolve independently of the runtime. And people could decide to stick with previous versions of the extension rather than be forced to upgrade as it is currently the case.

[bukka]

I have never said we do semver. It's just that we always tried to leave bigger BC breaks for major versions. I think converting object to resources is a bigger BC break and thus should be left for major version especially for more used resources like the ones in standard ext. In case of gmp it was ok because it was part of overloading RFC so it made in some way a sense and followed a proper process. The other changes in minor versions were more questionable (especially pgsql in 8.1).

I don't think we will probably agree on this which is fine and that's why this should go through RFC where it can be decided. This is not really right place to discuss it anyway. In terms of this change it would be good to have RFC to also confirm if everyone is happy with introduction of Standard namespace which I think should be confirmed as well.

[cmb69]

The mentioned RFC has been proposed and it has been accepted that the process ressource should be converted for PHP 9.0, so I'm marking this PR with the respective milestone.