-
-
Notifications
You must be signed in to change notification settings - Fork 473
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
CS/QA: make (nearly) all classes final
#2200
Conversation
The PHPCS autoloader does not always play nice with sniffs which extend other sniffs, especially if both sniffs (parent and child) are included in the same standard. To discourage sniffs extending each other, I propose making all non-abstract sniff classes `final`with the exception of four classes of which it is known that those are being extended based on a public code search. The four exceptions are: * `NamingConventions.ValidHookName` * `Security.EscapeOutput` * `Security.NonceVerification` * `Security.ValidatedSanitizedInput` If needs be, a sniff in an external standard can still _use_ logic from the WPCS sniffs. On the one hand, they can use the new `Helper` classes and traits. On the other hand, they can do a preliminary check on certain data and defer to a new or preinstantiated instance of a WPCS sniff for everything else. Note: this list of sniffs being extended is likely incomplete. If reports come in from external standards that they are running into trouble using alternative logic, we can still remove the `final` keyword from select sniffs in a patch release. However, adding the `final` keyword needs to be done in a major. Ref: * https://sourcegraph.com/search?q=context:global+use+WordPressCS+-repo:%5Egithub%5C.com/WordPress/WordPress-Coding-Standards%24+-repo:%5Egithub%5C.com/WPTT/WPThemeReview%24+-file:%5E%28.*/%29%3Fvendor/wp-coding-standards/.*%24&patternType=standard&case=yes&sm=0
... for the files in this package.
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 checked VIPCS, and it only extends abstract classes from WPCS.
When I was tinkering before, I think I had trouble loading these from WPCS, so we might need some documentation here on what, if anything, needs to be done to make these available to external standards. |
If both standards are registered with PHPCS (which the Composer plugin takes care of) and if all files basically follow the namespace scheme which the PHPCS autoloader expects, everything should work.... I'd need to see the errors you were getting in VIPCS to advise on how to fix that, but that'll have to wait for now. |
QA: make all test classes final
QA: make all non-abstract sniff classes final
The PHPCS autoloader does not always play nice with sniffs which extend other sniffs, especially if both sniffs (parent and child) are included in the same standard.
To discourage sniffs extending each other, I propose making all non-abstract sniff classes
final
with the exception of four classes of which it is known that those are being extended based on a public code search.The four exceptions are:
NamingConventions.ValidHookName
Security.EscapeOutput
Security.NonceVerification
Security.ValidatedSanitizedInput
If needs be, a sniff in an external standard can still use logic from the WPCS sniffs. On the one hand, they can use the new
Helper
classes and traits. On the other hand, they can do a preliminary check on certain data and defer to a new or preinstantiated instance of a WPCS sniff for everything else.Note: this list of sniffs being extended is likely incomplete.
If reports come in from external standards that they are running into trouble using alternative logic, we can still remove the
final
keyword from select sniffs in a patch release. However, adding thefinal
keyword needs to be done in a major.Ref:
PHPCS: enforce classes to be abstract or final
... for the files in this package.