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 ClassMetadata[Info] class in type hints instead of the ClassMetadata interface #8746

Closed
mpdude opened this issue Jun 7, 2021 · 3 comments

Comments

@mpdude
Copy link
Contributor

mpdude commented Jun 7, 2021

This was sparked by #8402:

At least \Doctrine\ORM\Mapping\ClassMetadataFactory::doLoadMetadata and \Doctrine\ORM\Mapping\ClassMetadataFactory::validateRuntimeMetadata (with the changes of #8402) use \Doctrine\Persistence\Mapping\ClassMetadata (an interface) as DocBlock type-hints for parameters, but access \Doctrine\ORM\Mapping\ClassMetadataInfo::$discriminatorMap through it.

This causes PHPStan and Psalm to complain, since Interfaces cannot have properties.

When code like $parent->discriminatorMap is supposed to be kept as-is (that is, no corresponding method added in the ClassMetadata interface), the type hints need to be changed. This might cascade through a chain of called methods, though.

@greg0ire
Copy link
Member

greg0ire commented Jun 14, 2021

I don't think changing the type declarations is an option for doLoadMetadata: it's forbidden to narrow them down since that would break the LSP.
Instead we could throw if anything else than an ORM ClassMetadata is passed.

For validateRuntimeMetadata() it's another story since there is no method override. We can change the type declaration 👍
After doing that, we should regenerate PHPStan and Psalm baselines in case they contain something about this.

@mpdude
Copy link
Contributor Author

mpdude commented Jun 8, 2023

Found this old issue. Don't really recall what I was talking about, might have been resolved since then.

@greg0ire
Copy link
Member

greg0ire commented Jun 8, 2023

I think the @extends annotation introduced in 98d7704#diff-3e426fe47b58d2d4b970b1b7a0cbf5999af950e4f84e48dce4bee5061b57ed31 does fix it.

@greg0ire greg0ire closed this as completed Jun 8, 2023
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

No branches or pull requests

2 participants