From 0714850e6ee3c431e9b1fcbac829382732dd1eec Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 30 Oct 2024 06:51:33 +0100 Subject: [PATCH 1/3] Generic/ConstructorName: bug fix - allow for unconventional spacing and comments When determining whether or not a "parent" constructor was being called using a call to a PHP4-style constructor, the sniff would only look at the _next_ token after a double colon, while the next token may be whitespace or a comment, which should be ignored. Fixed now by making the sniff check for the _next non empty_ token instead. Includes tests. --- .../NamingConventions/ConstructorNameSniff.php | 10 ++++++---- .../ConstructorNameUnitTest.inc | 17 +++++++++++++++++ .../ConstructorNameUnitTest.php | 13 ++++++++----- 3 files changed, 31 insertions(+), 9 deletions(-) diff --git a/src/Standards/Generic/Sniffs/NamingConventions/ConstructorNameSniff.php b/src/Standards/Generic/Sniffs/NamingConventions/ConstructorNameSniff.php index adb1d6ba77..7c571e2689 100644 --- a/src/Standards/Generic/Sniffs/NamingConventions/ConstructorNameSniff.php +++ b/src/Standards/Generic/Sniffs/NamingConventions/ConstructorNameSniff.php @@ -15,6 +15,7 @@ use PHP_CodeSniffer\Files\File; use PHP_CodeSniffer\Sniffs\AbstractScopeSniff; +use PHP_CodeSniffer\Util\Tokens; class ConstructorNameSniff extends AbstractScopeSniff { @@ -104,14 +105,15 @@ protected function processTokenWithinScope(File $phpcsFile, $stackPtr, $currScop $endFunctionIndex = $tokens[$stackPtr]['scope_closer']; $startIndex = $stackPtr; while (($doubleColonIndex = $phpcsFile->findNext(T_DOUBLE_COLON, $startIndex, $endFunctionIndex)) !== false) { - if ($tokens[($doubleColonIndex + 1)]['code'] === T_STRING - && strtolower($tokens[($doubleColonIndex + 1)]['content']) === $parentClassNameLc + $nextNonEmpty = $phpcsFile->findNext(Tokens::$emptyTokens, ($doubleColonIndex + 1), null, true); + if ($tokens[$nextNonEmpty]['code'] === T_STRING + && strtolower($tokens[$nextNonEmpty]['content']) === $parentClassNameLc ) { $error = 'PHP4 style calls to parent constructors are not allowed; use "parent::__construct()" instead'; - $phpcsFile->addError($error, ($doubleColonIndex + 1), 'OldStyleCall'); + $phpcsFile->addError($error, $nextNonEmpty, 'OldStyleCall'); } - $startIndex = ($doubleColonIndex + 1); + $startIndex = $nextNonEmpty; } }//end processTokenWithinScope() diff --git a/src/Standards/Generic/Tests/NamingConventions/ConstructorNameUnitTest.inc b/src/Standards/Generic/Tests/NamingConventions/ConstructorNameUnitTest.inc index c680591982..fc2e1478c6 100644 --- a/src/Standards/Generic/Tests/NamingConventions/ConstructorNameUnitTest.inc +++ b/src/Standards/Generic/Tests/NamingConventions/ConstructorNameUnitTest.inc @@ -95,3 +95,20 @@ interface CustomChildCreator { public function customChildCreator($elementName, Project $project); } + +// Bug: unconventional spacing and unexpected comments were not handled correctly. +class UnconventionalSpacing extends MyParent +{ + public function UnconventionalSpacing() { + self :: MyParent(); + static/*comment*/::/*comment*/MyParent(); + } + + public function __construct() { + parent + //comment + :: + // phpcs:ignore Stnd.Cat.SniffName -- for reasons. + MyParent(); + } +} diff --git a/src/Standards/Generic/Tests/NamingConventions/ConstructorNameUnitTest.php b/src/Standards/Generic/Tests/NamingConventions/ConstructorNameUnitTest.php index 616f651347..437eaef585 100644 --- a/src/Standards/Generic/Tests/NamingConventions/ConstructorNameUnitTest.php +++ b/src/Standards/Generic/Tests/NamingConventions/ConstructorNameUnitTest.php @@ -31,11 +31,14 @@ final class ConstructorNameUnitTest extends AbstractSniffUnitTest public function getErrorList() { return [ - 6 => 1, - 11 => 1, - 47 => 1, - 62 => 1, - 91 => 1, + 6 => 1, + 11 => 1, + 47 => 1, + 62 => 1, + 91 => 1, + 103 => 1, + 104 => 1, + 112 => 1, ]; }//end getErrorList() From a3a67d8c812a2c997b1cc53741a50236d4852935 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 30 Oct 2024 07:07:02 +0100 Subject: [PATCH 2/3] Generic/ConstructorName: bug fix - false positive on static call to other class When determining whether or not a "parent" constructor was being called using a call to a PHP4-style constructor, the sniff would only look at the _next_ token after a double colon, disregarding completely the class the method is being called on. This would lead to false positives for method calls to other classes, which would just happen to have a method named the same as the class being extended. This commit fixes this by verifying that the previous non-empty token before the double colon is either a hierarchy keyword or the name of the class being extended and only throwing an error if that's the case. Includes tests. --- .../NamingConventions/ConstructorNameSniff.php | 17 ++++++++++++++--- .../ConstructorNameUnitTest.inc | 18 +++++++++++++++++- .../ConstructorNameUnitTest.php | 4 ++++ 3 files changed, 35 insertions(+), 4 deletions(-) diff --git a/src/Standards/Generic/Sniffs/NamingConventions/ConstructorNameSniff.php b/src/Standards/Generic/Sniffs/NamingConventions/ConstructorNameSniff.php index 7c571e2689..19c8fb4e04 100644 --- a/src/Standards/Generic/Sniffs/NamingConventions/ConstructorNameSniff.php +++ b/src/Standards/Generic/Sniffs/NamingConventions/ConstructorNameSniff.php @@ -106,15 +106,26 @@ protected function processTokenWithinScope(File $phpcsFile, $stackPtr, $currScop $startIndex = $stackPtr; while (($doubleColonIndex = $phpcsFile->findNext(T_DOUBLE_COLON, $startIndex, $endFunctionIndex)) !== false) { $nextNonEmpty = $phpcsFile->findNext(Tokens::$emptyTokens, ($doubleColonIndex + 1), null, true); - if ($tokens[$nextNonEmpty]['code'] === T_STRING - && strtolower($tokens[$nextNonEmpty]['content']) === $parentClassNameLc + if ($tokens[$nextNonEmpty]['code'] !== T_STRING + || strtolower($tokens[$nextNonEmpty]['content']) !== $parentClassNameLc + ) { + $startIndex = $nextNonEmpty; + continue; + } + + $prevNonEmpty = $phpcsFile->findPrevious(Tokens::$emptyTokens, ($doubleColonIndex - 1), null, true); + if ($tokens[$prevNonEmpty]['code'] === T_PARENT + || $tokens[$prevNonEmpty]['code'] === T_SELF + || $tokens[$prevNonEmpty]['code'] === T_STATIC + || ($tokens[$prevNonEmpty]['code'] === T_STRING + && strtolower($tokens[$prevNonEmpty]['content']) === $parentClassNameLc) ) { $error = 'PHP4 style calls to parent constructors are not allowed; use "parent::__construct()" instead'; $phpcsFile->addError($error, $nextNonEmpty, 'OldStyleCall'); } $startIndex = $nextNonEmpty; - } + }//end while }//end processTokenWithinScope() diff --git a/src/Standards/Generic/Tests/NamingConventions/ConstructorNameUnitTest.inc b/src/Standards/Generic/Tests/NamingConventions/ConstructorNameUnitTest.inc index fc2e1478c6..2fb02d6aa1 100644 --- a/src/Standards/Generic/Tests/NamingConventions/ConstructorNameUnitTest.inc +++ b/src/Standards/Generic/Tests/NamingConventions/ConstructorNameUnitTest.inc @@ -101,7 +101,7 @@ class UnconventionalSpacing extends MyParent { public function UnconventionalSpacing() { self :: MyParent(); - static/*comment*/::/*comment*/MyParent(); + MyParent/*comment*/::/*comment*/MyParent(); } public function __construct() { @@ -112,3 +112,19 @@ class UnconventionalSpacing extends MyParent MyParent(); } } + +// Bug: calling a method with the same name as the class being extended on another class, should not be flagged. +class HierarchyKeywordBeforeColon extends MyParent +{ + public function HierarchyKeywordBeforeColon() { + parent::MyParent(); + MyParent::MyParent(); + SomeOtherClass::MyParent(); + } + + public function __construct() { + self::MyParent(); + static::MyParent(); + SomeOtherClass::MyParent(); + } +} diff --git a/src/Standards/Generic/Tests/NamingConventions/ConstructorNameUnitTest.php b/src/Standards/Generic/Tests/NamingConventions/ConstructorNameUnitTest.php index 437eaef585..9df7f37930 100644 --- a/src/Standards/Generic/Tests/NamingConventions/ConstructorNameUnitTest.php +++ b/src/Standards/Generic/Tests/NamingConventions/ConstructorNameUnitTest.php @@ -39,6 +39,10 @@ public function getErrorList() 103 => 1, 104 => 1, 112 => 1, + 120 => 1, + 121 => 1, + 126 => 1, + 127 => 1, ]; }//end getErrorList() From 7b9848460075ef1b5fbf0b3dd8f9d05fbbdba5db Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 30 Oct 2024 07:29:27 +0100 Subject: [PATCH 3/3] Generic/ConstructorName: minor efficiency tweak As things were, the `$startIndex` would be the `function` keyword in the function declaration, which means that the complete declaration (parameters, defaults etc) would be walked, while we only really want to look _inside_ the function body. Fixed by starting the `while` loop on the `scope_opener` instead. Additionally, while the `$startIndex` would be moved forward on each loop, it would still examine one token too much (`scope_opener` cannot be a `T_DOUBLE_COLON`, `T_STRING` after `T_DOUBLE_COLON` cannot be a `T_DOUBLE_COLON`). Also fixed now. --- .../Sniffs/NamingConventions/ConstructorNameSniff.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Standards/Generic/Sniffs/NamingConventions/ConstructorNameSniff.php b/src/Standards/Generic/Sniffs/NamingConventions/ConstructorNameSniff.php index 19c8fb4e04..48e7659e3c 100644 --- a/src/Standards/Generic/Sniffs/NamingConventions/ConstructorNameSniff.php +++ b/src/Standards/Generic/Sniffs/NamingConventions/ConstructorNameSniff.php @@ -91,7 +91,7 @@ protected function processTokenWithinScope(File $phpcsFile, $stackPtr, $currScop } // Stop if the constructor doesn't have a body, like when it is abstract. - if (isset($tokens[$stackPtr]['scope_closer']) === false) { + if (isset($tokens[$stackPtr]['scope_opener'], $tokens[$stackPtr]['scope_closer']) === false) { return; } @@ -103,8 +103,8 @@ protected function processTokenWithinScope(File $phpcsFile, $stackPtr, $currScop $parentClassNameLc = strtolower($parentClassName); $endFunctionIndex = $tokens[$stackPtr]['scope_closer']; - $startIndex = $stackPtr; - while (($doubleColonIndex = $phpcsFile->findNext(T_DOUBLE_COLON, $startIndex, $endFunctionIndex)) !== false) { + $startIndex = $tokens[$stackPtr]['scope_opener']; + while (($doubleColonIndex = $phpcsFile->findNext(T_DOUBLE_COLON, ($startIndex + 1), $endFunctionIndex)) !== false) { $nextNonEmpty = $phpcsFile->findNext(Tokens::$emptyTokens, ($doubleColonIndex + 1), null, true); if ($tokens[$nextNonEmpty]['code'] !== T_STRING || strtolower($tokens[$nextNonEmpty]['content']) !== $parentClassNameLc