From 8cdc1221e53a0c9b2f977ce36d87c8ff11dc125b Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 23 Jul 2022 22:05:41 +0200 Subject: [PATCH] PHP 8.1 | PSR2/PropertyDeclaration: add check for visibility before readonly keyword The `PSR2.Classes.PropertyDeclaration` sniff includes a check to verify that the `static` keyword is _after_ the visibility in a property declaration. PHP 8.1 introduced the `readonly` keyword for properties. [PSR PER 2.0.0](https://www.php-fig.org/per/coding-style/#46-modifier-keywords) dictates that visibility is declared before the `readonly` keyword. All code samples in both the RFC as well as the PHP manual also use the `visibility - readonly` modifier keyword order, so it is likely that this will become the standard modifier keyword order. A search for PHP projects which have started to use the `readonly` keyword, also shows these predominantly use the `visibility - readonly` modifier keyword order. With that in mind, I'm proposing to add a check for this order to the `PSR2.Classes.PropertyDeclaration` sniff - this being the only PHPCS native sniff which checks the modifier keyword order for properties. As this sniff is included in PSR2 and PSR12, the new check will automatically apply the PSR-PER property modifier keyword order rules to PSR2/PSR12. Includes unit tests. Ref: * https://wiki.php.net/rfc/readonly_properties_v2 * https://www.php.net/manual/en/language.oop5.properties.php#language.oop5.properties.readonly-properties * https://sourcegraph.com/search?q=context:global+%5Cs%28public%7Cprotected%7Cprivate%29%5Cs%2Breadonly%5Cs%2B+lang:php+-file:%28%5E%7C/%29%28vendor%7Ctests%3F%29/+fork:no+archived:no&patternType=regexp (searching `visibility - readonly` order - > 500 results) * https://sourcegraph.com/search?q=context:global+%5Csreadonly%5Cs%2B%28public%7Cprotected%7Cprivate%29%5Cs%2B+lang:php+-file:%28%5E%7C/%29%28vendor%7Ctests%3F%29/+fork:no+archived:no&patternType=regexp (searching `readonly - visibility` order - 41 (non false positive) results) --- .../Classes/PropertyDeclarationSniff.php | 68 +++++++++++++++---- .../Classes/PropertyDeclarationUnitTest.inc | 4 ++ .../PropertyDeclarationUnitTest.inc.fixed | 4 ++ .../Classes/PropertyDeclarationUnitTest.php | 2 + 4 files changed, 64 insertions(+), 14 deletions(-) diff --git a/src/Standards/PSR2/Sniffs/Classes/PropertyDeclarationSniff.php b/src/Standards/PSR2/Sniffs/Classes/PropertyDeclarationSniff.php index efdbb43827..aca0be2d0d 100644 --- a/src/Standards/PSR2/Sniffs/Classes/PropertyDeclarationSniff.php +++ b/src/Standards/PSR2/Sniffs/Classes/PropertyDeclarationSniff.php @@ -118,30 +118,70 @@ protected function processMemberVar(File $phpcsFile, $stackPtr) $phpcsFile->addError($error, $stackPtr, 'ScopeMissing', $data); } + /* + * Note: per PSR-PER section 4.6, the order should be: + * - Inheritance modifier: `abstract` or `final`. + * - Visibility modifier: `public`, `protected`, or `private`. + * - Scope modifier: `static`. + * - Mutation modifier: `readonly`. + * - Type declaration. + * - Name. + * + * Ref: https://www.php-fig.org/per/coding-style/#46-modifier-keywords + * + * At this time (PHP 8.2), inheritance modifiers cannot be applied to properties and + * the `static` and `readonly` modifiers are mutually exclusive and cannot be used together. + * + * Based on that, the below modifier keyword order checks are sufficient (for now). + */ + if ($propertyInfo['scope_specified'] === true && $propertyInfo['is_static'] === true) { $scopePtr = $phpcsFile->findPrevious(Tokens::$scopeModifiers, ($stackPtr - 1)); $staticPtr = $phpcsFile->findPrevious(T_STATIC, ($stackPtr - 1)); - if ($scopePtr < $staticPtr) { - return; - } + if ($scopePtr > $staticPtr) { + $error = 'The static declaration must come after the visibility declaration'; + $fix = $phpcsFile->addFixableError($error, $stackPtr, 'StaticBeforeVisibility'); + if ($fix === true) { + $phpcsFile->fixer->beginChangeset(); - $error = 'The static declaration must come after the visibility declaration'; - $fix = $phpcsFile->addFixableError($error, $stackPtr, 'StaticBeforeVisibility'); - if ($fix === true) { - $phpcsFile->fixer->beginChangeset(); + for ($i = ($scopePtr + 1); $scopePtr < $stackPtr; $i++) { + if ($tokens[$i]['code'] !== T_WHITESPACE) { + break; + } - for ($i = ($scopePtr + 1); $scopePtr < $stackPtr; $i++) { - if ($tokens[$i]['code'] !== T_WHITESPACE) { - break; + $phpcsFile->fixer->replaceToken($i, ''); } - $phpcsFile->fixer->replaceToken($i, ''); + $phpcsFile->fixer->replaceToken($scopePtr, ''); + $phpcsFile->fixer->addContentBefore($staticPtr, $propertyInfo['scope'].' '); + + $phpcsFile->fixer->endChangeset(); } + } + }//end if + + if ($propertyInfo['scope_specified'] === true && $propertyInfo['is_readonly'] === true) { + $scopePtr = $phpcsFile->findPrevious(Tokens::$scopeModifiers, ($stackPtr - 1)); + $readonlyPtr = $phpcsFile->findPrevious(T_READONLY, ($stackPtr - 1)); + if ($scopePtr > $readonlyPtr) { + $error = 'The readonly declaration must come after the visibility declaration'; + $fix = $phpcsFile->addFixableError($error, $stackPtr, 'ReadonlyBeforeVisibility'); + if ($fix === true) { + $phpcsFile->fixer->beginChangeset(); + + for ($i = ($scopePtr + 1); $scopePtr < $stackPtr; $i++) { + if ($tokens[$i]['code'] !== T_WHITESPACE) { + break; + } + + $phpcsFile->fixer->replaceToken($i, ''); + } - $phpcsFile->fixer->replaceToken($scopePtr, ''); - $phpcsFile->fixer->addContentBefore($staticPtr, $propertyInfo['scope'].' '); + $phpcsFile->fixer->replaceToken($scopePtr, ''); + $phpcsFile->fixer->addContentBefore($readonlyPtr, $propertyInfo['scope'].' '); - $phpcsFile->fixer->endChangeset(); + $phpcsFile->fixer->endChangeset(); + } } }//end if diff --git a/src/Standards/PSR2/Tests/Classes/PropertyDeclarationUnitTest.inc b/src/Standards/PSR2/Tests/Classes/PropertyDeclarationUnitTest.inc index 33bec44e70..3e086c6f22 100644 --- a/src/Standards/PSR2/Tests/Classes/PropertyDeclarationUnitTest.inc +++ b/src/Standards/PSR2/Tests/Classes/PropertyDeclarationUnitTest.inc @@ -80,4 +80,8 @@ class ReadOnlyProp { protected readonly ?string $foo; readonly array $foo; + + readonly public int $wrongOrder1; + + readonly protected ?string $wrongOrder2; } diff --git a/src/Standards/PSR2/Tests/Classes/PropertyDeclarationUnitTest.inc.fixed b/src/Standards/PSR2/Tests/Classes/PropertyDeclarationUnitTest.inc.fixed index df83112af2..c4e22fc18b 100644 --- a/src/Standards/PSR2/Tests/Classes/PropertyDeclarationUnitTest.inc.fixed +++ b/src/Standards/PSR2/Tests/Classes/PropertyDeclarationUnitTest.inc.fixed @@ -77,4 +77,8 @@ class ReadOnlyProp { protected readonly ?string $foo; readonly array $foo; + + public readonly int $wrongOrder1; + + protected readonly ?string $wrongOrder2; } diff --git a/src/Standards/PSR2/Tests/Classes/PropertyDeclarationUnitTest.php b/src/Standards/PSR2/Tests/Classes/PropertyDeclarationUnitTest.php index f1dd0194d2..6a1778b904 100644 --- a/src/Standards/PSR2/Tests/Classes/PropertyDeclarationUnitTest.php +++ b/src/Standards/PSR2/Tests/Classes/PropertyDeclarationUnitTest.php @@ -49,6 +49,8 @@ public function getErrorList() 76 => 1, 80 => 1, 82 => 1, + 84 => 1, + 86 => 1, ]; }//end getErrorList()