PHP 8 support - Githubissues

Ayesh commented 4 years ago

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 commented 4 years ago

Related: phpspec/phpspec#1314

ciaranmcnulty commented 4 years ago

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

Ayesh commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

Ayesh commented 4 years ago

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.

Ayesh commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

Thanks, @Ayesh!

phpspec / prophecy

PHP 8 support #477