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

Generics for cmf #167

Merged
merged 4 commits into from
Apr 20, 2021
Merged

Generics for cmf #167

merged 4 commits into from
Apr 20, 2021

Conversation

greg0ire
Copy link
Member

This should make a lot of static analysis issues vanish, because tools
will be able to understand that they are guaranteed to receive some
specialization of ClassMetadata, or that they should provide it.

Also upgraded to doctrine/coding-standard 9 so make sure to review commit by commit, it will be easier.

composer.json Outdated
@@ -31,7 +31,7 @@
"require-dev": {
"composer/package-versions-deprecated": "^1.11",
"phpstan/phpstan": "^0.12",
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be bumped to ^0.12.84 as you rely on the fixed behavior of that release ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't follow the latest developments on PHPStan so I don't know that I am, but sure, let's do this 👍

Copy link
Member

Choose a reason for hiding this comment

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

well, look at the diff on the baseline file 😄
The removed entry had a link to a phpstan issue, which got solved in 0.12.84. That's how I found out the necessary version

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, right, already forgot about that 😅

This should make a lot of static analysis issues vanish, because tools
will be able to understand that they are guaranteed to receive some
specialization of ClassMetadata, or that they should provide it.
@greg0ire
Copy link
Member Author

@orklah please review

Copy link

@simPod simPod left a comment

Choose a reason for hiding this comment

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

I did not checkout the code and don't have thorough overview over doctrine/persistence but the changes I see on Github seem 👌🏾

@@ -232,6 +238,7 @@ public function getMetadataFor($className)
if ($this->cache) {
$cached = $this->cache->getItem($this->getCacheKey($realClassName))->get();
if ($cached instanceof ClassMetadata) {
/** @psalm-var CMTemplate $cached */
Copy link

Choose a reason for hiding this comment

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

This feels like cheating, but without trying to wrap the cache interface, I'm not sure we can do better

composer.json Outdated
@@ -30,8 +30,8 @@
},
"require-dev": {
"composer/package-versions-deprecated": "^1.11",
"phpstan/phpstan": "^0.12",
"doctrine/coding-standard": "^6.0 || ^8.0",
"phpstan/phpstan": "^0.12.84",
Copy link

Choose a reason for hiding this comment

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

I'd advise pinning the version of static tools, especially now that we started deploying baseline. It will make sure the version used to generate the baseline is the same for everyone, and make sure not to break builds the day before the release :)

This ensures baseline files are generated with the same version by
everyone, and should stabilize the build.
@greg0ire greg0ire merged commit 6a73390 into doctrine:2.2.x Apr 20, 2021
@greg0ire greg0ire deleted the generics-for-cmf branch April 20, 2021 21:45
@greg0ire greg0ire added this to the 2.2.0 milestone Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants