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

Extra: Replace Squiz.Scope.MemberVarScope with PSR2.Classes.PropertyDeclaration #1592

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Dec 23, 2018

PR #1103 added the Squiz.Scope.MemberVarScope sniff to the WordPress-Extra ruleset which checks that all class properties have their visibility declared.

The upstream PSR2.Classes.PropertyDeclaration sniff also does this, however it contains more extensive checks.
In addition to checking for the visibility of properties being declared, this sniff also checks:

  • That property names are not prefixed with an underscore _ to indicate visibility.
    For methods, we already check this in the Extra ruleset via the PSR2.Methods.MethodDeclaration sniff.
  • That the var keyword is not used for property declarations.
    This could be considered a duplicate check with the "use visibility" check.
  • That each property declaration statement only declares one property, i.e. it forbids the use of code like this:
    public $prop1 = true,
        $prop2 = '',
        $prop3 = 123;
    As there is no precedent or reference to this type of property declarations in the handbook or elsewhere in the various rulesets, I'm not sure that we're ready to take a stand on this type of coding, so I've excluded this particular error code for now.
    Even though there is no precedent or reference to this type of property declarations in the handbook or elsewhere in the various rulesets, WP Core itself does not appear to use multi-property assignments, so we may as well check leave the check in for now.
    People can always still exclude it in their own custom rulesets and if we'd get a lot of push-back, we can reconsider.
  • As of PHPCS 3.4.0: that the order of the property keywords is visibility static, not static visibility (includes fixer).
    For methods, we already check this in the Extra ruleset via the PSR2.Methods.MethodDeclaration sniff.

All in all, using the PSR2 sniff is more in line with the checks we already have in place for methods, so IMO, this seemed like a good change to make.

…tyDeclaration`

PR 1103 added the `Squiz.Scope.MemberVarScope` sniff to the `WordPress-Extra` ruleset which checks that all class properties have their visibility declared.

The upstream `PSR2.Classes.PropertyDeclaration` sniff also does this, however it contains more extensive checks.
In addition to checking for the visibility of properties being declared, this sniff also checks:
* That property names are not prefixed with an underscore `_` to indicate visibility.
    _For methods, we already check this in the `Extra` ruleset via the `PSR2.Methods.MethodDeclaration` sniff._
* That the `var` keyword is not used for property declarations.
    This could be considered a duplicate check with the "use visibility" check.
* That each property declaration statement only declares one property, i.e. it forbids the use of code like this:
    ```php
    public $prop1 = true,
        $prop2 = '',
        $prop3 = 123;
    ```
    Even though there is no precedent or reference to this type of property declarations in the handbook or elsewhere in the various rulesets, WP Core itself does not appear to use multi-property assignments, so we may as well check leave the check in for now.
    People can always still exclude it in their own custom rulesets and if we'd get a lot of push-back, we can reconsider.
* As of PHPCS 3.4.0: that the order of the property keywords is `visibility static`, not `static visibility` (includes fixer).
    _For methods, we already check this in the `Extra` ruleset via the `PSR2.Methods.MethodDeclaration` sniff._

All in all, using the `PSR2` sniff is more in line with the checks we already have in place for methods, so IMO, this seemed like a good change to make.
@jrfnl jrfnl force-pushed the feature/extra-replace-squiz-membervarscope-with-psr2-version branch from 470f7b5 to c8f8c97 Compare December 23, 2018 14:42
@GaryJones GaryJones merged commit bcc3b85 into develop Dec 23, 2018
@GaryJones GaryJones deleted the feature/extra-replace-squiz-membervarscope-with-psr2-version branch December 23, 2018 18:49
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.

2 participants