Use ReflectionParameter::getType() instead of getClass() #240
Conversation
|
Note that this is not the same as before. You also need to resolve |
|
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. |
|
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. |
|
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... |
|
Please rebase this pr. Than ci will be fixed. I did that last week when I saw this pr failing. |
`iReflectionParameter::getClass()` is deprecated as of PHP 8 and will trigger a warning.
DerManoMann
force-pushed
the
reflectionparameter-getclass-deprecated-php8
branch
from
836cd07
to
cd8b5f4
Compare
|
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. |
|
sure, will try today. is there documents on how to run those tests locally, perhaps? |
|
If you have docker on your local machine you can use the make file. |
DerManoMann
force-pushed
the
reflectionparameter-getclass-deprecated-php8
branch
3 times, most recently
from
e00d5b0
to
0690553
Compare
DerManoMann
force-pushed
the
reflectionparameter-getclass-deprecated-php8
branch
from
0690553
to
321abc5
Compare
|
These changes are actually not correct. They don't deal with union types properly. I'll send a replacement PR. |
|
@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. |
|
@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? |
|
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:
Even 1) as a separate release would be useful. |
|
Line 263 of StandardTagFactory of the code on master. |
|
Oh, I think I see what's going on... |
|
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. |
|
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. |
|
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 :-) |
|
All green, Think this is good to go. Thanks @DerManoMann! |
|
Thanks @jaapio! |
|
And most of all, thank you @DerManoMann to take the time to help us out here |
I was referring to the replacement PR I was preparing: #242. |
ReflectionParameter::getClass()is deprecated as of PHP 8 and will trigger a warning.