-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add missing instanceof check #4401
Conversation
Psalm complains that the condition is redundant:
Should we mute Psalm on this one, or should we remove |
I think if there was a possibility of returning something else, Psalm would complain, so I would remove |
Actually, unless I'm mistaken, there may be a problem with Psalm's analysis here:
And the test is:
But the sample data passes, for example, Is this a bug in Psalm? |
I think it might indeed be a bug, but I cannot reproduce it: https://psalm.dev/r/7cfea887fe (or maybe it should in fact complain here)? UPD: my reproducer is wrong, |
Suprising! Not sure how to report it then. Should we suppress the Psalm error in the meantime? |
Well I can reproduce it locally, so a reproducer based on |
@greg0ire Done! |
sebastianbergmann/phpunit#4509 has already been merged, so the fix should land in the next PHPUnit 8.5 release. Until then, |
Thanks for taking care of this and identifying the upstream issue. I also noticed this issue while merging these changes up but never prioritized fixing it. |
<errorLevel type="suppress"> | ||
<!-- | ||
Requires a release of | ||
https://github.com/sebastianbergmann/phpunit/commit/9c60d7d9fd3bfa80fa4aeab7090e1bbe0830dbcd |
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.
In the future, I believe it's better to identify the issue by the GitHub issue rather than a commit. Otherwise, it requires extra navigation which may not always work (e.g. if commit gets rebased or processed in some other way).
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 advise this because the commit view allows to see which tags/branches it is on. I wonder if that works in case of "Rebase and merge" / "squash"
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.
You can always navigate from issue to commit, not sure the opposite is true?
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.
In most cases you can, I'm not sure if there are exceptions. Anyway, the PR does not contain information about branches and tags, only the commit does AFAIK
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.
The exception is when a PR gets rebased (effectively, excluded from the tree). If the navigation works from the commit to the PR, it will work the other way around as well. At the same time, a PR or a commit not always exists (it can be just an issue), so from the consistency standpoint, it's better to link to issues and PRs instead of issues and commits. Additionally, issues usually contain more context, discussions, labels, etc.
Summary
The
$expectedClass
variable is currently unused. It should be used for aninstanceof
check.