Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test failing on array types on PHP 8+ #191

Merged
merged 1 commit into from
Mar 4, 2021

Conversation

Zvax
Copy link
Contributor

@Zvax Zvax commented Mar 4, 2021

After a discussion in r11 it appears that from this PR the behaviour changed so that in PHP80+ typehinting an array fails because getType returns the actual string "array" whereas pre-PHP80 the value was simply null.

Returning "array" causes auryn to try and instantiate an array, which is not a class, and triggers error 8: Could not make array: Class "array" does not exist.

Orthogonally, phpunit being at version 4 makes it incompatible with PHP80+, which is necessary to make the test fail. Allegedly this would be fixed by #188.

cmb in chat proposed a quick stub that could solve the issue. The changes do not seem to impact in any way the code from the phpunit upgrade PR so there's no merging order requirement.

@kelunik
Copy link
Collaborator

kelunik commented Mar 4, 2021

$type = $param->getType();
if ($type instanceof \ReflectionNamedType && $type->isBuiltin()) {
    return null;
}

return $type ? (string) $type : null;

I have that locally now in getParamTypeHint.

@Zvax
Copy link
Contributor Author

Zvax commented Mar 4, 2021

Yes, this fixes the error here, I have put this in StandardReflector. I don't know how to test it with the caching reflector to make sure it works with it as well.

@kelunik kelunik changed the title Adds test failling on array typehints in PHP80+ Add test failing on array types on PHP 8+ Mar 4, 2021
@kelunik kelunik merged commit 306e7a8 into rdlowrey:master Mar 4, 2021
@Zvax Zvax deleted the dev-array-typehint branch March 4, 2021 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants