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 implementsInterface() PHPDoc #277

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

herndlm
Copy link

@herndlm herndlm commented Oct 5, 2022

class_implements accepts either an object or a class-string, but the PHPDoc for the assert function is wrong and limits its functionality to a class-string only. I'm not entirely sure if this was on purpose, the README just mentions "Check that a class implements an interface", but it would work with either an instance of a class or a class-string and the assertion is under the "object" category.

See also https://phpstan.org/r/c72bb8af-4f27-462b-85f6-e2d3b81b9786 where this is tested in PHPStan (which just got support for PHPDoc-based type narrowing, this is not released yet)

Refs: phpstan/phpstan-webmozart-assert#144 (comment)

I could not cleanly run psalm locally without unrelated errors, let's see what happens on CI 🤞

@herndlm herndlm marked this pull request as ready for review October 5, 2022 12:24
@BackEndTea
Copy link
Collaborator

The docs look good, could you add some test cases using objects, so we cover it in our unit tests?

@herndlm herndlm force-pushed the fix-implements-interface-phpdoc branch from d5a97ab to 9ff015b Compare October 7, 2022 18:56
@herndlm
Copy link
Author

herndlm commented Oct 7, 2022

The docs look good, could you add some test cases using objects, so we cover it in our unit tests?

sure, adapted

@herndlm
Copy link
Author

herndlm commented Oct 16, 2022

@BackEndTea in case you didn't see it: should be adapted. In case you're just busy: no worries, this is not important 😊

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.

2 participants