PHP 8 support #477

Ayesh · 2020-07-04T21:50:27Z

Work in progress to bring PHP 8 support for prophecy.

Majority of the errors were deprecation notices. I fixed the optional-before-required errors and deprecation of ReflectionParameter methods getClass, isCallable, and isArray with branching code that checks getType is available, and then uses it instead of the shorthand now-deprecated functions.

I'm not completely sure about the fixes related to the new mixed type.

After these changes, there are only 4 errors/failures remaining from PHPUnit:

1) Tests\Prophecy\Doubler\ClassPatch\MagicCallPatchTest::it_supports_classes_with_invalid_tags
Method ReflectionParameter::getClass() is deprecated

prophecy/tests/Doubler/ClassPatch/MagicCallPatchTest.php:23

2) Tests\Prophecy\Doubler\Generator\ClassMirrorTest::it_marks_required_args_without_types_as_not_optional
Required parameter $arg_2 follows optional parameter $arg_1

prophecy\fixtures\WithArguments.php:7
prophecy/tests/Doubler/Generator/ClassMirrorTest.php:67

3) Tests\Prophecy\Doubler\Generator\ClassMirrorTest::it_doesnt_use_scalar_typehints
Error: Call to a member function getArguments() on null

prophecy/tests/Doubler/Generator/ClassMirrorTest.php:386

4) Tests\Prophecy\Doubler\Generator\ClassMirrorTest::it_changes_argument_names_if_they_are_varying
Error: Call to undefined method ReflectionUnionType::isBuiltin()

prophecy/tests/Doubler/Generator/ClassMirrorTest.php:480

I cannot get xdebug to work with PHP nightly builds, so these errors do not contain any backtraces. Any insights would be appreciated.
I tried to skip some of the tests (such as #2 above) with @require PHP <= 8 annotations for the test because the deprecated behavior is exactly what's this test is asserting.

ReflectionParameter::export method is deprecated, and removed in PHP 8, which causes #3 above.

Ayesh · 2020-07-04T22:01:28Z

Related: phpspec/phpspec#1314

ciaranmcnulty · 2020-07-05T07:20:57Z

I’d like to get #476 merged which would then simplify this a lot 👍

Ayesh · 2020-07-05T07:23:42Z

That would he lovely! We can remove a lot of void workarounds and always assume getType is available. I'll wait for those PRs, and then rebase this PR after that.

Thank you.

ciaranmcnulty · 2020-07-05T07:43:41Z

Ok I merged it. I actually started a branch like this one but the complexity got too much for me so I did that other change :)

If you can rebase that looks good

For debugging in php8 I had similar issues and resorted to running in 7.4 for debug just to figure out where calls were coming from

ciaranmcnulty · 2020-07-05T07:50:09Z

Maybe split out the changes that are to get the existing suite paying, from changes to support new features (eg mixed hint)

The latter would need additional tests etc

Ayesh · 2020-07-05T07:58:36Z

Thanks a lot @ciaranmcnulty . I sent PR #478 to enable builds. We can either remove workarounds for PHP <7.2 first, and then work on PHP 8 fixes, or the other way around.

stof · 2020-07-06T08:58:50Z

src/Prophecy/Doubler/Generator/ClassMirror.php

@@ -45,7 +45,7 @@ class ClassMirror
     *
     * @throws \Prophecy\Exception\InvalidArgumentException
     */
-    public function reflect(ReflectionClass $class = null, array $interfaces)
+    public function reflect(ReflectionClass $class = null, array $interfaces = array())


I would rather change that to ?ReflectionClass $class, array $interfaces, to avoid changing the signature

stof · 2020-07-06T08:59:25Z

src/Prophecy/Doubler/Generator/ClassMirror.php

