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

Fixed ReflectionMethod::isAbstract() in adapter #586

Merged
merged 1 commit into from
May 26, 2020

Conversation

kukulich
Copy link
Collaborator

@@ -273,7 +273,7 @@ public function isProtected()
*/
public function isAbstract()
{
return $this->betterReflectionMethod->isAbstract();
return $this->betterReflectionMethod->isAbstract() || $this->betterReflectionMethod->getDeclaringClass()->isInterface();
Copy link
Member

Choose a reason for hiding this comment

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

@kukulich I think this should probable be fixed in \Roave\BetterReflection\Reflection\ReflectionClass::isAbstract too, perhaps?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It looks the current version is right: https://3v4l.org/EBjjL

Copy link
Member

Choose a reason for hiding this comment

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

wow, PHP is confusing 🤣

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I made a mistake, I meant the BR ReflectionMethod @kukulich 🙄 sorry

Copy link
Member

Choose a reason for hiding this comment

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

Reported in #596

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@asgrim I'm not sure. BR is "better" so it should maybe report only really abstract methods...

But there's ReflectionMethod::getModifiers() based also on isAbstract()...

You tell me how I should fix it :D

Copy link
Contributor

Choose a reason for hiding this comment

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

I wasn't kidding when I said that PHPStan is an integration test suite for BetterReflection :D https://twitter.com/OndrejMirtes/status/1261621042176548865

Copy link
Member

Choose a reason for hiding this comment

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

You tell me how I should fix it :D

Oooooooooof

Guess we'll need more tests, but if you have a bug or scenario in mind, report a new issue referencing this thread.

The test suite and code are in good shape enough to "outsource" all of this work to the twitters :-)

@asgrim asgrim requested a review from Ocramius May 26, 2020 14:23
@asgrim asgrim added this to the 4.3.0 milestone May 26, 2020
@asgrim asgrim merged commit a1b32d1 into Roave:master May 26, 2020
@asgrim
Copy link
Member

asgrim commented May 26, 2020

Oops, merged the wrong MR 🙄 @kukulich see comment above 😁

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.

None yet

4 participants