Point method visibility errors at the method, not its first attribute - #6630
Merged
Merged
Conversation
- A method's start line includes its attribute groups, so "should also be public" and "should be protected or public" landed on the first attribute line. Both errors now use the line of the method name. - MethodVisibilityComparisonHelper::compare() takes the ClassMethod node to read that line; OverridingMethodRule and ConsistentConstructorRule pass it. - Tests cover an attribute, a docblock with a multi-line attribute, two attribute groups and a method without attributes, plus a consistent constructor with an attribute.
staabm
approved these changes
Sep 30, 2026
Contributor
There was a problem hiding this comment.
lgtm. maybe you are interessted in working on a similar problem in a new PR reported in phpstan/phpstan#10714
this PR merges cleanly into 2.3.x so its fine for 2.2.x.
please target future PRs on 2.3.x
thank you
Contributor
|
//cc @SanderMuller |
VincentLanglet
approved these changes
Sep 30, 2026
Contributor
|
Thank you |
Author
Thank you. I'll check it out |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes phpstan/phpstan#14398
A method's start line includes its attribute groups, so "overriding public method should also be public" and "should be protected or public" were reported on the first attribute line (
#[\Override]in the issue) instead of the method.MethodVisibilityComparisonHelper::compare()now takes theClassMethodand reports both errors on the line of the method name, the same->line($node->name->getStartLine())other rules use for names. Its two callers,OverridingMethodRuleandConsistentConstructorRule, pass$node->getOriginalNode().Without the change, the new tests get the attribute lines (39, 47, 54 and 24) instead of the method lines.
Other errors from
OverridingMethodRule(final method,#[\Override], parameter and return types) still point at the attribute line. I kept this PR to the reported error and can move the others in a follow-up if you want them on the method line too.