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

[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 :smile:

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:

(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 :+1: 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).

:+1:

[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.

[JaxkDev]

Is there any update on this ?