From f3f61aa143eca9104d16b8c4cc0dd9e0780ddee7 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Tue, 25 Feb 2025 10:37:09 +0100 Subject: [PATCH 1/6] Squiz/ClassFileName: expand the tests This commit adds some extra tests for the sniff. * Document handling of unfinished OO declarations (which contain enough info to still action). * Safeguard that comments in unexpected places doesn't cause problems. As this sniff looks at the file name of the file containing the tests, we cannot use the _normal_ `SniffNameUnitTest.#.inc` naming conventions for extra tests files, so the `getTestFiles()` method from the `AbstractSniffUnitTest` class needs to be overloaded to retrieve the test case files in a slightly different way. --- .../ClassFileNameLiveCodingFailUnitTest.inc | 6 + .../ClassFileNameLiveCodingPassUnitTest.inc | 6 + .../Tests/Classes/ClassFileNameUnitTest.inc | 9 +- .../Tests/Classes/ClassFileNameUnitTest.php | 116 +++++++++++++----- 4 files changed, 101 insertions(+), 36 deletions(-) create mode 100644 src/Standards/Squiz/Tests/Classes/ClassFileNameLiveCodingFailUnitTest.inc create mode 100644 src/Standards/Squiz/Tests/Classes/ClassFileNameLiveCodingPassUnitTest.inc diff --git a/src/Standards/Squiz/Tests/Classes/ClassFileNameLiveCodingFailUnitTest.inc b/src/Standards/Squiz/Tests/Classes/ClassFileNameLiveCodingFailUnitTest.inc new file mode 100644 index 0000000000..bda3f6d08d --- /dev/null +++ b/src/Standards/Squiz/Tests/Classes/ClassFileNameLiveCodingFailUnitTest.inc @@ -0,0 +1,6 @@ + +enum + // Comment + ExtraClassFileNameUnitTest {} diff --git a/src/Standards/Squiz/Tests/Classes/ClassFileNameUnitTest.php b/src/Standards/Squiz/Tests/Classes/ClassFileNameUnitTest.php index 7a01e9c09f..d4ea028d67 100644 --- a/src/Standards/Squiz/Tests/Classes/ClassFileNameUnitTest.php +++ b/src/Standards/Squiz/Tests/Classes/ClassFileNameUnitTest.php @@ -9,6 +9,7 @@ namespace PHP_CodeSniffer\Standards\Squiz\Tests\Classes; +use DirectoryIterator; use PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest; /** @@ -20,46 +21,99 @@ final class ClassFileNameUnitTest extends AbstractSniffUnitTest { + /** + * Get a list of all test files to check. + * + * These will have the same base as the sniff name but different extensions. + * We ignore the .php file as it is the class. + * + * @param string $testFileBase The base path that the unit tests files will have. + * + * @return string[] + */ + protected function getTestFiles($testFileBase) + { + $testFiles = []; + + $dir = substr($testFileBase, 0, strrpos($testFileBase, DIRECTORY_SEPARATOR)); + $di = new DirectoryIterator($dir); + + // Strip off the path and the "UnitTest." suffix from the $testFileBase to allow + // for some less conventionally named test case files. + $fileBase = str_replace($dir, '', $testFileBase); + $fileBase = substr($fileBase, 1, -9); + + foreach ($di as $file) { + $fileName = $file->getBasename('UnitTest.inc'); + $extension = $file->getExtension(); + if (substr($fileName, 0, strlen($fileBase)) === $fileBase + && $extension === 'inc' + ) { + $testFiles[] = $file->getPathname(); + } + } + + // Put them in order. + sort($testFiles, SORT_NATURAL); + + return $testFiles; + + }//end getTestFiles() + + /** * Returns the lines where errors should occur. * * The key of the array should represent the line number and the value * should represent the number of errors that should occur on that line. * + * @param string $testFile The name of the file being tested. + * * @return array */ - public function getErrorList() + public function getErrorList($testFile='') { - return [ - 12 => 1, - 13 => 1, - 14 => 1, - 15 => 1, - 16 => 1, - 17 => 1, - 18 => 1, - 19 => 1, - 20 => 1, - 21 => 1, - 22 => 1, - 23 => 1, - 27 => 1, - 28 => 1, - 29 => 1, - 30 => 1, - 31 => 1, - 32 => 1, - 33 => 1, - 34 => 1, - 35 => 1, - 36 => 1, - 37 => 1, - 38 => 1, - 39 => 1, - 40 => 1, - 41 => 1, - 42 => 1, - ]; + switch ($testFile) { + case 'ClassFileNameUnitTest.inc': + return [ + 12 => 1, + 13 => 1, + 14 => 1, + 15 => 1, + 16 => 1, + 17 => 1, + 18 => 1, + 19 => 1, + 20 => 1, + 21 => 1, + 22 => 1, + 23 => 1, + 27 => 1, + 28 => 1, + 29 => 1, + 30 => 1, + 31 => 1, + 32 => 1, + 33 => 1, + 34 => 1, + 35 => 1, + 36 => 1, + 37 => 1, + 38 => 1, + 39 => 1, + 40 => 1, + 41 => 1, + 42 => 1, + ]; + + case 'ClassFileNameLiveCodingFailUnitTest.inc': + return [ + 6 => 1, + ]; + + default: + return []; + }//end switch }//end getErrorList() From 91e4b67c9e7923573d6cb2ccdbc88a50ecbf8fc4 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Tue, 25 Feb 2025 09:24:50 +0100 Subject: [PATCH 2/6] Squiz/ClassFileName: minor stability tweak Use the predefined `Tokens::$ooScopeTokens` array to automatically benefit from new OO structures being added to that list. --- .../Squiz/Sniffs/Classes/ClassFileNameSniff.php | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/Standards/Squiz/Sniffs/Classes/ClassFileNameSniff.php b/src/Standards/Squiz/Sniffs/Classes/ClassFileNameSniff.php index fc3ec0473b..b42c6ffe0e 100644 --- a/src/Standards/Squiz/Sniffs/Classes/ClassFileNameSniff.php +++ b/src/Standards/Squiz/Sniffs/Classes/ClassFileNameSniff.php @@ -11,6 +11,7 @@ use PHP_CodeSniffer\Files\File; use PHP_CodeSniffer\Sniffs\Sniff; +use PHP_CodeSniffer\Util\Tokens; class ClassFileNameSniff implements Sniff { @@ -23,12 +24,10 @@ class ClassFileNameSniff implements Sniff */ public function register() { - return [ - T_CLASS, - T_INTERFACE, - T_TRAIT, - T_ENUM, - ]; + $targets = Tokens::$ooScopeTokens; + unset($targets[T_ANON_CLASS]); + + return $targets; }//end register() From ce14c669cd95e1524c20578463218c11619c8304 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Tue, 25 Feb 2025 10:51:08 +0100 Subject: [PATCH 3/6] Squiz/ClassFileName: bow out earlier for STDIN As `STDIN` won't contain a usable file name, bow out and don't examine the input again. _Note: the `process()` method should really end on a `return $this->phpcsFile->numTokens;` as well as a file containing multiple classes should not get multiple contradicting errors, but that would require each test for this sniff to be in their own file, which I currently don't deem worth the effort._ --- .../Squiz/Sniffs/Classes/ClassFileNameSniff.php | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/Standards/Squiz/Sniffs/Classes/ClassFileNameSniff.php b/src/Standards/Squiz/Sniffs/Classes/ClassFileNameSniff.php index b42c6ffe0e..7a01c68ee0 100644 --- a/src/Standards/Squiz/Sniffs/Classes/ClassFileNameSniff.php +++ b/src/Standards/Squiz/Sniffs/Classes/ClassFileNameSniff.php @@ -43,13 +43,14 @@ public function register() */ public function process(File $phpcsFile, $stackPtr) { - $fullPath = basename($phpcsFile->getFilename()); - $fileName = substr($fullPath, 0, strrpos($fullPath, '.')); - if ($fileName === '') { - // No filename probably means STDIN, so we can't do this check. - return; + $filename = $phpcsFile->getFilename(); + if ($filename === 'STDIN') { + return $phpcsFile->numTokens; } + $fullPath = basename($filename); + $fileName = substr($fullPath, 0, strrpos($fullPath, '.')); + $tokens = $phpcsFile->getTokens(); $decName = $phpcsFile->findNext(T_STRING, $stackPtr); From 8f928f79bace48b1a89cf46a3daf95ebcb3903b6 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Tue, 25 Feb 2025 11:03:24 +0100 Subject: [PATCH 4/6] Squiz/ClassFileName: don't error when there is no class name As things were, the sniff would try to find the next `T_STRING`, but didn't take live coding into account, which means it would end up comparing `$tokens[false]['content']` to the filename, which would end up comparing `getTokens(); - $decName = $phpcsFile->findNext(T_STRING, $stackPtr); + $tokens = $phpcsFile->getTokens(); + $ooName = $phpcsFile->getDeclarationName($stackPtr); + if ($ooName === null) { + // Probably parse error/live coding. + return; + } - if ($tokens[$decName]['content'] !== $fileName) { + if ($ooName !== $fileName) { $error = '%s name doesn\'t match filename; expected "%s %s"'; $data = [ ucfirst($tokens[$stackPtr]['content']), diff --git a/src/Standards/Squiz/Tests/Classes/ClassFileNameParseError1UnitTest.inc b/src/Standards/Squiz/Tests/Classes/ClassFileNameParseError1UnitTest.inc new file mode 100644 index 0000000000..d66403d822 --- /dev/null +++ b/src/Standards/Squiz/Tests/Classes/ClassFileNameParseError1UnitTest.inc @@ -0,0 +1,6 @@ + Date: Tue, 25 Feb 2025 10:21:16 +0100 Subject: [PATCH 5/6] Squiz/ClassFileName: inverse the error message As things were, the `Squiz.Classes.ClassFileName` sniff would recommend for a class (OO structure) to be called the same as the file. However, file names can contain characters which are not allowed in PHP class names, so this advise could end up being impossible to action for names not complying with the PHP requirements for symbol names. This commit changes the error message to advise to change the file name instead of the class name, which should make the error actionable in all cases. Includes tests. While the tests would not _fail_ without this change, the tests can be used to verify the difference in the error messages on the command-line. Before this change, the errors would read like so: ``` FILE: src/Standards/Squiz/Tests/Classes/ClassFileName Spaces In FilenameUnitTest.inc ------------------------------------------------------------------------------------------------------------------------------------------------ FOUND 1 ERROR AFFECTING 1 LINE ------------------------------------------------------------------------------------------------------------------------------------------------ 7 | ERROR | Class name doesn't match filename; expected "class ClassFileName Spaces In FilenameUnitTest" (Squiz.Classes.ClassFileName.NoMatch) ------------------------------------------------------------------------------------------------------------------------------------------------ FILE: src/Standards/Squiz/Tests/Classes/ClassFileName-Dashes-In-FilenameUnitTest.inc ------------------------------------------------------------------------------------------------------------------------------------------------ FOUND 1 ERROR AFFECTING 1 LINE ------------------------------------------------------------------------------------------------------------------------------------------------ 7 | ERROR | Class name doesn't match filename; expected "class ClassFileName-Dashes-In-FilenameUnitTest" (Squiz.Classes.ClassFileName.NoMatch) ------------------------------------------------------------------------------------------------------------------------------------------------ ``` With this change, the errors will read: ``` FILE: src/Standards/Squiz/Tests/Classes/ClassFileName Spaces In FilenameUnitTest.inc ----------------------------------------------------------------------------------------------------------------------------------------------------- FOUND 1 ERROR AFFECTING 1 LINE ----------------------------------------------------------------------------------------------------------------------------------------------------- 7 | ERROR | Filename doesn't match class name; expected file name "ClassFileNameSpacesInFilenameUnitTest.inc" (Squiz.Classes.ClassFileName.NoMatch) ----------------------------------------------------------------------------------------------------------------------------------------------------- FILE: src/Standards/Squiz/Tests/Classes/ClassFileName-Dashes-In-FilenameUnitTest.inc ----------------------------------------------------------------------------------------------------------------------------------------------------- FOUND 1 ERROR AFFECTING 1 LINE ----------------------------------------------------------------------------------------------------------------------------------------------------- 7 | ERROR | Filename doesn't match class name; expected file name "ClassFileNameDashesInFilenameUnitTest.inc" (Squiz.Classes.ClassFileName.NoMatch) ----------------------------------------------------------------------------------------------------------------------------------------------------- ``` Side-note: while the `strrpos($fullPath, '.')` _could_ return `false`, this is not something we need to be concerned about in this sniff as PHPCS doesn't examine files without a file extension (at this time), so there should always be a `.` in the file name. --- .../Squiz/Sniffs/Classes/ClassFileNameSniff.php | 10 +++++----- .../ClassFileName Spaces In FilenameUnitTest.inc | 7 +++++++ .../ClassFileName-Dashes-In-FilenameUnitTest.inc | 7 +++++++ .../Squiz/Tests/Classes/ClassFileNameUnitTest.php | 10 ++++++++++ 4 files changed, 29 insertions(+), 5 deletions(-) create mode 100644 src/Standards/Squiz/Tests/Classes/ClassFileName Spaces In FilenameUnitTest.inc create mode 100644 src/Standards/Squiz/Tests/Classes/ClassFileName-Dashes-In-FilenameUnitTest.inc diff --git a/src/Standards/Squiz/Sniffs/Classes/ClassFileNameSniff.php b/src/Standards/Squiz/Sniffs/Classes/ClassFileNameSniff.php index 396093727c..8f3d77388f 100644 --- a/src/Standards/Squiz/Sniffs/Classes/ClassFileNameSniff.php +++ b/src/Standards/Squiz/Sniffs/Classes/ClassFileNameSniff.php @@ -48,8 +48,9 @@ public function process(File $phpcsFile, $stackPtr) return $phpcsFile->numTokens; } - $fullPath = basename($filename); - $fileName = substr($fullPath, 0, strrpos($fullPath, '.')); + $fullPath = basename($filename); + $fileName = substr($fullPath, 0, strrpos($fullPath, '.')); + $extension = substr($fullPath, (strrpos($fullPath, '.') + 1)); $tokens = $phpcsFile->getTokens(); $ooName = $phpcsFile->getDeclarationName($stackPtr); @@ -59,11 +60,10 @@ public function process(File $phpcsFile, $stackPtr) } if ($ooName !== $fileName) { - $error = '%s name doesn\'t match filename; expected "%s %s"'; + $error = 'Filename doesn\'t match %s name; expected file name "%s"'; $data = [ - ucfirst($tokens[$stackPtr]['content']), $tokens[$stackPtr]['content'], - $fileName, + $ooName.'.'.$extension, ]; $phpcsFile->addError($error, $stackPtr, 'NoMatch', $data); } diff --git a/src/Standards/Squiz/Tests/Classes/ClassFileName Spaces In FilenameUnitTest.inc b/src/Standards/Squiz/Tests/Classes/ClassFileName Spaces In FilenameUnitTest.inc new file mode 100644 index 0000000000..a89e3d03c3 --- /dev/null +++ b/src/Standards/Squiz/Tests/Classes/ClassFileName Spaces In FilenameUnitTest.inc @@ -0,0 +1,7 @@ + 1, ]; + case 'ClassFileName Spaces In FilenameUnitTest.inc': + return [ + 7 => 1, + ]; + + case 'ClassFileName-Dashes-In-FilenameUnitTest.inc': + return [ + 7 => 1, + ]; + default: return []; }//end switch From cf5d4c1b8a1aef637d88c9d4ccd5b348fba0e88c Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 7 Mar 2025 13:56:23 +0100 Subject: [PATCH 6/6] Squiz/ClassFileName: use more meaningful variable names --- .../Squiz/Sniffs/Classes/ClassFileNameSniff.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Standards/Squiz/Sniffs/Classes/ClassFileNameSniff.php b/src/Standards/Squiz/Sniffs/Classes/ClassFileNameSniff.php index 8f3d77388f..a5e53ac1ba 100644 --- a/src/Standards/Squiz/Sniffs/Classes/ClassFileNameSniff.php +++ b/src/Standards/Squiz/Sniffs/Classes/ClassFileNameSniff.php @@ -43,14 +43,14 @@ public function register() */ public function process(File $phpcsFile, $stackPtr) { - $filename = $phpcsFile->getFilename(); - if ($filename === 'STDIN') { + $fullPath = $phpcsFile->getFilename(); + if ($fullPath === 'STDIN') { return $phpcsFile->numTokens; } - $fullPath = basename($filename); - $fileName = substr($fullPath, 0, strrpos($fullPath, '.')); - $extension = substr($fullPath, (strrpos($fullPath, '.') + 1)); + $fileName = basename($fullPath); + $fileNoExt = substr($fileName, 0, strrpos($fileName, '.')); + $extension = substr($fileName, (strrpos($fileName, '.') + 1)); $tokens = $phpcsFile->getTokens(); $ooName = $phpcsFile->getDeclarationName($stackPtr); @@ -59,7 +59,7 @@ public function process(File $phpcsFile, $stackPtr) return; } - if ($ooName !== $fileName) { + if ($ooName !== $fileNoExt) { $error = 'Filename doesn\'t match %s name; expected file name "%s"'; $data = [ $tokens[$stackPtr]['content'],