-
Notifications
You must be signed in to change notification settings - Fork 502
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
Conversation
use function sprintf; | ||
|
||
/** @implements Rule<InClassMethodNode> */ | ||
class AbstractProtectedMethodRule implements Rule |
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'd make it clear this is only about interface, both in rule class name, and the error message
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 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.
- methods must be
public
in interfaces https://3v4l.org/tmQX8 - methods are not allowed to be declared
abstract
in interfaces https://3v4l.org/rlVKF - the combination of the 2:
private abstract
orprotected abstract
in interfaces https://3v4l.org/LR2DP - final on abstract is not allowed https://phpstan.org/r/f7056d2d-412e-4698-910f-e5e9439e0648 ->
final private abstract
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;
}
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'm fine with saying Protected method fooInterface::sayProtected() cannot be abstract in interface.
.
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 changed the PR to error about method visibility in interfaces (no matter abstract method or not)
This pull request has been marked as ready for review. |
Thank you. |
FYI just noticed a typo in the class name |
ohh good catch. do you take care of it, or should I PR a name-change? |
Please send a PR |
interfaces cannot contain non-public methods
https://3v4l.org/3VBl2