Use ReflectionParameter::getType() instead of getClass()

[DerManoMann]

ReflectionParameter::getClass() is deprecated as of PHP 8 and will trigger a warning.

[GrahamCampbell]

Note that this is not the same as before. You also need to resolve self to the actual class name.

[GrahamCampbell]

That can be done like this: https://github.com/laravel/framework/blob/v7.18.0/src/Illuminate/Support/Reflector.php#L24-L30.

[DerManoMann]

That can be done like this: https://github.com/laravel/framework/blob/v7.18.0/src/Illuminate/Support/Reflector.php#L24-L30.

Updated - Thanks. I had a quick look at the tests but couldn't find an obvious place to add a test. If it is preferred to add one I would appreciate a nudge into the right direction...

[jaapio]

Hi @DerManoMann,

I think his is covered by the 3 cases below this line: https://github.com/phpDocumentor/ReflectionDocBlock/blob/master/tests/unit/DocBlock/StandardTagFactoryTest.php#L171

This part of the code is used to inject dependencies into the factory method of the tags. Please let me know if you need any help to get this tested.

[DerManoMann]

Happy to believe you @jaapio :)

There are certainly tests failing without the typehint working, so regessions are covered IMO.

Just not sure what to do about the QA workflow apparently being broken. AFAICS the break is unrelated to my change...

[jaapio]

Please rebase this pr. Than ci will be fixed. I did that last week when I saw this pr failing.

[jaapio]

Thanks for rebasing this... If you have time it would be very nice if you could fix the failing steps. php 8 can be ignored for now since we are waiting for some external dependencies to be fixed.

[DerManoMann]

sure, will try today. is there documents on how to run those tests locally, perhaps?

[jaapio]

If you have docker on your local machine you can use the make file.

[GrahamCampbell]

These changes are actually not correct. They don't deal with union types properly. I'll send a replacement PR.

[jaapio]

@GrahamCampbell I think you could have said this in a different way. Your help is very much appreciated, like the help of everyone else. @DerManoMann is doing a very good job here to help us move forward and I don't think we should just close this pr or superseded his contributions by creating a separate pr. I think it would be more helpful for the project to help him improve this PR.

[GrahamCampbell]

@jaapio Is there a bug in the original code? Could you help me understand what's going on. Why do we resolve the type using reflection and then look it up in the locator array. I think the locator is only meant to be used to look up parameter names, and not types?

[ondrejmirtes]

I don't even think that support for unions has to be added as part of this PR. A 2-step approach can be taken:

1) Make sure we don't use any deprecated functionality when running on PHP 8. 2) Support all the new stuff from PHP 8 like union types, static, mixed...

Even 1) as a separate release would be useful.

[GrahamCampbell]

Line 263 of StandardTagFactory of the code on master.

[GrahamCampbell]

Oh, I think I see what's going on...

[jaapio]

This method is an internal method... it might support UnionTypes at some point. But this is NOT about reflecting types on docblock content it is just meant to be some dependency injection mechanism for tags. It has nothing to do with docblock or type resolving.

[GrahamCampbell]

Yeh. I don't think we want to support union types in that method. Doesn't make sense for a service locator to inspect union types, other than a union with null. Tests pass on my machine, but your makefile doesn't run on PHP 8 (because phpunit isn't managed by composer). Will have to wait to see what GitHub actions says.

[jaapio]

Yes you are right we don't need Uniontypes here. Since this library is only reflecting docblock which do support uniontypes since ..... I don't expect much changes to be needed to support php8 :-)

As I explained earlier to you we cannot manage PHPUnit via composer. since this library is a dependency of phpunit. PR's simply won't build with composer :-) phpunit is installed with phive, as a phar with scoped namespaces so we are sure that phpunit is using the required version of this lib. And we are testing the version that needs to be tested. Instead of letting php mess with the autoloading, testing the wrong version.

[jaapio]

All green, Think this is good to go. Thanks @DerManoMann!

[mvriel]

Thanks @jaapio!

[mvriel]

And most of all, thank you @DerManoMann to take the time to help us out here

[GrahamCampbell]

https://github.com/phpDocumentor/ReflectionDocBlock/pull/242

[GrahamCampbell]

Tests pass on my machine, but your makefile doesn't run on PHP 8 (because phpunit isn't managed by composer).

I was referring to the replacement PR I was preparing: #242.