@@ -250,7 +250,19 @@ private function isNullable(ReflectionParameter $parameter)
    private function getParameterClassName(ReflectionParameter $parameter)
    {
        try {
-            return $parameter->getClass() ? $parameter->getClass()->getName() : null;
+            if (!method_exists($parameter, 'getType')) {


The min version is now PHP 7.2, so this is never true.

stof · 2020-07-06T09:00:49Z

src/Prophecy/Doubler/Generator/ClassMirror.php

+            if (!$type || $type->isBuiltin()) {
+                return null;
+            }
+            if (!method_exists($type, 'getName')) {


I'd rather add a check for instanceof ReflectionNamedType, which will be needed anyway when adding the compatibility with union types.

stof · 2020-07-06T09:01:50Z

src/Prophecy/Doubler/Generator/ClassMirror.php

@@ -222,11 +222,11 @@ private function getTypeHint(ReflectionParameter $parameter)
            return $className;
        }

-        if (true === $parameter->isArray()) {
+        if (version_compare(PHP_VERSION, '8', '<') && true === $parameter->isArray()) {


I suggest using \PHP_VERSION_ID < 80000 instead, which can be resolved at compile time.

Aha so that's the reason

yeah (not that qualifying the call is important for that. Relying on constant fallback will delay it until runtime)

stof · 2020-07-06T09:02:08Z

src/Prophecy/Doubler/Generator/ClassMirror.php

            return 'array';
        }

-        if (version_compare(PHP_VERSION, '5.4', '>=') && true === $parameter->isCallable()) {
+        if (true === $parameter->isCallable()) {


shouldn't we have a version check here too according to the commit message ?

stof · 2020-07-06T09:03:18Z

please also rebase this PR now that code for PHP < 7.2 has been removed.

Ayesh · 2020-07-06T09:15:57Z

Thanks a lot for the great feedback. I will rebase/update code with new minimum PHP version in mind and other changes.

I used version_compare() because it was already being used in other areas, but I think breaking that convention is worth the dead code elimination improvements.

This pattern [emits a deprecation notice in PHP 8](https://php.watch/versions/8.0/deprecate-required-param-after-optional). First parameter to `\Prophecy\Doubler\Generator\ClassMirror::reflect()` is now marked nullable because PHP 7.2 is the new minimum PHP version.

…ay` deprecations `ReflectionParameter::isCallable()` and `ReflectionParameter::isArray()` are [deprecated in PHP 8](https://php.watch/versions/8.0/deprecated-reflectionparameter-methods). They are now replaced with `ReflectionParameter::getType()` calls.

[`ReflectionParameter::getClass()` is deprecated in PHP 8](https://php.watch/versions/8.0/deprecated-reflectionparameter-methods) It is now replaced with a combination of `ReflectionParameter::getType()` and `ReflectionParameter::getClass`.

…w nullability on it PHP 8's new [`mixed` type](https://php.watch/versions/8.0/mixed-type#nullable) cannot be used as a nullable type, and needs special handling.

Ayesh · 2020-07-06T11:46:58Z

@stof I rebased and made some changes in the code, and with PHP 7.2 as the minimum, I could remove a lot of code. It would be great if you could take a look.

GrahamCampbell · 2020-07-07T12:33:06Z

src/Prophecy/Doubler/Generator/ClassMirror.php

     */
-    public function reflect(ReflectionClass $class = null, array $interfaces)
+    public function reflect(?ReflectionClass $class, array $interfaces)


This is a major breaking change because the class is non-final. Please revert.

This weirdly does not seem to cause any issue https://3v4l.org/roQB8 #lolphp

Oh, it's because the 2nd param is required, so PHP treats them all as required, and null is specially implemented.

@GrahamCampbell did you delete a comment? It's not an issue for callers because they have no way of taking advantage of the optionality

Yeh, I replaced with with the comment that the 1st param was not really optional anyway

GrahamCampbell · 2020-07-07T12:33:39Z

src/Prophecy/Doubler/Generator/ClassMirror.php

+                return $type->getName();
+            }
+
+            return null;
        } catch (\ReflectionException $e) {


Don't need this.

That's true @Ayesh the try/catch is because getClass can throw, getName does not

GrahamCampbell · 2020-07-07T13:47:28Z

I this this PR is ready for merge now. It gets the ball rolling at least, even if it is not complete (just like the phpspec PR).

ciaranmcnulty · 2020-07-07T13:48:52Z

Thanks, @Ayesh!

Ayesh mentioned this pull request Jul 4, 2020

Support PHP 8 sebastianbergmann/phpunit#4325

Closed

31 tasks

Ayesh marked this pull request as draft July 4, 2020 22:01

Ayesh mentioned this pull request Jul 5, 2020

Travis CI: Enable builds on PHP nightly #478

Merged

stof reviewed Jul 6, 2020

View reviewed changes

Ayesh added 5 commits July 6, 2020 18:45

Fixes in ClassMirrorTest parameter mocks

1176cc6

Fix Class generator to mark mixed as an internal type, and not allo…

96054a0

…w nullability on it PHP 8's new [`mixed` type](https://php.watch/versions/8.0/mixed-type#nullable) cannot be used as a nullable type, and needs special handling.

GrahamCampbell suggested changes Jul 7, 2020

View reviewed changes

Remove unnecessary try/catch

5cb1c60

ciaranmcnulty marked this pull request as ready for review July 7, 2020 13:48

ciaranmcnulty merged commit ad49d54 into phpspec:master Jul 7, 2020

greg0ire mentioned this pull request Jul 7, 2020

Double has signatures that are incompatible with prophesized interface when using self #484

Closed

5 tasks

stof mentioned this pull request Jul 17, 2020

Add php8 support #492

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHP 8 support #477

PHP 8 support #477

Ayesh commented Jul 4, 2020

Ayesh commented Jul 4, 2020

ciaranmcnulty commented Jul 5, 2020

Ayesh commented Jul 5, 2020

ciaranmcnulty commented Jul 5, 2020

ciaranmcnulty commented Jul 5, 2020

Ayesh commented Jul 5, 2020

stof Jul 6, 2020

stof Jul 6, 2020

stof Jul 6, 2020

stof Jul 6, 2020

ciaranmcnulty Jul 7, 2020

stof Jul 7, 2020

stof Jul 6, 2020

stof commented Jul 6, 2020

Ayesh commented Jul 6, 2020

Ayesh commented Jul 6, 2020

GrahamCampbell Jul 7, 2020

ciaranmcnulty Jul 7, 2020

GrahamCampbell Jul 7, 2020

ciaranmcnulty Jul 7, 2020

GrahamCampbell Jul 7, 2020

GrahamCampbell Jul 7, 2020

ciaranmcnulty Jul 7, 2020

GrahamCampbell commented Jul 7, 2020

ciaranmcnulty commented Jul 7, 2020

PHP 8 support #477

PHP 8 support #477

Conversation

Ayesh commented Jul 4, 2020

Ayesh commented Jul 4, 2020

ciaranmcnulty commented Jul 5, 2020

Ayesh commented Jul 5, 2020

ciaranmcnulty commented Jul 5, 2020

ciaranmcnulty commented Jul 5, 2020

Ayesh commented Jul 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Jul 6, 2020

Ayesh commented Jul 6, 2020

Ayesh commented Jul 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GrahamCampbell commented Jul 7, 2020

ciaranmcnulty commented Jul 7, 2020