Address deprecation of ReflectionType::getClass()

[cdosoftei]

Calling ReflectionType::getClass() will trigger a deprecation warning on PHP8. This PR switches to getType() if available.

Deprecated: Method ReflectionParameter::getClass() is deprecated in /app/vendor/react/promise/src/functions.php on line 345

[clue]

@cdosoftei This is a interesting one, thanks for looking into this!

This PR targets the master branch which currently requires PHP 7.1+, so the ReflectionParameter::getType() should always be available.

Have you tried this piece of logic with a union type in PHP 8? Perhaps we should add some additional tests (which should be skipped on PHP 7).

Once this PR is in, we'll have to backport a similar change to the v2.x release branch. In this case, we'll have to fall back to the deprecated getType() method on legacy PHP versions (php/php-src#5209).

[cdosoftei]

Thanks for the feedback @clue!

I had not originally paid attention to the project's branching strategy but this makes perfect sense now and I see how the PRs would look different between master vs 2.x (and I love it, no need to clutter releases for current PHP versions with a lot of legacy tests).

I had not tested the behaviour against unions and will do so shortly, I'll see if I can do so via a (7.1+ compatible) unit test.

[cdosoftei]

Thanks again for the feedback, some more will be in order as outlined below 😄

The second commit to this PR introduces a 7.1+ compatible solution that addresses the PHP 8 deprecation as well as union types. However, certain aspects can be implemented in different fashions so I'd like some input on the following:

(1) errcontext is deprecated for set_error_handler as of 7.2, however, as of version 8, the runtime will refuse to set the error handler if the parameter is configured, rendering the test suite's ErrorCollector unusable. Since the context itself is not used throughout the suite, I went ahead and removed it to facilitate testing with PHP 8.

(2) PHPUnit 9.3.7 complained about the XML configuration so I migrated per the suggestion, which renders a file compatible with version 7.5.20 used for the PHP 7.1 test suite.

(3) The union type tests would break any PHP <8 execution of the suite, so I had them commented for now. There are several ways to address this and I'd like to defer the solution to a maintainer.

(4) I've implemented the argument handler as a loop, in anticipation of union types. One could also externalize the argument's analysis to different function which could be invoked once if the reflection type does not support unions (i.e. getTypes method is not available) and then invoke the said function in a loop if the type is adequately identified as an union (or even perform a recursion). Best practice advice is appreciated!

(5) I've placed the ReflectionClass instantiation in a try-catch statement to mimic the behaviour of the deprecated getClass which would return null if it's unable to instantiate the respective reflection class.

(6) Lastly, I did not implement a test for pure function callbacks as the current suite does not use them (the current test cases are a duplication of the invokable object callbacks).

[clue]

Thanks for the update, love the direction this is taking! 👍

(1) errcontext is deprecated for set_error_handler as of 7.2, however, as of version 8, the runtime will refuse to set the error handler if the parameter is configured, rendering the test suite's ErrorCollector unusable. Since the context itself is not used throughout the suite, I went ahead and removed it to facilitate testing with PHP 8.

LGTM 👍 It looks like this only affects the master branch.

(2) PHPUnit 9.3.7 complained about the XML configuration so I migrated per the suggestion, which renders a file compatible with version 7.5.20 used for the PHP 7.1 test suite.

Thanks for looking into this! I've just discussed this with @SimonFrings, he'll look into filing a dedicated PR to address this issue (refs #173). This means this should probably be removed from this PR and then later be rebased.

(3) The union type tests would break any PHP <8 execution of the suite, so I had them commented for now. There are several ways to address this and I'd like to defer the solution to a maintainer.

The commented out code LGTM. This only affects the test suite, so I would suggest defining these dummy classes in an eval statement and skip the tests on older PHP versions. This way, the PHP compiler doesn't complain about syntax that is incompatible with older PHP versions.

(4) I've implemented the argument handler as a loop, in anticipation of union types. One could also externalize the argument's analysis to different function which could be invoked once if the reflection type does not support unions (i.e. getTypes method is not available) and then invoke the said function in a loop if the type is adequately identified as an union (or even perform a recursion). Best practice advice is appreciated!

I'm not sure I follow what problem you're seeing after eyeballing your implementation. It's looks like the check can be replaced with $type instanceof ReflectionUnionType, otherwise LGTM afaict.

(5) I've placed the ReflectionClass instantiation in a try-catch statement to mimic the behaviour of the deprecated getClass which would return null if it's unable to instantiate the respective reflection class.

The code looks okay to me given the previous structure. Have you tried replacing the ReflectionClass instantiation with an instanceof check instead?

(6) Lastly, I did not implement a test for pure function callbacks as the current suite does not use them (the current test cases are a duplication of the invokable object callbacks).

👍

[cdosoftei]

Thanks a bunch for the feedback -- I've incorporated the last round of tweaks and feeling pretty good about it; I was thinking the eval'd test code could one day be superseded by phpVersion tagging, thought that might require some test suite restructuring.

[clue]

@cdosoftei Thanks for the update!

Can you squash your changes and rebase now that #177 is in?

I've successfully confirmed this to work just fine with PHP 8 locally 🎉

$ docker run -it --rm -v `pwd`:/data --workdir /data php:8.0.0beta1-cli-alpine php -d memory_limit=-1 vendor/bin/phpunit
PHPUnit 9.3.7 by Sebastian Bergmann and contributors.

...............................................................  63 / 371 ( 16%)
............................................................... 126 / 371 ( 33%)
............................................................... 189 / 371 ( 50%)
............................................................... 252 / 371 ( 67%)
............................................................... 315 / 371 ( 84%)
........................................................        371 / 371 (100%)

Time: 00:00.421, Memory: 188.00 MB

OK (371 tests, 602 assertions)

[clue]

Can be replaced with @requires PHP 8 annotations for each test à la

promise/tests/PromiseTest.php

Line 135 in 12dcf28

* @requires PHP 7

.

[cdosoftei]

Thanks for the feedback, this is incredibly helpful for writing cleaner tests and better contributions in general!

I do have one more, likely final, follow up:

(1) While the annotation does the job nicely for the test methods, I am not sure of a similar/cleaner approach for the eval piece that defines the class with union type argument methods:

https://github.com/singlecomm/promise/blob/d9bd281fffeb7c1dcc997f177e97dd28bb1b7ca9/tests/FunctionCheckTypehintTest.php#L143-L162

(2) When running the test suite against PHP 7.x, the PHPUnit summary accounts the PHP 8 specific tests as being skipped (albeit it does allow the CI to exit successfully). I believe a @doesNotPerformAssertions annotation (or the prior self::expectNotToPerformAssertions() approach) would be conflicting but I am wondering if there are other options.

https://travis-ci.org/github/reactphp/promise/jobs/719402155

Thank you!

[clue]

Thanks for keeping up with this and the excellent communication!

(1) While the annotation does the job nicely for the test methods, I am not sure of a similar/cleaner approach for the eval piece that defines the class with union type argument methods:

https://github.com/singlecomm/promise/blob/d9bd281fffeb7c1dcc997f177e97dd28bb1b7ca9/tests/FunctionCheckTypehintTest.php#L143-L162

The suggested changes LGTM as-is, but I understand where you're coming from 👍 As an alternative, we should also be able to move this class to tests/fixtures and rely on autoloading to only parse this file for PHP 8 from your specific tests. In this case, I would argue this whole file could use a cleanup and all its fixtures should be moved accordingly. Is this something you can look into?

(2) When running the test suite against PHP 7.x, the PHPUnit summary accounts the PHP 8 specific tests as being skipped (albeit it does allow the CI to exit successfully). I believe a @doesNotPerformAssertions annotation (or the prior self::expectNotToPerformAssertions() approach) would be conflicting but I am wondering if there are other options.

https://travis-ci.org/github/reactphp/promise/jobs/719402155

IMHO skipping tests that are unsupported on certain platforms is the best solution for this situation (clear communication, matches semantically and allows showing more details with --verbose). I agree that the PHPUnit summary (yellow) looks perhaps a bit "unhelpful". In the past we've tried this circumvent this to show a "green" output on your "average PHP installation", but this turned out to be hard to sustain/maintain with multiple PHP environments. Accordingly, we're also having to skip some tests for our other components, e.g. https://travis-ci.org/github/reactphp/event-loop/jobs/709667085, https://travis-ci.org/github/reactphp/socket/jobs/713968611 and others.

[cdosoftei]

Implementing the fixture classes appropriately renders a much nicer test suite structure indeed!

I believe all validations are satisfied at this point; I've messed up the git/branching strategy on my end -- do you mind squashing the commits at merging time? Thanks!

[JaxkDev]

Is there any update on this ?