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

Fix metadata constructor inference by phpstan #8734

Merged
merged 2 commits into from
Jun 5, 2021
Merged

Fix metadata constructor inference by phpstan #8734

merged 2 commits into from
Jun 5, 2021

Conversation

VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet commented May 31, 2021

Close #8709

Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do any errors go away if you regenerate Psalm's baseline? I think PHPStan would complain if it was the case for its baseline.

phpstan.neon Outdated Show resolved Hide resolved
@VincentLanglet
Copy link
Contributor Author

Do any errors go away if you regenerate Psalm's baseline? I think PHPStan would complain if it was the case for its baseline.

PHPStan does complain.
No error will be removed from Psalm baseline since Psalm already correctly inferred the constructor.

@greg0ire
Copy link
Member

greg0ire commented Jun 3, 2021

@orklah please review

@greg0ire greg0ire requested a review from ostrolucky June 3, 2021 18:03
@orklah
Copy link
Contributor

orklah commented Jun 3, 2021

It's a shame to fix that in Doctrine's code but there's not much else to do...

I'm quite puzzled by the tests though. I wonder if it's a good move to add tests to check that Psalm and PHPStan understand an arbitrary piece of code. I'm afraid future errors in those tests could lead to version stagnation for those tools while providing little value for the user (they will still upgrade their versions and their analysis will still be broken).

I'd much rather see PHPStan and Psalm add doctrine to their own CI instead (Psalm already does this for a few libraries that relies on static analysis)

@greg0ire
Copy link
Member

greg0ire commented Jun 3, 2021

About the tests, this approach was suggested by both beberlei and Ocramius, see #8709 (comment) . IMO it's good to test that the annotations we write are correct and don't break downstream projects.

As for the deps stagnation, I'm not too worried about it, I think we will regularly upgrade just like we do now, to get bugfixes.

@greg0ire greg0ire merged commit 3a0f60d into doctrine:2.9.x Jun 5, 2021
@greg0ire
Copy link
Member

greg0ire commented Jun 5, 2021

Thanks @VincentLanglet !

@greg0ire greg0ire added this to the 2.9.3 milestone Jun 5, 2021
@VincentLanglet VincentLanglet deleted the fixMetadata branch June 6, 2021 21:03
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.

The class ClassMetada is not correctly inferring the constructor of ClassMetadaInfo
4 participants