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

Use ReflectionNamedType instead of ReflectionType (PHP 7.1+) #5204

Closed
wants to merge 4 commits into from
Closed

Use ReflectionNamedType instead of ReflectionType (PHP 7.1+) #5204

wants to merge 4 commits into from

Conversation

alies-dev
Copy link
Contributor

@alies-dev alies-dev commented Feb 12, 2021

ReflectionNamedType has been introduced in PHP7.1: class ReflectionNamedType extends ReflectionType

Technically speaking, ReflectionFunction::getReturnType(): ReflectionType is a valid construction, but it returns ReflectionNamedFunction in version PHP 7.1+. The same for other reflection API classes that have getType() or getReturnType() methods.

The problem raises when we use a chain ReflectionFunction->getReturnType()->getName(), it's not correct anymore: getName() available in ReflectionNamedType but doesn't in ReflectionType.

see https://3v4l.org/0CMfa (tests ReflectionFunction and child)
see https://3v4l.org/Y2Jqo (tests ReflectionProperty::getType())


Related PR on phpstorm-stubs: JetBrains/phpstorm-stubs#1039

lptn and others added 3 commits February 12, 2021 14:23
`ReflectionNamedType` has been [introduced in PHP7.1](https://www.php.net/manual/en/class.reflectionnamedtype.php): `class ReflectionNamedType extends ReflectionType`

Technically speaking, `ReflectionFunction::getReturnType(): ReflectionType` is a valid construction. 

But when we use a chain `ReflectionFunction->getReturnType()->getName()`, it's not correct anymore: `getName()` available in ReflectionNamedType but doesn't in ReflectionType.

see https://3v4l.org/Y3tvW
@caugner
Copy link
Contributor

caugner commented Jul 12, 2021

@lptn Thank you for your PR. Would you mind rebasing your branch onto the current master?

I noticed one important change in your PR, which is to move PropertyType::getType to dictionaries/CallMap_74_delta.php.

Meanwhile, #5584 added PropertyType::hasType directly to dictionaries/CallMap.php, but your PR makes me think that both getType and hasType belong in CallMap_74_delta.php, because they aren't available in earlier versions.

@alies-dev
Copy link
Contributor Author

alies-dev commented Jul 12, 2021

hey @caugner

thanks for the feedback! Yes, your concern is valid, PropertyType::hasType is available from PHP7.4 only: https://3v4l.org/L6lPY . Unfortunately, I've removed my fork and local branch (it was created on 12 Feb), but can create a new PR with the same changes + move PropertyType::hasType to callmap delta 7.4

@caugner
Copy link
Contributor

caugner commented Jul 12, 2021

@lptn Doesn't GitHub offer you to restore the branch here? I wonder if you also don't have commit 1aff7e2 in your local or remote repository anymore?

edit: Anyways, it would probably help to recreate those changes. If you push them as patch-1, we might even be able to reuse this PR.

@alies-dev
Copy link
Contributor Author

alies-dev commented Jul 12, 2021

@caugner
I don't have both local and remote forks anymore (ironically I've removed them few days ago). I've created a new fork and used the same branch name - no success. So, I've created a new PR: #6077

@caugner
Copy link
Contributor

caugner commented Jul 12, 2021

@lptn Thanks! Let's close this one in favor of that one then. (I can't, but you should be able to.)

@alies-dev alies-dev closed this Jul 12, 2021
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.

3 participants