From 0c65a669dde01dc301a2daacdb6d9fab0b5f2b2f Mon Sep 17 00:00:00 2001 From: jrfnl Date: Mon, 18 Sep 2023 06:00:45 +0200 Subject: [PATCH 01/19] Files/FileName: minor documentation improvements --- Yoast/Sniffs/Files/FileNameSniff.php | 12 ++++++------ Yoast/Tests/Files/FileNameUnitTest.php | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/Yoast/Sniffs/Files/FileNameSniff.php b/Yoast/Sniffs/Files/FileNameSniff.php index f3d145e4..1d5280c4 100644 --- a/Yoast/Sniffs/Files/FileNameSniff.php +++ b/Yoast/Sniffs/Files/FileNameSniff.php @@ -45,7 +45,7 @@ final class FileNameSniff implements Sniff { * - When several overlapping prefixes match, the longest matching prefix * will be removed. * - * @var string[] + * @var array */ public $oo_prefixes = []; @@ -65,7 +65,7 @@ final class FileNameSniff implements Sniff { * from the root of the repository - , the PHPCS `--basepath` config variable * needs to be set. If it is not, a warning will be issued. * - * @var string[] + * @var array */ public $excluded_files_strict_check = []; @@ -244,11 +244,11 @@ protected function is_file_excluded( File $phpcsFile, $path_to_file ) { * * Optionally flips the array to allow for using `isset` instead of `in_array`. * - * @param mixed $property The current property value. - * @param bool $flip Whether to flip the array values to keys. - * @param bool $to_lower Whether to lowercase the array values. + * @param array $property The current property value. + * @param bool $flip Whether to flip the array values to keys. + * @param bool $to_lower Whether to lowercase the array values. * - * @return (string|bool)[] + * @return array|array */ protected function clean_custom_array_property( $property, $flip = false, $to_lower = false ) { $property = \array_filter( \array_map( 'trim', $property ) ); diff --git a/Yoast/Tests/Files/FileNameUnitTest.php b/Yoast/Tests/Files/FileNameUnitTest.php index 472c8a07..66346ba1 100644 --- a/Yoast/Tests/Files/FileNameUnitTest.php +++ b/Yoast/Tests/Files/FileNameUnitTest.php @@ -17,7 +17,7 @@ final class FileNameUnitTest extends AbstractSniffUnitTest { /** * Error files with the expected nr of errors. * - * @var int[] + * @var array */ private $expected_results = [ @@ -98,7 +98,7 @@ public function setCliValues( $filename, $config ): void { * * @param string $testFileBase The base path that the unit tests files will have. * - * @return string[] + * @return array */ protected function getTestFiles( $testFileBase ): array { $sep = \DIRECTORY_SEPARATOR; From 2a6c62d71296f60b64195d4a596e06694dfb8e08 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 21 Oct 2023 16:06:29 +0200 Subject: [PATCH 02/19] Files/FileName: efficiency fix Always return a stack pointer at the end of the file, even when the file passed comes from `STDIN`, as we ignore `STDIN` files for this sniff, so there is no need to continue listening for tokens in the file. Includes ignoring this line for code coverage checks as the line is untestable in a unit test situation. --- Yoast/Sniffs/Files/FileNameSniff.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Yoast/Sniffs/Files/FileNameSniff.php b/Yoast/Sniffs/Files/FileNameSniff.php index 1d5280c4..6b4a3703 100644 --- a/Yoast/Sniffs/Files/FileNameSniff.php +++ b/Yoast/Sniffs/Files/FileNameSniff.php @@ -92,7 +92,7 @@ public function process( File $phpcsFile, $stackPtr ) { $file = \preg_replace( '`^([\'"])(.*)\1$`Ds', '$2', $phpcsFile->getFileName() ); if ( $file === 'STDIN' ) { - return; + return ( $phpcsFile->numTokens + 1 ); // @codeCoverageIgnore } $path_info = \pathinfo( $file ); From 0fb28ac0b6cdaf470bb9c67ca992ef5ba296e398 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 18 Oct 2023 02:49:53 +0200 Subject: [PATCH 03/19] Files/FileName: use PHPCSUtils in more places --- Yoast/Sniffs/Files/FileNameSniff.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Yoast/Sniffs/Files/FileNameSniff.php b/Yoast/Sniffs/Files/FileNameSniff.php index 6b4a3703..e5598e4d 100644 --- a/Yoast/Sniffs/Files/FileNameSniff.php +++ b/Yoast/Sniffs/Files/FileNameSniff.php @@ -6,6 +6,7 @@ use PHP_CodeSniffer\Sniffs\Sniff; use PHP_CodeSniffer\Util\Common; use PHPCSUtils\Utils\ObjectDeclarations; +use PHPCSUtils\Utils\TextStrings; /** * Ensures files comply with the Yoast file name rules. @@ -89,7 +90,7 @@ public function register() { */ public function process( File $phpcsFile, $stackPtr ) { // Stripping potential quotes to ensure `stdin_path` passed by IDEs does not include quotes. - $file = \preg_replace( '`^([\'"])(.*)\1$`Ds', '$2', $phpcsFile->getFileName() ); + $file = TextStrings::stripQuotes( $phpcsFile->getFileName() ); if ( $file === 'STDIN' ) { return ( $phpcsFile->numTokens + 1 ); // @codeCoverageIgnore From b25f1c63382c2adc962aa1db2d7c853d327c0b50 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Tue, 31 Oct 2023 03:42:23 +0100 Subject: [PATCH 04/19] Files/FileName: make non-interface methods `private` As the sniff class is now `final` (since PR 319), there is no need for any `protected` methods, so let's make these `private`. --- Yoast/Sniffs/Files/FileNameSniff.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Yoast/Sniffs/Files/FileNameSniff.php b/Yoast/Sniffs/Files/FileNameSniff.php index e5598e4d..16f29479 100644 --- a/Yoast/Sniffs/Files/FileNameSniff.php +++ b/Yoast/Sniffs/Files/FileNameSniff.php @@ -205,7 +205,7 @@ public function process( File $phpcsFile, $stackPtr ) { * * @return bool */ - protected function is_file_excluded( File $phpcsFile, $path_to_file ) { + private function is_file_excluded( File $phpcsFile, $path_to_file ) { $exclude = $this->clean_custom_array_property( $this->excluded_files_strict_check, true, true ); if ( ! empty( $exclude ) ) { @@ -251,7 +251,7 @@ protected function is_file_excluded( File $phpcsFile, $path_to_file ) { * * @return array|array */ - protected function clean_custom_array_property( $property, $flip = false, $to_lower = false ) { + private function clean_custom_array_property( $property, $flip = false, $to_lower = false ) { $property = \array_filter( \array_map( 'trim', $property ) ); if ( $to_lower === true ) { From ccec2bfc7cbfc9ef5320082cf85a8c34c4bc290a Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 21 Oct 2023 16:02:50 +0200 Subject: [PATCH 05/19] Files/FileName: minor code tweaks Lower nesting levels and remove `elseif` when not needed. :point_right: This commit will be easiest to review while ignoring whitespace changes. --- Yoast/Sniffs/Files/FileNameSniff.php | 41 +++++++++++++--------------- 1 file changed, 19 insertions(+), 22 deletions(-) diff --git a/Yoast/Sniffs/Files/FileNameSniff.php b/Yoast/Sniffs/Files/FileNameSniff.php index 16f29479..9cb90152 100644 --- a/Yoast/Sniffs/Files/FileNameSniff.php +++ b/Yoast/Sniffs/Files/FileNameSniff.php @@ -208,32 +208,29 @@ public function process( File $phpcsFile, $stackPtr ) { private function is_file_excluded( File $phpcsFile, $path_to_file ) { $exclude = $this->clean_custom_array_property( $this->excluded_files_strict_check, true, true ); - if ( ! empty( $exclude ) ) { - $exclude = \array_map( [ $this, 'normalize_directory_separators' ], $exclude ); - $path_to_file = $this->normalize_directory_separators( $path_to_file ); - - if ( ! isset( $phpcsFile->config->basepath ) ) { - $phpcsFile->addWarning( - 'For the exclude property to work with relative file path files, the --basepath needs to be set.', - 0, - 'MissingBasePath' - ); - } - else { - $base_path = $this->normalize_directory_separators( $phpcsFile->config->basepath ); - $path_to_file = Common::stripBasepath( $path_to_file, $base_path ); - } + if ( empty( $exclude ) ) { + return false; + } - // Lowercase the filename to not interfere with the lowercase/dashes rule. - $path_to_file = \strtolower( \ltrim( $path_to_file, '/' ) ); + $exclude = \array_map( [ $this, 'normalize_directory_separators' ], $exclude ); + $path_to_file = $this->normalize_directory_separators( $path_to_file ); - if ( isset( $exclude[ $path_to_file ] ) ) { - // Filename is on the exclude list. - return true; - } + if ( ! isset( $phpcsFile->config->basepath ) ) { + $phpcsFile->addWarning( + 'For the exclude property to work with relative file path files, the --basepath needs to be set.', + 0, + 'MissingBasePath' + ); } + else { + $base_path = $this->normalize_directory_separators( $phpcsFile->config->basepath ); + $path_to_file = Common::stripBasepath( $path_to_file, $base_path ); + } + + // Lowercase the filename to not interfere with the lowercase/dashes rule. + $path_to_file = \strtolower( \ltrim( $path_to_file, '/' ) ); - return false; + return isset( $exclude[ $path_to_file ] ); } /** From 4b1157a6f3e6712a292b56a36c7aea962ce169dd Mon Sep 17 00:00:00 2001 From: jrfnl Date: Mon, 23 Oct 2023 04:04:32 +0200 Subject: [PATCH 06/19] Files/FileName: rename a local variable `$name` is very generic, so let's rename this variable to `$oo_name` to make it more descriptive. --- Yoast/Sniffs/Files/FileNameSniff.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Yoast/Sniffs/Files/FileNameSniff.php b/Yoast/Sniffs/Files/FileNameSniff.php index 9cb90152..635ad88b 100644 --- a/Yoast/Sniffs/Files/FileNameSniff.php +++ b/Yoast/Sniffs/Files/FileNameSniff.php @@ -122,23 +122,23 @@ public function process( File $phpcsFile, $stackPtr ) { $oo_structure = $phpcsFile->findNext( self::NAMED_OO_TOKENS, $stackPtr ); if ( $oo_structure !== false ) { - $tokens = $phpcsFile->getTokens(); - $name = ObjectDeclarations::getName( $phpcsFile, $oo_structure ); + $tokens = $phpcsFile->getTokens(); + $oo_name = ObjectDeclarations::getName( $phpcsFile, $oo_structure ); $prefixes = $this->clean_custom_array_property( $this->oo_prefixes ); if ( ! empty( $prefixes ) ) { // Use reverse natural sorting to get the longest of overlapping prefixes first. \rsort( $prefixes, ( \SORT_NATURAL | \SORT_FLAG_CASE ) ); foreach ( $prefixes as $prefix ) { - if ( $name !== $prefix && \stripos( $name, $prefix ) === 0 ) { - $name = \substr( $name, \strlen( $prefix ) ); - $name = \ltrim( $name, '_-' ); + if ( $oo_name !== $prefix && \stripos( $oo_name, $prefix ) === 0 ) { + $oo_name = \substr( $oo_name, \strlen( $prefix ) ); + $oo_name = \ltrim( $oo_name, '_-' ); break; } } } - $expected = \strtolower( \str_replace( '_', '-', $name ) ); + $expected = \strtolower( \str_replace( '_', '-', $oo_name ) ); switch ( $tokens[ $oo_structure ]['code'] ) { case \T_CLASS: From b6bf13bca9790c6ba94b3a949f0fd0ab7226134e Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 22 Oct 2023 10:25:15 +0200 Subject: [PATCH 07/19] Files/FileName: more defensive coding for parse errors/live coding Prevents a PHP notice in case of a IDE check during live coding. Includes test. :point_right: this change will be easiest to review while ignoring whitespace changes. --- Yoast/Sniffs/Files/FileNameSniff.php | 66 ++++++++++--------- Yoast/Tests/Files/FileNameUnitTest.php | 3 + .../Files/FileNameUnitTests/live-coding.inc | 3 + 3 files changed, 40 insertions(+), 32 deletions(-) create mode 100644 Yoast/Tests/Files/FileNameUnitTests/live-coding.inc diff --git a/Yoast/Sniffs/Files/FileNameSniff.php b/Yoast/Sniffs/Files/FileNameSniff.php index 635ad88b..e13c17d9 100644 --- a/Yoast/Sniffs/Files/FileNameSniff.php +++ b/Yoast/Sniffs/Files/FileNameSniff.php @@ -125,46 +125,48 @@ public function process( File $phpcsFile, $stackPtr ) { $tokens = $phpcsFile->getTokens(); $oo_name = ObjectDeclarations::getName( $phpcsFile, $oo_structure ); - $prefixes = $this->clean_custom_array_property( $this->oo_prefixes ); - if ( ! empty( $prefixes ) ) { - // Use reverse natural sorting to get the longest of overlapping prefixes first. - \rsort( $prefixes, ( \SORT_NATURAL | \SORT_FLAG_CASE ) ); - foreach ( $prefixes as $prefix ) { - if ( $oo_name !== $prefix && \stripos( $oo_name, $prefix ) === 0 ) { - $oo_name = \substr( $oo_name, \strlen( $prefix ) ); - $oo_name = \ltrim( $oo_name, '_-' ); - break; + if ( ! empty( $oo_name ) ) { + $prefixes = $this->clean_custom_array_property( $this->oo_prefixes ); + if ( ! empty( $prefixes ) ) { + // Use reverse natural sorting to get the longest of overlapping prefixes first. + \rsort( $prefixes, ( \SORT_NATURAL | \SORT_FLAG_CASE ) ); + foreach ( $prefixes as $prefix ) { + if ( $oo_name !== $prefix && \stripos( $oo_name, $prefix ) === 0 ) { + $oo_name = \substr( $oo_name, \strlen( $prefix ) ); + $oo_name = \ltrim( $oo_name, '_-' ); + break; + } } } - } - $expected = \strtolower( \str_replace( '_', '-', $oo_name ) ); + $expected = \strtolower( \str_replace( '_', '-', $oo_name ) ); - switch ( $tokens[ $oo_structure ]['code'] ) { - case \T_CLASS: - $error = 'Class file names should be based on the class name without the plugin prefix. Expected %s, but found %s.'; - $error_code = 'InvalidClassFileName'; - break; + switch ( $tokens[ $oo_structure ]['code'] ) { + case \T_CLASS: + $error = 'Class file names should be based on the class name without the plugin prefix. Expected %s, but found %s.'; + $error_code = 'InvalidClassFileName'; + break; - case \T_INTERFACE: - $error = 'Interface file names should be based on the interface name without the plugin prefix and should have "-interface" as a suffix. Expected "%s", but found "%s".'; - $error_code = 'InvalidInterfaceFileName'; + case \T_INTERFACE: + $error = 'Interface file names should be based on the interface name without the plugin prefix and should have "-interface" as a suffix. Expected "%s", but found "%s".'; + $error_code = 'InvalidInterfaceFileName'; - // Don't duplicate "interface" in the filename. - if ( \substr( $expected, -10 ) !== '-interface' ) { - $expected .= '-interface'; - } - break; + // Don't duplicate "interface" in the filename. + if ( \substr( $expected, -10 ) !== '-interface' ) { + $expected .= '-interface'; + } + break; - case \T_TRAIT: - $error = 'Trait file names should be based on the trait name without the plugin prefix and should have "-trait" as a suffix. Expected "%s", but found "%s".'; - $error_code = 'InvalidTraitFileName'; + case \T_TRAIT: + $error = 'Trait file names should be based on the trait name without the plugin prefix and should have "-trait" as a suffix. Expected "%s", but found "%s".'; + $error_code = 'InvalidTraitFileName'; - // Don't duplicate "trait" in the filename. - if ( \substr( $expected, -6 ) !== '-trait' ) { - $expected .= '-trait'; - } - break; + // Don't duplicate "trait" in the filename. + if ( \substr( $expected, -6 ) !== '-trait' ) { + $expected .= '-trait'; + } + break; + } } } else { diff --git a/Yoast/Tests/Files/FileNameUnitTest.php b/Yoast/Tests/Files/FileNameUnitTest.php index 66346ba1..40b8c2d7 100644 --- a/Yoast/Tests/Files/FileNameUnitTest.php +++ b/Yoast/Tests/Files/FileNameUnitTest.php @@ -25,6 +25,9 @@ final class FileNameUnitTest extends AbstractSniffUnitTest { * In /FileNameUnitTests. */ + // Live coding/parse error test. + 'live-coding.inc' => 0, + // Exclusions. 'excluded-file.inc' => 0, 'ExcludedFile.inc' => 1, // Lowercase expected. diff --git a/Yoast/Tests/Files/FileNameUnitTests/live-coding.inc b/Yoast/Tests/Files/FileNameUnitTests/live-coding.inc new file mode 100644 index 00000000..0604d825 --- /dev/null +++ b/Yoast/Tests/Files/FileNameUnitTests/live-coding.inc @@ -0,0 +1,3 @@ + Date: Mon, 23 Oct 2023 02:19:36 +0200 Subject: [PATCH 08/19] Files/FileName: improve error message consistency Always have quotes around the expected + found file names. As things were, this was not the case for the generic "lowercase/hyphenated" message + the class file name message. Fixed now. --- Yoast/Sniffs/Files/FileNameSniff.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Yoast/Sniffs/Files/FileNameSniff.php b/Yoast/Sniffs/Files/FileNameSniff.php index e13c17d9..af77828b 100644 --- a/Yoast/Sniffs/Files/FileNameSniff.php +++ b/Yoast/Sniffs/Files/FileNameSniff.php @@ -114,7 +114,7 @@ public function process( File $phpcsFile, $stackPtr ) { $extension = $path_info['extension']; } - $error = 'Filenames should be all lowercase with hyphens as word separators. Expected %s, but found %s.'; + $error = 'Filenames should be all lowercase with hyphens as word separators.'; $error_code = 'NotHyphenatedLowercase'; $expected = \strtolower( \str_replace( '_', '-', $file_name ) ); @@ -143,12 +143,12 @@ public function process( File $phpcsFile, $stackPtr ) { switch ( $tokens[ $oo_structure ]['code'] ) { case \T_CLASS: - $error = 'Class file names should be based on the class name without the plugin prefix. Expected %s, but found %s.'; + $error = 'Class file names should be based on the class name without the plugin prefix.'; $error_code = 'InvalidClassFileName'; break; case \T_INTERFACE: - $error = 'Interface file names should be based on the interface name without the plugin prefix and should have "-interface" as a suffix. Expected "%s", but found "%s".'; + $error = 'Interface file names should be based on the interface name without the plugin prefix and should have "-interface" as a suffix.'; $error_code = 'InvalidInterfaceFileName'; // Don't duplicate "interface" in the filename. @@ -158,7 +158,7 @@ public function process( File $phpcsFile, $stackPtr ) { break; case \T_TRAIT: - $error = 'Trait file names should be based on the trait name without the plugin prefix and should have "-trait" as a suffix. Expected "%s", but found "%s".'; + $error = 'Trait file names should be based on the trait name without the plugin prefix and should have "-trait" as a suffix.'; $error_code = 'InvalidTraitFileName'; // Don't duplicate "trait" in the filename. @@ -172,7 +172,7 @@ public function process( File $phpcsFile, $stackPtr ) { else { $has_function = $phpcsFile->findNext( \T_FUNCTION, $stackPtr ); if ( $has_function !== false && $file_name !== 'functions' ) { - $error = 'Files containing function declarations should have "-functions" as a suffix. Expected "%s", but found "%s".'; + $error = 'Files containing function declarations should have "-functions" as a suffix.'; $error_code = 'InvalidFunctionsFileName'; if ( \substr( $expected, -10 ) !== '-functions' ) { @@ -185,7 +185,7 @@ public function process( File $phpcsFile, $stackPtr ) { // Throw the error. if ( $expected !== '' && $file_name !== $expected ) { $phpcsFile->addError( - $error, + $error . ' Expected "%s", but found "%s".', 0, $error_code, [ From 61b06f1a375b07411b468de3a50c9e8925325450 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Tue, 24 Oct 2023 04:57:07 +0200 Subject: [PATCH 09/19] Files/FileName: examine views using only short echo open tags Files using _only_ short open tags were never examined. Fixed now. Includes additional tests. --- Yoast/Sniffs/Files/FileNameSniff.php | 4 +++- Yoast/Tests/Files/FileNameUnitTest.php | 3 +++ Yoast/Tests/Files/FileNameUnitTests/ShortOpen.inc | 1 + Yoast/Tests/Files/FileNameUnitTests/short-open.inc | 1 + Yoast/Tests/Files/FileNameUnitTests/short_Open.inc | 1 + 5 files changed, 9 insertions(+), 1 deletion(-) create mode 100644 Yoast/Tests/Files/FileNameUnitTests/ShortOpen.inc create mode 100644 Yoast/Tests/Files/FileNameUnitTests/short-open.inc create mode 100644 Yoast/Tests/Files/FileNameUnitTests/short_Open.inc diff --git a/Yoast/Sniffs/Files/FileNameSniff.php b/Yoast/Sniffs/Files/FileNameSniff.php index af77828b..1befd27d 100644 --- a/Yoast/Sniffs/Files/FileNameSniff.php +++ b/Yoast/Sniffs/Files/FileNameSniff.php @@ -5,6 +5,7 @@ use PHP_CodeSniffer\Files\File; use PHP_CodeSniffer\Sniffs\Sniff; use PHP_CodeSniffer\Util\Common; +use PHPCSUtils\Tokens\Collections; use PHPCSUtils\Utils\ObjectDeclarations; use PHPCSUtils\Utils\TextStrings; @@ -22,6 +23,7 @@ * have a "-functions" suffix. * * @since 0.5 + * @since 3.0.0 The sniff will now also be enforced for files only using the PHP short open tag. */ final class FileNameSniff implements Sniff { @@ -76,7 +78,7 @@ final class FileNameSniff implements Sniff { * @return array */ public function register() { - return [ \T_OPEN_TAG ]; + return Collections::phpOpenTags(); } /** diff --git a/Yoast/Tests/Files/FileNameUnitTest.php b/Yoast/Tests/Files/FileNameUnitTest.php index 40b8c2d7..e6e0863b 100644 --- a/Yoast/Tests/Files/FileNameUnitTest.php +++ b/Yoast/Tests/Files/FileNameUnitTest.php @@ -37,6 +37,9 @@ final class FileNameUnitTest extends AbstractSniffUnitTest { 'some_file.inc' => 1, // Dashes, not underscores. 'SomeFile.inc' => 1, // Lowercase expected. 'some-File.inc' => 1, // Lowercase expected. + 'short-open.inc' => 0, + 'ShortOpen.inc' => 1, // Lowercase expected. + 'short_Open.inc' => 1, // Lowercase expected, dashes, not underscores. // Class file names. 'my-class.inc' => 0, diff --git a/Yoast/Tests/Files/FileNameUnitTests/ShortOpen.inc b/Yoast/Tests/Files/FileNameUnitTests/ShortOpen.inc new file mode 100644 index 00000000..72fd545b --- /dev/null +++ b/Yoast/Tests/Files/FileNameUnitTests/ShortOpen.inc @@ -0,0 +1 @@ + diff --git a/Yoast/Tests/Files/FileNameUnitTests/short-open.inc b/Yoast/Tests/Files/FileNameUnitTests/short-open.inc new file mode 100644 index 00000000..72fd545b --- /dev/null +++ b/Yoast/Tests/Files/FileNameUnitTests/short-open.inc @@ -0,0 +1 @@ + diff --git a/Yoast/Tests/Files/FileNameUnitTests/short_Open.inc b/Yoast/Tests/Files/FileNameUnitTests/short_Open.inc new file mode 100644 index 00000000..72fd545b --- /dev/null +++ b/Yoast/Tests/Files/FileNameUnitTests/short_Open.inc @@ -0,0 +1 @@ + From 5d0f740f0d90eccab7530684732635469ae530e6 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Tue, 24 Oct 2023 05:29:05 +0200 Subject: [PATCH 10/19] Files/FileName: allow for the inline PHPCS ignore annotations These type of selective ignore annotations were introduced in PHPCS 3.2.0. This commit now allows for disabling this sniff through an annotation, though error code based ignores are still not supported. Includes unit tests. --- Yoast/Sniffs/Files/FileNameSniff.php | 26 ++++++++++++++++++- Yoast/Tests/Files/FileNameUnitTest.php | 11 ++++++++ .../ignore-annotations/Errorcode_Disable.inc | 8 ++++++ .../ignore-annotations/blanket-disable.inc | 6 +++++ .../ignore-annotations/category-disable.inc | 6 +++++ .../disable-matching-enable.inc | 8 ++++++ .../disable-non-matching-enable.inc | 12 +++++++++ .../non-relevant-disable.inc | 6 +++++ .../partial-file-disable.inc | 10 +++++++ .../ignore-annotations/rule-disable.inc | 6 +++++ .../ignore-annotations/yoast-disable.inc | 6 +++++ 11 files changed, 104 insertions(+), 1 deletion(-) create mode 100644 Yoast/Tests/Files/FileNameUnitTests/ignore-annotations/Errorcode_Disable.inc create mode 100644 Yoast/Tests/Files/FileNameUnitTests/ignore-annotations/blanket-disable.inc create mode 100644 Yoast/Tests/Files/FileNameUnitTests/ignore-annotations/category-disable.inc create mode 100644 Yoast/Tests/Files/FileNameUnitTests/ignore-annotations/disable-matching-enable.inc create mode 100644 Yoast/Tests/Files/FileNameUnitTests/ignore-annotations/disable-non-matching-enable.inc create mode 100644 Yoast/Tests/Files/FileNameUnitTests/ignore-annotations/non-relevant-disable.inc create mode 100644 Yoast/Tests/Files/FileNameUnitTests/ignore-annotations/partial-file-disable.inc create mode 100644 Yoast/Tests/Files/FileNameUnitTests/ignore-annotations/rule-disable.inc create mode 100644 Yoast/Tests/Files/FileNameUnitTests/ignore-annotations/yoast-disable.inc diff --git a/Yoast/Sniffs/Files/FileNameSniff.php b/Yoast/Sniffs/Files/FileNameSniff.php index 1befd27d..dfc13a97 100644 --- a/Yoast/Sniffs/Files/FileNameSniff.php +++ b/Yoast/Sniffs/Files/FileNameSniff.php @@ -98,6 +98,31 @@ public function process( File $phpcsFile, $stackPtr ) { return ( $phpcsFile->numTokens + 1 ); // @codeCoverageIgnore } + // Respect phpcs:disable comments as long as they are not accompanied by an enable. + $tokens = $phpcsFile->getTokens(); + $i = -1; + while ( $i = $phpcsFile->findNext( \T_PHPCS_DISABLE, ( $i + 1 ) ) ) { + if ( empty( $tokens[ $i ]['sniffCodes'] ) + || isset( $tokens[ $i ]['sniffCodes']['Yoast'] ) + || isset( $tokens[ $i ]['sniffCodes']['Yoast.Files'] ) + || isset( $tokens[ $i ]['sniffCodes']['Yoast.Files.FileName'] ) + ) { + do { + $i = $phpcsFile->findNext( \T_PHPCS_ENABLE, ( $i + 1 ) ); + } while ( $i !== false + && ! empty( $tokens[ $i ]['sniffCodes'] ) + && ! isset( $tokens[ $i ]['sniffCodes']['Yoast'] ) + && ! isset( $tokens[ $i ]['sniffCodes']['Yoast.Files'] ) + && ! isset( $tokens[ $i ]['sniffCodes']['Yoast.Files.FileName'] ) + ); + + if ( $i === false ) { + // The entire (rest of the) file is disabled. + return ( $phpcsFile->numTokens + 1 ); + } + } + } + $path_info = \pathinfo( $file ); // Basename = filename + extension. @@ -124,7 +149,6 @@ public function process( File $phpcsFile, $stackPtr ) { $oo_structure = $phpcsFile->findNext( self::NAMED_OO_TOKENS, $stackPtr ); if ( $oo_structure !== false ) { - $tokens = $phpcsFile->getTokens(); $oo_name = ObjectDeclarations::getName( $phpcsFile, $oo_structure ); if ( ! empty( $oo_name ) ) { diff --git a/Yoast/Tests/Files/FileNameUnitTest.php b/Yoast/Tests/Files/FileNameUnitTest.php index e6e0863b..7ebcaf68 100644 --- a/Yoast/Tests/Files/FileNameUnitTest.php +++ b/Yoast/Tests/Files/FileNameUnitTest.php @@ -75,6 +75,17 @@ final class FileNameUnitTest extends AbstractSniffUnitTest { 'some-file.inc' => 1, // Missing '-functions' suffix. 'excluded-functions-file.inc' => 0, + // Ignore annotation handling. + 'blanket-disable.inc' => 0, + 'yoast-disable.inc' => 0, + 'category-disable.inc' => 0, + 'rule-disable.inc' => 0, + 'disable-matching-enable.inc' => 1, + 'disable-non-matching-enable.inc' => 0, + 'non-relevant-disable.inc' => 1, + 'partial-file-disable.inc' => 1, + 'Errorcode_Disable.inc' => 1, // The sniff can only be disabled completely, not by error code. + /* * In /. */ diff --git a/Yoast/Tests/Files/FileNameUnitTests/ignore-annotations/Errorcode_Disable.inc b/Yoast/Tests/Files/FileNameUnitTests/ignore-annotations/Errorcode_Disable.inc new file mode 100644 index 00000000..9245db89 --- /dev/null +++ b/Yoast/Tests/Files/FileNameUnitTests/ignore-annotations/Errorcode_Disable.inc @@ -0,0 +1,8 @@ + Date: Tue, 24 Oct 2023 05:46:54 +0200 Subject: [PATCH 11/19] Files/FileName: improve handling of files using non-underscore, non-dash word separators This mirrors the same change as made upstream in the WPCS native FileName sniff in PR WordPress/WordPress-Coding-Standards 2285. Files using non-underscore punctuation characters as word separators were previously not being flagged by the sniff. Fixed now. Includes extra tests. --- Yoast/Sniffs/Files/FileNameSniff.php | 2 +- Yoast/Tests/Files/FileNameUnitTest.php | 2 ++ Yoast/Tests/Files/FileNameUnitTests/dot.not.underscore.inc | 3 +++ Yoast/Tests/Files/FileNameUnitTests/with#other+punctuation.inc | 3 +++ 4 files changed, 9 insertions(+), 1 deletion(-) create mode 100644 Yoast/Tests/Files/FileNameUnitTests/dot.not.underscore.inc create mode 100644 Yoast/Tests/Files/FileNameUnitTests/with#other+punctuation.inc diff --git a/Yoast/Sniffs/Files/FileNameSniff.php b/Yoast/Sniffs/Files/FileNameSniff.php index dfc13a97..f183aed0 100644 --- a/Yoast/Sniffs/Files/FileNameSniff.php +++ b/Yoast/Sniffs/Files/FileNameSniff.php @@ -143,7 +143,7 @@ public function process( File $phpcsFile, $stackPtr ) { $error = 'Filenames should be all lowercase with hyphens as word separators.'; $error_code = 'NotHyphenatedLowercase'; - $expected = \strtolower( \str_replace( '_', '-', $file_name ) ); + $expected = \strtolower( \preg_replace( '`[[:punct:]]`', '-', $file_name ) ); if ( $this->is_file_excluded( $phpcsFile, $file ) === false ) { $oo_structure = $phpcsFile->findNext( self::NAMED_OO_TOKENS, $stackPtr ); diff --git a/Yoast/Tests/Files/FileNameUnitTest.php b/Yoast/Tests/Files/FileNameUnitTest.php index 7ebcaf68..4f86dc72 100644 --- a/Yoast/Tests/Files/FileNameUnitTest.php +++ b/Yoast/Tests/Files/FileNameUnitTest.php @@ -40,6 +40,8 @@ final class FileNameUnitTest extends AbstractSniffUnitTest { 'short-open.inc' => 0, 'ShortOpen.inc' => 1, // Lowercase expected. 'short_Open.inc' => 1, // Lowercase expected, dashes, not underscores. + 'dot.not.underscore.inc' => 1, // Dashes, not other punctuation. + 'with#other+punctuation.inc' => 1, // Dashes, not other punctuation. // Class file names. 'my-class.inc' => 0, diff --git a/Yoast/Tests/Files/FileNameUnitTests/dot.not.underscore.inc b/Yoast/Tests/Files/FileNameUnitTests/dot.not.underscore.inc new file mode 100644 index 00000000..fe448aeb --- /dev/null +++ b/Yoast/Tests/Files/FileNameUnitTests/dot.not.underscore.inc @@ -0,0 +1,3 @@ + Date: Mon, 23 Oct 2023 02:30:43 +0200 Subject: [PATCH 12/19] Files/FileName: refactor some near duplicate code The switch cases for `interface`s and `trait`s are basically duplicates of each other with the term being variable. This refactors the code to remove the duplicate code and use the term as a variable. Includes updated unit tests to ensure the message creation is stable. --- Yoast/Sniffs/Files/FileNameSniff.php | 33 +++++++++---------- .../interfaces/no-duplicate-interface.inc | 2 +- .../traits/different-trait.inc | 2 +- .../traits/outline-mything.inc | 2 +- 4 files changed, 19 insertions(+), 20 deletions(-) diff --git a/Yoast/Sniffs/Files/FileNameSniff.php b/Yoast/Sniffs/Files/FileNameSniff.php index f183aed0..afaadda9 100644 --- a/Yoast/Sniffs/Files/FileNameSniff.php +++ b/Yoast/Sniffs/Files/FileNameSniff.php @@ -173,23 +173,22 @@ public function process( File $phpcsFile, $stackPtr ) { $error_code = 'InvalidClassFileName'; break; - case \T_INTERFACE: - $error = 'Interface file names should be based on the interface name without the plugin prefix and should have "-interface" as a suffix.'; - $error_code = 'InvalidInterfaceFileName'; - - // Don't duplicate "interface" in the filename. - if ( \substr( $expected, -10 ) !== '-interface' ) { - $expected .= '-interface'; - } - break; - - case \T_TRAIT: - $error = 'Trait file names should be based on the trait name without the plugin prefix and should have "-trait" as a suffix.'; - $error_code = 'InvalidTraitFileName'; - - // Don't duplicate "trait" in the filename. - if ( \substr( $expected, -6 ) !== '-trait' ) { - $expected .= '-trait'; + // Interfaces, traits. + default: + $oo_type = \strtolower( $tokens[ $oo_structure ]['content'] ); + $oo_type_ucfirst = \ucfirst( $oo_type ); + + $error = \sprintf( + '%1$s file names should be based on the %2$s name without the plugin prefix and should have "-%2$s" as a suffix.', + $oo_type_ucfirst, + $oo_type + ); + $error_code = \sprintf( 'Invalid%sFileName', $oo_type_ucfirst ); + + // Don't duplicate "interface/trait" in the filename. + $expected_suffix = '-' . $oo_type; + if ( \substr( $expected, -\strlen( $expected_suffix ) ) !== $expected_suffix ) { + $expected .= $expected_suffix; } break; } diff --git a/Yoast/Tests/Files/FileNameUnitTests/interfaces/no-duplicate-interface.inc b/Yoast/Tests/Files/FileNameUnitTests/interfaces/no-duplicate-interface.inc index 6909ef98..eb87bf9f 100644 --- a/Yoast/Tests/Files/FileNameUnitTests/interfaces/no-duplicate-interface.inc +++ b/Yoast/Tests/Files/FileNameUnitTests/interfaces/no-duplicate-interface.inc @@ -3,6 +3,6 @@ phpcs:set Yoast.Files.FileName oo_prefixes[] wpseo,yoast Date: Mon, 23 Oct 2023 02:04:46 +0200 Subject: [PATCH 13/19] Files/FileName: add handling of files containing PHP 8.1+ enums Enums should be treated the same as interfaces/traits, i.e.: * File name should reflect the name of the `enum`. * Prefixes should be stripped from the name. * The file name should end with `-enum`. The sniff has been updated to handle enums in this way. Includes additional unit tests. Includes updated documentation. --- Yoast/Docs/Files/FileNameStandard.xml | 2 +- Yoast/Sniffs/Files/FileNameSniff.php | 11 ++++++----- Yoast/Tests/Files/FileNameUnitTest.php | 8 ++++++++ .../Files/FileNameUnitTests/enums/different-enum.inc | 8 ++++++++ .../Files/FileNameUnitTests/enums/excluded-enum.inc | 8 ++++++++ .../FileNameUnitTests/enums/no-duplicate-enum.inc | 8 ++++++++ .../Files/FileNameUnitTests/enums/something-enum.inc | 8 ++++++++ .../Tests/Files/FileNameUnitTests/enums/something.inc | 8 ++++++++ .../Files/FileNameUnitTests/enums/yoast-something.inc | 8 ++++++++ 9 files changed, 63 insertions(+), 6 deletions(-) create mode 100644 Yoast/Tests/Files/FileNameUnitTests/enums/different-enum.inc create mode 100644 Yoast/Tests/Files/FileNameUnitTests/enums/excluded-enum.inc create mode 100644 Yoast/Tests/Files/FileNameUnitTests/enums/no-duplicate-enum.inc create mode 100644 Yoast/Tests/Files/FileNameUnitTests/enums/something-enum.inc create mode 100644 Yoast/Tests/Files/FileNameUnitTests/enums/something.inc create mode 100644 Yoast/Tests/Files/FileNameUnitTests/enums/yoast-something.inc diff --git a/Yoast/Docs/Files/FileNameStandard.xml b/Yoast/Docs/Files/FileNameStandard.xml index 21c25aa2..1d185498 100644 --- a/Yoast/Docs/Files/FileNameStandard.xml +++ b/Yoast/Docs/Files/FileNameStandard.xml @@ -44,7 +44,7 @@ class WPSEO_Utils {} diff --git a/Yoast/Sniffs/Files/FileNameSniff.php b/Yoast/Sniffs/Files/FileNameSniff.php index afaadda9..4e1f7ec2 100644 --- a/Yoast/Sniffs/Files/FileNameSniff.php +++ b/Yoast/Sniffs/Files/FileNameSniff.php @@ -16,9 +16,9 @@ * - (WP) Filenames should be lowercase and words should be separated by dashes (not underscores). * - All class files should only contain one class (enforced by another sniff) and the file name * should reflect the class name without the plugin specific prefix. - * - All interface and trait files should only contain one interface/trait (enforced by another sniff) - * and the file name should reflect the interface/trait name without the plugin specific prefix - * and with an "-interface" or "-trait" suffix. + * - All interface, trait and enum files should only contain one interface/trait/enum (enforced by another sniff) + * and the file name should reflect the interface/trait/enum name without the plugin specific prefix + * and with an "-interface", "-trait" or "-enum" suffix. * - Files which don't contain an object structure, but do contain function declarations should * have a "-functions" suffix. * @@ -34,6 +34,7 @@ final class FileNameSniff implements Sniff { */ private const NAMED_OO_TOKENS = [ \T_CLASS, + \T_ENUM, \T_INTERFACE, \T_TRAIT, ]; @@ -173,7 +174,7 @@ public function process( File $phpcsFile, $stackPtr ) { $error_code = 'InvalidClassFileName'; break; - // Interfaces, traits. + // Interfaces, traits, enums. default: $oo_type = \strtolower( $tokens[ $oo_structure ]['content'] ); $oo_type_ucfirst = \ucfirst( $oo_type ); @@ -185,7 +186,7 @@ public function process( File $phpcsFile, $stackPtr ) { ); $error_code = \sprintf( 'Invalid%sFileName', $oo_type_ucfirst ); - // Don't duplicate "interface/trait" in the filename. + // Don't duplicate "interface/trait/enum" in the filename. $expected_suffix = '-' . $oo_type; if ( \substr( $expected, -\strlen( $expected_suffix ) ) !== $expected_suffix ) { $expected .= $expected_suffix; diff --git a/Yoast/Tests/Files/FileNameUnitTest.php b/Yoast/Tests/Files/FileNameUnitTest.php index 4f86dc72..bda16a37 100644 --- a/Yoast/Tests/Files/FileNameUnitTest.php +++ b/Yoast/Tests/Files/FileNameUnitTest.php @@ -71,6 +71,14 @@ final class FileNameUnitTest extends AbstractSniffUnitTest { 'no-duplicate-trait.inc' => 0, // Check that 'Trait' in trait name does not cause duplication in filename. 'excluded-trait-file.inc' => 0, + // Enum file names. + 'something-enum.inc' => 0, + 'different-enum.inc' => 1, // Filename not in line with enum name. + 'something.inc' => 1, // Missing '-enum' suffix. + 'yoast-something.inc' => 1, // Prefix 'yoast' not needed. + 'no-duplicate-enum.inc' => 0, // Check that 'Enum' in enum name does not cause duplication in filename. + 'excluded-enum.inc' => 0, + // Functions file names. 'functions.inc' => 0, 'some-functions.inc' => 0, diff --git a/Yoast/Tests/Files/FileNameUnitTests/enums/different-enum.inc b/Yoast/Tests/Files/FileNameUnitTests/enums/different-enum.inc new file mode 100644 index 00000000..5df3f49a --- /dev/null +++ b/Yoast/Tests/Files/FileNameUnitTests/enums/different-enum.inc @@ -0,0 +1,8 @@ + +phpcs:set Yoast.Files.FileName oo_prefixes[] wpseo,yoast + + +phpcs:set Yoast.Files.FileName excluded_files_strict_check[] enums/excluded-enum.inc + + +phpcs:set Yoast.Files.FileName oo_prefixes[] wpseo,yoast + + +phpcs:set Yoast.Files.FileName oo_prefixes[] wpseo,yoast + + +phpcs:set Yoast.Files.FileName oo_prefixes[] wpseo,yoast + + +phpcs:set Yoast.Files.FileName oo_prefixes[] wpseo,yoast + + Date: Sun, 22 Oct 2023 08:30:31 +0200 Subject: [PATCH 14/19] Files/FileName: bug fix - excluded paths were not always normalized correctly The `is_file_excluded()` method would first call the `clean_custom_array_property()` method with the `$flip` parameter set to `true`, resulting in an array containing the excluded files as the keys. After that it would call the `normalize_directory_separators()` method on all _values_ via an `array_map()`. In effect, this means that the actual values, which are stored in the array _keys_ would still not be normalized, meaning the "is file excluded" check could return the wrong result for ruleset provided paths containing a backslash directory separator. Even though, in practice, this didn't lead to problems as the paths in rulesets are set using forward slashes, it is still a bug. Fixed now by no longer "flipping" the array. And as the `$flip` parameter of the `clean_custom_array_property()` method is now no longer used, the parameter and its handling has been removed from the function. Includes test. --- Yoast/Sniffs/Files/FileNameSniff.php | 15 ++++----------- Yoast/Tests/Files/FileNameUnitTest.php | 1 + .../classes/excluded-backslash-file.inc | 8 ++++++++ 3 files changed, 13 insertions(+), 11 deletions(-) create mode 100644 Yoast/Tests/Files/FileNameUnitTests/classes/excluded-backslash-file.inc diff --git a/Yoast/Sniffs/Files/FileNameSniff.php b/Yoast/Sniffs/Files/FileNameSniff.php index 4e1f7ec2..d6c5b9b1 100644 --- a/Yoast/Sniffs/Files/FileNameSniff.php +++ b/Yoast/Sniffs/Files/FileNameSniff.php @@ -234,7 +234,7 @@ public function process( File $phpcsFile, $stackPtr ) { * @return bool */ private function is_file_excluded( File $phpcsFile, $path_to_file ) { - $exclude = $this->clean_custom_array_property( $this->excluded_files_strict_check, true, true ); + $exclude = $this->clean_custom_array_property( $this->excluded_files_strict_check, true ); if ( empty( $exclude ) ) { return false; @@ -258,7 +258,7 @@ private function is_file_excluded( File $phpcsFile, $path_to_file ) { // Lowercase the filename to not interfere with the lowercase/dashes rule. $path_to_file = \strtolower( \ltrim( $path_to_file, '/' ) ); - return isset( $exclude[ $path_to_file ] ); + return \in_array( $path_to_file, $exclude, true ); } /** @@ -268,25 +268,18 @@ private function is_file_excluded( File $phpcsFile, $path_to_file ) { * - Remove whitespace surrounding values. * - Remove empty array entries. * - * Optionally flips the array to allow for using `isset` instead of `in_array`. - * * @param array $property The current property value. - * @param bool $flip Whether to flip the array values to keys. * @param bool $to_lower Whether to lowercase the array values. * - * @return array|array + * @return array */ - private function clean_custom_array_property( $property, $flip = false, $to_lower = false ) { + private function clean_custom_array_property( $property, $to_lower = false ) { $property = \array_filter( \array_map( 'trim', $property ) ); if ( $to_lower === true ) { $property = \array_map( 'strtolower', $property ); } - if ( $flip === true ) { - $property = \array_fill_keys( $property, false ); - } - return $property; } diff --git a/Yoast/Tests/Files/FileNameUnitTest.php b/Yoast/Tests/Files/FileNameUnitTest.php index bda16a37..277190ae 100644 --- a/Yoast/Tests/Files/FileNameUnitTest.php +++ b/Yoast/Tests/Files/FileNameUnitTest.php @@ -54,6 +54,7 @@ final class FileNameUnitTest extends AbstractSniffUnitTest { 'yoast.inc' => 0, // Class name = prefix, so there would be nothing left otherwise. 'class-wpseo-some-class.inc' => 1, // Prefixes 'class' and 'wpseo' not necessary. 'excluded-CLASS-file.inc' => 1, // Lowercase expected. + 'excluded-backslash-file.inc' => 0, // Interface file names. 'outline-something-interface.inc' => 0, diff --git a/Yoast/Tests/Files/FileNameUnitTests/classes/excluded-backslash-file.inc b/Yoast/Tests/Files/FileNameUnitTests/classes/excluded-backslash-file.inc new file mode 100644 index 00000000..71c95b11 --- /dev/null +++ b/Yoast/Tests/Files/FileNameUnitTests/classes/excluded-backslash-file.inc @@ -0,0 +1,8 @@ + +phpcs:set Yoast.Files.FileName excluded_files_strict_check[] classes\excluded-backslash-file.inc + + Date: Sun, 22 Oct 2023 09:36:15 +0200 Subject: [PATCH 15/19] Files/FileName: simplification in excluded file comparison There is no need to lowercase the "excluded files" + the file name for the `is_file_excluded()` check. Checking these in their original, proper case should be just fine. And as the `$to_lower` parameter of the `clean_custom_array_property()` method is now no longer used, the parameter and its handling has been removed from the function. Includes additional test safeguarding that the comparison is done case-sensitively. --- Yoast/Sniffs/Files/FileNameSniff.php | 16 ++++------------ Yoast/Tests/Files/FileNameUnitTest.php | 1 + .../classes/excluded-class-wrong-case.inc | 8 ++++++++ 3 files changed, 13 insertions(+), 12 deletions(-) create mode 100644 Yoast/Tests/Files/FileNameUnitTests/classes/excluded-class-wrong-case.inc diff --git a/Yoast/Sniffs/Files/FileNameSniff.php b/Yoast/Sniffs/Files/FileNameSniff.php index d6c5b9b1..a8337a03 100644 --- a/Yoast/Sniffs/Files/FileNameSniff.php +++ b/Yoast/Sniffs/Files/FileNameSniff.php @@ -234,7 +234,7 @@ public function process( File $phpcsFile, $stackPtr ) { * @return bool */ private function is_file_excluded( File $phpcsFile, $path_to_file ) { - $exclude = $this->clean_custom_array_property( $this->excluded_files_strict_check, true ); + $exclude = $this->clean_custom_array_property( $this->excluded_files_strict_check ); if ( empty( $exclude ) ) { return false; @@ -255,8 +255,7 @@ private function is_file_excluded( File $phpcsFile, $path_to_file ) { $path_to_file = Common::stripBasepath( $path_to_file, $base_path ); } - // Lowercase the filename to not interfere with the lowercase/dashes rule. - $path_to_file = \strtolower( \ltrim( $path_to_file, '/' ) ); + $path_to_file = \ltrim( $path_to_file, '/' ); return \in_array( $path_to_file, $exclude, true ); } @@ -269,18 +268,11 @@ private function is_file_excluded( File $phpcsFile, $path_to_file ) { * - Remove empty array entries. * * @param array $property The current property value. - * @param bool $to_lower Whether to lowercase the array values. * * @return array */ - private function clean_custom_array_property( $property, $to_lower = false ) { - $property = \array_filter( \array_map( 'trim', $property ) ); - - if ( $to_lower === true ) { - $property = \array_map( 'strtolower', $property ); - } - - return $property; + private function clean_custom_array_property( $property ) { + return \array_filter( \array_map( 'trim', $property ) ); } /** diff --git a/Yoast/Tests/Files/FileNameUnitTest.php b/Yoast/Tests/Files/FileNameUnitTest.php index 277190ae..69408edd 100644 --- a/Yoast/Tests/Files/FileNameUnitTest.php +++ b/Yoast/Tests/Files/FileNameUnitTest.php @@ -55,6 +55,7 @@ final class FileNameUnitTest extends AbstractSniffUnitTest { 'class-wpseo-some-class.inc' => 1, // Prefixes 'class' and 'wpseo' not necessary. 'excluded-CLASS-file.inc' => 1, // Lowercase expected. 'excluded-backslash-file.inc' => 0, + 'excluded-class-wrong-case.inc' => 1, // Filename not in line with class name. File not excluded due to wrong case used. // Interface file names. 'outline-something-interface.inc' => 0, diff --git a/Yoast/Tests/Files/FileNameUnitTests/classes/excluded-class-wrong-case.inc b/Yoast/Tests/Files/FileNameUnitTests/classes/excluded-class-wrong-case.inc new file mode 100644 index 00000000..433fd45e --- /dev/null +++ b/Yoast/Tests/Files/FileNameUnitTests/classes/excluded-class-wrong-case.inc @@ -0,0 +1,8 @@ + +phpcs:set Yoast.Files.FileName excluded_files_strict_check[] classes/excluded-Class-wrong-Case.inc + + Date: Sun, 22 Oct 2023 08:09:12 +0200 Subject: [PATCH 16/19] Files/FileName: only clean up the OO prefixes when needed Efficiency tweak. Cleaning the ruleset passed OO prefixes doesn't need to be done time and again every single time this sniff is called. For a normal PHPCS run, where the property is set in a ruleset, doing it once and reusing the cleaned up version in subsequent calls to the sniff will be sufficient. For test runs, this may need to be done more often, but that can be handled by storing the previous value of `$oo_prefixes` property and checking if it has changed before doing the validation. This commit implements this. --- Yoast/Sniffs/Files/FileNameSniff.php | 48 +++++++++++++++++++++++++--- 1 file changed, 43 insertions(+), 5 deletions(-) diff --git a/Yoast/Sniffs/Files/FileNameSniff.php b/Yoast/Sniffs/Files/FileNameSniff.php index a8337a03..63a84f41 100644 --- a/Yoast/Sniffs/Files/FileNameSniff.php +++ b/Yoast/Sniffs/Files/FileNameSniff.php @@ -73,6 +73,22 @@ final class FileNameSniff implements Sniff { */ public $excluded_files_strict_check = []; + /** + * Cache of previously set OO prefixes. + * + * Prevents having to do the same prefix validation over and over again. + * + * @var array + */ + private $previous_oo_prefixes = []; + + /** + * Validated & cleaned up OO set prefixes. + * + * @var array + */ + private $clean_oo_prefixes = []; + /** * Returns an array of tokens this test wants to listen for. * @@ -153,11 +169,9 @@ public function process( File $phpcsFile, $stackPtr ) { $oo_name = ObjectDeclarations::getName( $phpcsFile, $oo_structure ); if ( ! empty( $oo_name ) ) { - $prefixes = $this->clean_custom_array_property( $this->oo_prefixes ); - if ( ! empty( $prefixes ) ) { - // Use reverse natural sorting to get the longest of overlapping prefixes first. - \rsort( $prefixes, ( \SORT_NATURAL | \SORT_FLAG_CASE ) ); - foreach ( $prefixes as $prefix ) { + $this->validate_oo_prefixes(); + if ( ! empty( $this->clean_oo_prefixes ) ) { + foreach ( $this->clean_oo_prefixes as $prefix ) { if ( $oo_name !== $prefix && \stripos( $oo_name, $prefix ) === 0 ) { $oo_name = \substr( $oo_name, \strlen( $prefix ) ); $oo_name = \ltrim( $oo_name, '_-' ); @@ -285,4 +299,28 @@ private function clean_custom_array_property( $property ) { private function normalize_directory_separators( $path ) { return \ltrim( \strtr( $path, '\\', '/' ), '/' ); } + + /** + * Validate and sort the OO prefixes passed from a custom ruleset. + * + * This will only need to be done once in a normal PHPCS run, though for + * tests the function may be called multiple times. + * + * @return void + */ + private function validate_oo_prefixes() { + if ( $this->previous_oo_prefixes === $this->oo_prefixes ) { + return; + } + + // Set the cache *before* validation so as to not break the above compare. + $this->previous_oo_prefixes = $this->oo_prefixes; + + $this->clean_oo_prefixes = $this->clean_custom_array_property( $this->oo_prefixes ); + + if ( ! empty( $this->clean_oo_prefixes ) ) { + // Use reverse natural sorting to get the longest of overlapping prefixes first. + \rsort( $this->clean_oo_prefixes, ( \SORT_NATURAL | \SORT_FLAG_CASE ) ); + } + } } From a0e1125647d95a86327a0112241a2fda6334c908 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 22 Oct 2023 10:04:10 +0200 Subject: [PATCH 17/19] Files/FileName: only clean up the excluded files when needed Efficiency tweak. Cleaning the ruleset passed "excluded files" doesn't need to be done time and again every single time this sniff is called. For a normal PHPCS run, where the property is set in a ruleset, doing it once and reusing the cleaned up version in subsequent calls to the sniff will be sufficient. For test runs, this may need to be done more often, but that can be handled by storing the previous value of `$excluded_files_strict_check` property and checking if it has changed before doing the validation. This commit implements this. Additionally it adds some extra safeguards for incorrectly passed "excluded files" paths. Includes removing the `$phpcsFile` parameter from the `is_file_excluded()` method as it is no longer needed by that method. Includes additional unit tests. Includes updating a pre-existing test to pass duplicate excluded files in different ways. --- Yoast/Sniffs/Files/FileNameSniff.php | 104 ++++++++++++++---- Yoast/Tests/Files/FileNameUnitTest.php | 3 + .../classes/excluded-dot-prefixed.inc | 8 ++ .../classes/excluded-illegal.inc | 8 ++ .../classes/excluded-multiple.inc | 8 ++ .../Files/FileNameUnitTests/excluded-file.inc | 2 +- 6 files changed, 113 insertions(+), 20 deletions(-) create mode 100644 Yoast/Tests/Files/FileNameUnitTests/classes/excluded-dot-prefixed.inc create mode 100644 Yoast/Tests/Files/FileNameUnitTests/classes/excluded-illegal.inc create mode 100644 Yoast/Tests/Files/FileNameUnitTests/classes/excluded-multiple.inc diff --git a/Yoast/Sniffs/Files/FileNameSniff.php b/Yoast/Sniffs/Files/FileNameSniff.php index 63a84f41..e7f5c66d 100644 --- a/Yoast/Sniffs/Files/FileNameSniff.php +++ b/Yoast/Sniffs/Files/FileNameSniff.php @@ -4,7 +4,6 @@ use PHP_CodeSniffer\Files\File; use PHP_CodeSniffer\Sniffs\Sniff; -use PHP_CodeSniffer\Util\Common; use PHPCSUtils\Tokens\Collections; use PHPCSUtils\Utils\ObjectDeclarations; use PHPCSUtils\Utils\TextStrings; @@ -89,6 +88,22 @@ final class FileNameSniff implements Sniff { */ private $clean_oo_prefixes = []; + /** + * Cache of previously set list of excluded files. + * + * Prevents having to do the same file validation over and over again. + * + * @var array + */ + private $previous_excluded_files = []; + + /** + * Validated & cleaned up list of absolute paths to the excluded files. + * + * @var array Key is the path, value irrelevant. + */ + private $validated_excluded_files = []; + /** * Returns an array of tokens this test wants to listen for. * @@ -248,30 +263,15 @@ public function process( File $phpcsFile, $stackPtr ) { * @return bool */ private function is_file_excluded( File $phpcsFile, $path_to_file ) { - $exclude = $this->clean_custom_array_property( $this->excluded_files_strict_check ); - - if ( empty( $exclude ) ) { + $this->validate_excluded_files( $phpcsFile ); + if ( empty( $this->validated_excluded_files ) ) { return false; } - $exclude = \array_map( [ $this, 'normalize_directory_separators' ], $exclude ); $path_to_file = $this->normalize_directory_separators( $path_to_file ); - - if ( ! isset( $phpcsFile->config->basepath ) ) { - $phpcsFile->addWarning( - 'For the exclude property to work with relative file path files, the --basepath needs to be set.', - 0, - 'MissingBasePath' - ); - } - else { - $base_path = $this->normalize_directory_separators( $phpcsFile->config->basepath ); - $path_to_file = Common::stripBasepath( $path_to_file, $base_path ); - } - $path_to_file = \ltrim( $path_to_file, '/' ); - return \in_array( $path_to_file, $exclude, true ); + return isset( $this->validated_excluded_files[ $path_to_file ] ); } /** @@ -323,4 +323,70 @@ private function validate_oo_prefixes() { \rsort( $this->clean_oo_prefixes, ( \SORT_NATURAL | \SORT_FLAG_CASE ) ); } } + + /** + * Validate the list of excluded files passed from a custom ruleset. + * + * This will only need to be done once in a normal PHPCS run, though for + * tests the function may be called multiple times. + * + * @param File $phpcsFile The file being scanned. + * + * @return void + */ + private function validate_excluded_files( $phpcsFile ) { + // The basepath check needs to be done first as otherwise the previous/current comparison would be broken. + if ( ! isset( $phpcsFile->config->basepath ) ) { + $phpcsFile->addWarning( + 'For the exclude property to work with relative file path files, the --basepath needs to be set.', + 0, + 'MissingBasePath' + ); + + // Only relevant for the tests: make sure previously set validated paths are cleared out. + $this->validated_excluded_files = []; + + // No use continuing as we can't turn relative paths into absolute paths. + return; + } + + if ( $this->previous_excluded_files === $this->excluded_files_strict_check ) { + return; + } + + // Set the cache *before* validation so as to not break the above compare. + $this->previous_excluded_files = $this->excluded_files_strict_check; + + // Reset a potentially previous set validated value. + $this->validated_excluded_files = []; + + $exclude = $this->clean_custom_array_property( $this->excluded_files_strict_check ); + if ( empty( $exclude ) ) { + return; + } + + $base_path = $this->normalize_directory_separators( $phpcsFile->config->basepath ); + $exclude = \array_map( [ $this, 'normalize_directory_separators' ], $exclude ); + + foreach ( $exclude as $relative ) { + if ( \strpos( $relative, '..' ) !== false ) { + // Ignore paths containing path walking. + continue; + } + + if ( \strpos( $relative, './' ) === 0 ) { + $relative = \substr( $relative, 2 ); + } + + /* + * Note: no need to check if the file really exists. We'll be doing a literal absolute path comparison, + * so if the file doesn't exist, it will never match. + */ + $this->validated_excluded_files[] = $base_path . '/' . $relative; + } + + if ( ! empty( $this->validated_excluded_files ) ) { + $this->validated_excluded_files = \array_flip( $this->validated_excluded_files ); + } + } } diff --git a/Yoast/Tests/Files/FileNameUnitTest.php b/Yoast/Tests/Files/FileNameUnitTest.php index 69408edd..4711af0e 100644 --- a/Yoast/Tests/Files/FileNameUnitTest.php +++ b/Yoast/Tests/Files/FileNameUnitTest.php @@ -56,6 +56,9 @@ final class FileNameUnitTest extends AbstractSniffUnitTest { 'excluded-CLASS-file.inc' => 1, // Lowercase expected. 'excluded-backslash-file.inc' => 0, 'excluded-class-wrong-case.inc' => 1, // Filename not in line with class name. File not excluded due to wrong case used. + 'excluded-illegal.inc' => 1, // Filename not in line with class name. File not excluded due to illegal path setting. + 'excluded-multiple.inc' => 0, + 'excluded-dot-prefixed.inc' => 0, // Interface file names. 'outline-something-interface.inc' => 0, diff --git a/Yoast/Tests/Files/FileNameUnitTests/classes/excluded-dot-prefixed.inc b/Yoast/Tests/Files/FileNameUnitTests/classes/excluded-dot-prefixed.inc new file mode 100644 index 00000000..4282178b --- /dev/null +++ b/Yoast/Tests/Files/FileNameUnitTests/classes/excluded-dot-prefixed.inc @@ -0,0 +1,8 @@ + +phpcs:set Yoast.Files.FileName excluded_files_strict_check[] ./classes/excluded-dot-prefixed.inc + + +phpcs:set Yoast.Files.FileName excluded_files_strict_check[] classes/../classes/excluded-illegal.inc + + +phpcs:set Yoast.Files.FileName excluded_files_strict_check[] classes/excluded-CLASS-file.inc,classes/excluded-multiple.inc + + -phpcs:set Yoast.Files.FileName excluded_files_strict_check[] excluded-file.inc +phpcs:set Yoast.Files.FileName excluded_files_strict_check[] excluded-file.inc,./excluded-file.inc,/excluded-file.inc Date: Mon, 23 Oct 2023 06:04:22 +0200 Subject: [PATCH 18/19] Files/FileName: only throw the "missing basepath" warning once No need to throw this for every single file. Throwing it once should be sufficient. Includes moving the check for the missing basepath to earlier in the process flow. --- Yoast/Sniffs/Files/FileNameSniff.php | 40 +++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 6 deletions(-) diff --git a/Yoast/Sniffs/Files/FileNameSniff.php b/Yoast/Sniffs/Files/FileNameSniff.php index e7f5c66d..e139f2b4 100644 --- a/Yoast/Sniffs/Files/FileNameSniff.php +++ b/Yoast/Sniffs/Files/FileNameSniff.php @@ -104,6 +104,15 @@ final class FileNameSniff implements Sniff { */ private $validated_excluded_files = []; + /** + * Track if the "missing basepath" warning has been thrown. + * + * This prevents this warning potentially being thrown for every single file in a PHPCS run. + * + * @var bool + */ + private $basepath_warning_thrown = false; + /** * Returns an array of tokens this test wants to listen for. * @@ -177,6 +186,10 @@ public function process( File $phpcsFile, $stackPtr ) { $error_code = 'NotHyphenatedLowercase'; $expected = \strtolower( \preg_replace( '`[[:punct:]]`', '-', $file_name ) ); + if ( ! isset( $phpcsFile->config->basepath ) ) { + $this->add_missing_basepath_warning( $phpcsFile ); + } + if ( $this->is_file_excluded( $phpcsFile, $file ) === false ) { $oo_structure = $phpcsFile->findNext( self::NAMED_OO_TOKENS, $stackPtr ); if ( $oo_structure !== false ) { @@ -337,12 +350,6 @@ private function validate_oo_prefixes() { private function validate_excluded_files( $phpcsFile ) { // The basepath check needs to be done first as otherwise the previous/current comparison would be broken. if ( ! isset( $phpcsFile->config->basepath ) ) { - $phpcsFile->addWarning( - 'For the exclude property to work with relative file path files, the --basepath needs to be set.', - 0, - 'MissingBasePath' - ); - // Only relevant for the tests: make sure previously set validated paths are cleared out. $this->validated_excluded_files = []; @@ -389,4 +396,25 @@ private function validate_excluded_files( $phpcsFile ) { $this->validated_excluded_files = \array_flip( $this->validated_excluded_files ); } } + + /** + * Throw a warning if the basepath is missing (and this warning hasn't been thrown before). + * + * @param File $phpcsFile The file being scanned. + * + * @return void + */ + private function add_missing_basepath_warning( File $phpcsFile ) { + if ( $this->basepath_warning_thrown === true ) { + return; + } + + $phpcsFile->addWarning( + 'For the exclude property to work with relative file path files, the --basepath needs to be set.', + 0, + 'MissingBasePath' + ); + + $this->basepath_warning_thrown = true; + } } From b94cf92e91ea5453429fd5c7b381f3cc5ded3c44 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 22 Oct 2023 10:08:50 +0200 Subject: [PATCH 19/19] Files/FileName: implement use of new PathHelper and PathValidationHelper classes --- Yoast/Sniffs/Files/FileNameSniff.php | 52 +++++----------------------- 1 file changed, 8 insertions(+), 44 deletions(-) diff --git a/Yoast/Sniffs/Files/FileNameSniff.php b/Yoast/Sniffs/Files/FileNameSniff.php index e139f2b4..c08e1ac3 100644 --- a/Yoast/Sniffs/Files/FileNameSniff.php +++ b/Yoast/Sniffs/Files/FileNameSniff.php @@ -7,6 +7,8 @@ use PHPCSUtils\Tokens\Collections; use PHPCSUtils\Utils\ObjectDeclarations; use PHPCSUtils\Utils\TextStrings; +use YoastCS\Yoast\Utils\PathHelper; +use YoastCS\Yoast\Utils\PathValidationHelper; /** * Ensures files comply with the Yoast file name rules. @@ -100,7 +102,7 @@ final class FileNameSniff implements Sniff { /** * Validated & cleaned up list of absolute paths to the excluded files. * - * @var array Key is the path, value irrelevant. + * @var array Both the key and the value will be the same absolute path. */ private $validated_excluded_files = []; @@ -281,8 +283,7 @@ private function is_file_excluded( File $phpcsFile, $path_to_file ) { return false; } - $path_to_file = $this->normalize_directory_separators( $path_to_file ); - $path_to_file = \ltrim( $path_to_file, '/' ); + $path_to_file = PathHelper::normalize_path( $path_to_file ); return isset( $this->validated_excluded_files[ $path_to_file ] ); } @@ -302,17 +303,6 @@ private function clean_custom_array_property( $property ) { return \array_filter( \array_map( 'trim', $property ) ); } - /** - * Normalize all directory separators to be a forward slash and remove prefixed slash. - * - * @param string $path Path to normalize. - * - * @return string - */ - private function normalize_directory_separators( $path ) { - return \ltrim( \strtr( $path, '\\', '/' ), '/' ); - } - /** * Validate and sort the OO prefixes passed from a custom ruleset. * @@ -364,37 +354,11 @@ private function validate_excluded_files( $phpcsFile ) { // Set the cache *before* validation so as to not break the above compare. $this->previous_excluded_files = $this->excluded_files_strict_check; - // Reset a potentially previous set validated value. - $this->validated_excluded_files = []; - - $exclude = $this->clean_custom_array_property( $this->excluded_files_strict_check ); - if ( empty( $exclude ) ) { - return; - } + $absolute_paths = PathValidationHelper::relative_to_absolute( $phpcsFile, $this->excluded_files_strict_check ); + $absolute_paths = \array_unique( $absolute_paths ); + $absolute_paths = \array_values( $absolute_paths ); - $base_path = $this->normalize_directory_separators( $phpcsFile->config->basepath ); - $exclude = \array_map( [ $this, 'normalize_directory_separators' ], $exclude ); - - foreach ( $exclude as $relative ) { - if ( \strpos( $relative, '..' ) !== false ) { - // Ignore paths containing path walking. - continue; - } - - if ( \strpos( $relative, './' ) === 0 ) { - $relative = \substr( $relative, 2 ); - } - - /* - * Note: no need to check if the file really exists. We'll be doing a literal absolute path comparison, - * so if the file doesn't exist, it will never match. - */ - $this->validated_excluded_files[] = $base_path . '/' . $relative; - } - - if ( ! empty( $this->validated_excluded_files ) ) { - $this->validated_excluded_files = \array_flip( $this->validated_excluded_files ); - } + $this->validated_excluded_files = \array_combine( $absolute_paths, $absolute_paths ); } /**