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

Implement AbstractProtectedMethodRule #2758

Merged
merged 4 commits into from Nov 21, 2023
Merged

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Nov 21, 2023

interfaces cannot contain non-public methods

https://3v4l.org/3VBl2

use function sprintf;

/** @implements Rule<InClassMethodNode> */
class AbstractProtectedMethodRule implements Rule
Copy link
Member

Choose a reason for hiding this comment

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

I'd make it clear this is only about interface, both in rule class name, and the error message

Copy link
Contributor Author

@staabm staabm Nov 21, 2023

Choose a reason for hiding this comment

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

I also though about that.

DX wise there are a lot of error situations. I think we need to make sure, that we don't report too much errors at a time.

thats also the reason why I did not cover interfaces in AbstractPrivateMethodRule in the first place, as if we use a different error message in this rule here it would be kind of inconsistent to have

interface fooInterface {
	abstract private function sayPrivate() : void; // 'Private method fooInterface::sayPrivate() cannot be abstract.'
	abstract protected function sayProtected() : void; // 'Protected method fooInterface::sayPrivate() cannot be abstract in interface.'
	abstract public function sayPublic() : void;
}

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with saying Protected method fooInterface::sayProtected() cannot be abstract in interface..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the PR to error about method visibility in interfaces (no matter abstract method or not)

@staabm staabm marked this pull request as ready for review November 21, 2023 09:41
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@ondrejmirtes ondrejmirtes merged commit abf65a7 into phpstan:1.10.x Nov 21, 2023
425 checks passed
@ondrejmirtes
Copy link
Member

Thank you.

@staabm staabm deleted the prot2 branch November 21, 2023 09:45
@ondrejmirtes
Copy link
Member

FYI just noticed a typo in the class name MethodVisibitiliyInInterfaceRule

@staabm
Copy link
Contributor Author

staabm commented Nov 26, 2023

ohh good catch. do you take care of it, or should I PR a name-change?

@ondrejmirtes
Copy link
Member

Please send a PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants