-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Make phpdoc more precise for AbstractQuery #9777
Make phpdoc more precise for AbstractQuery #9777
Conversation
15c0a1f
to
52eb73a
Compare
lib/Doctrine/ORM/AbstractQuery.php
Outdated
* @param int $fetchMode | ||
* @param class-string $class | ||
* @param string $assocName | ||
* @param ClassMetadata::FETCH_EAGER|ClassMetadata::FETCH_LAZY $fetchMode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @param ClassMetadata::FETCH_EAGER|ClassMetadata::FETCH_LAZY $fetchMode | |
* @param int $fetchMode | |
* @psalm-param ClassMetadata::FETCH_EAGER|ClassMetadata::FETCH_LAZY $fetchMode |
I don't know how well literal types are supported outside of PHPStan and Psalm. So far, we have vendor-prefixed annotations that use them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the implementation, it seems to hide potential errors. Invalid fetch modes are healed right now, but I think, we should deprecate them, so we can throw in 3.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so let's keep int
for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documenting literal types is a good idea, I think. It's just that I would vendor-prefix the @param
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 sounds like you would with with @VincentLanglet on #9778 then
In this particular instance, I think it can't hurt, and I would be eager to know what your stance is on that PR. I was wondering if we should:
- only ever document the non-deprecated path
- always document both
- decide of that on a case-by-case basis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it can't hurt
It already makes the build fail though, not sure that it's a great idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@param
annotations should document the public API, the intended usage of that method. In this case: It is not intended that anyone would pass 42
, 'hello world'
or even an array or a cURL resource to this method although it has always been possible.
It already makes the build fail though
That's Psalm being Psalm. On 3.0, we would throw if the caller passed anything but the two allowed values, right? You would face the very same problem then. I'd argue that you cannot write that kind of input validation without Psalm complaining. PHPStan has a setting treatPhpDocTypesAsCertain
for that very purpose.
52eb73a
to
f1dfdf8
Compare
4d491c6
to
a848d9d
Compare
d4f1ae5
to
691a021
Compare
691a021
to
2aea57f
Compare
Argh! a conflict! |
2aea57f
to
c1dd1cf
Compare
Blocked by #9774