From 808c905066a0977119d336611cb893b8557eb0f3 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 28 Oct 2022 09:37:37 +0200 Subject: [PATCH 1/2] Core: move rules related to property/method modifier order from `Extra` to `Core` > 1. When using multiple modifiers for a property or method declaration, the order should be as follows: > 1. First the optional `abstract` or `final` keyword. > 2. Next a visibility keyword. > 3. Lastly, the optional `static` [red: or `readonly`] keyword. For property declarations, this is already covered by the `PSR2.Classes.PropertyDeclaration` sniff. For method declarations, the `PSR2.Methods.MethodDeclaration` sniff has now been moved from `Extra` to `Core`. The `PSR2.Methods.MethodDeclaration` sniff contains one additional error code, which IMO should probably not (yet) go into the `Core` ruleset. * `Underscore` - which warns against prefixing a method name with an underscore, advising to use visibility instead. While I'm 100% in favour of this, introducing this for WP Core would be problematic as renaming (`public`/`protected`) methods would be a BC-break. As this rule was previously already included in the `Extra` ruleset, the error code which is being silenced for `Core`, will be "unsilenced" again for `Extra`. The `PSR2.Classes.PropertyDeclaration` sniff does not yet account for ordering the visibility vs readonly keywords. I have opened a PR upstream - squizlabs/PHP_CodeSniffer 3637 - proposing to add a check for this to the `PSR2.Classes.PropertyDeclaration` sniff. * If that PR would be accepted, WPCS will automatically inherit the check and we'd be good. * If that PR would be rejected, I propose to create a dedicated sniff in PHPCSExtra to handle property modifier keywords with configuration options for the preferred order. Refs: * https://make.wordpress.org/core/2020/03/20/updating-the-coding-standards-for-modern-php/ - Visibility should always be declared section * https://developer.wordpress.org/coding-standards/wordpress-coding-standards/php/#visibility-and-modifier-order * WordPress/wpcs-docs 108 * WordPress/WordPress-Coding-Standards 1101 * WordPress/WordPress-Coding-Standards 1103 * squizlabs/PHP_CodeSniffer 3637 --- WordPress-Core/ruleset.xml | 31 +++++++++++++++++++++++++++++++ WordPress-Extra/ruleset.xml | 4 +++- 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/WordPress-Core/ruleset.xml b/WordPress-Core/ruleset.xml index 2fc7299c89..05e4bf67d9 100644 --- a/WordPress-Core/ruleset.xml +++ b/WordPress-Core/ruleset.xml @@ -387,6 +387,37 @@ + + + + + + + + + + + + 0 + + + - 5 + + 5 + From d4402e51bcf9f3966fd57bfadf86a87185f0c343 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 23 Jul 2022 23:32:48 +0200 Subject: [PATCH 2/2] Core: move last modifier keyword sniff from Extra to Core While this was not explicitly mentioned in the Make post, it is implied that there should be exactly one space after each modifier keyword for properties and methods. This was already being checked for in `Extra`. I'm proposing to move this sniff to the `Core` ruleset. Note: I've checked and introducing this sniff to the `Core` ruleset does not yield any issues for WP Core as is, so it would be a silent introduction. Refs: * WordPress/WordPress-Coding-Standards 1101 --- WordPress-Core/ruleset.xml | 3 +++ WordPress-Extra/ruleset.xml | 4 ---- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/WordPress-Core/ruleset.xml b/WordPress-Core/ruleset.xml index 05e4bf67d9..323ed05023 100644 --- a/WordPress-Core/ruleset.xml +++ b/WordPress-Core/ruleset.xml @@ -417,6 +417,9 @@ 0 + + + - -