From 6e5ddb69d4f88294b3a4ef964f2896792340fb93 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 19 Oct 2023 05:47:36 +0200 Subject: [PATCH 01/14] NamingConventions/NamespaceName: minor documentation tweaks --- .../Sniffs/NamingConventions/NamespaceNameSniff.php | 12 ++++++------ .../NamingConventions/NamespaceNameUnitTest.php | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php b/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php index afc974ff..cf96b536 100644 --- a/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php +++ b/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php @@ -35,7 +35,7 @@ final class NamespaceNameSniff implements Sniff { * one or more sub-directories of the project root can be indicated * as starting points for the translation. * - * @var string[] + * @var array */ public $src_directory = []; @@ -71,7 +71,7 @@ final class NamespaceNameSniff implements Sniff { * - not be prefixed with a slash; * - have a trailing slash. * - * @var string[] + * @var array */ private $validated_src_directory = []; @@ -80,7 +80,7 @@ final class NamespaceNameSniff implements Sniff { * * Prevents having to do the same validation over and over again. * - * @var string[] + * @var array */ private $previous_src_directory = []; @@ -96,9 +96,9 @@ public function register() { /** * Filter out all prefixes which don't have namespace separators. * - * @param string[] $prefixes The unvalidated prefixes. + * @param array $prefixes The unvalidated prefixes. * - * @return string[] + * @return array */ protected function filter_prefixes( $prefixes ) { return $this->filter_allow_only_namespace_prefixes( $prefixes ); @@ -149,7 +149,7 @@ public function process( File $phpcsFile, $stackPtr ) { $this->validate_prefixes(); - // Strip off the plugin prefix. + // Strip off the (longest) plugin prefix. $namespace_name_no_prefix = $namespace_name; $found_prefix = ''; if ( ! empty( $this->validated_prefixes ) ) { diff --git a/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.php b/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.php index e2b3c904..330973c2 100644 --- a/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.php +++ b/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.php @@ -36,7 +36,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 ddae5aa1fe1813064df96e33be6b32a8962ef07a Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 21 Oct 2023 16:04:29 +0200 Subject: [PATCH 02/14] NamingConventions/NamespaceName: minor tweak Ignore an untestable line for code coverage analysis. --- Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php b/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php index cf96b536..6fe0d0b2 100644 --- a/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php +++ b/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php @@ -226,7 +226,7 @@ public function process( File $phpcsFile, $stackPtr ) { $file = \preg_replace( '`^([\'"])(.*)\1$`Ds', '$2', $phpcsFile->getFileName() ); if ( $file === 'STDIN' ) { - return; + return; // @codeCoverageIgnore } $directory = $this->normalize_directory_separators( \dirname( $file ) ); From 0da762d93f24fa031ef84640c97122efdd2de604 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 1 Nov 2023 00:14:31 +0100 Subject: [PATCH 03/14] NamingConventions/NamespaceName: tweak the test case files * Tie the test description comments closer together with the settings being tested. * Consolidate "mismatch" tests. No need for two test files. One path translates to one valid namespace (depending on settings), so all possible cases for one path can always be tested within one test case file. * Remove parse error from mismatch test. * Add one additional test case to the `src/sub-b/path-translation-src-sub-b.inc` test case file. --- .../NamespaceNameUnitTest.php | 9 +++------ .../src/path-translation-ignore-src.inc | 6 +++--- .../path-translation-ignore-src-sub-path.inc | 6 +++--- .../path-translation-mismatch-illegal.inc | 16 --------------- .../mismatch/path-translation-mismatch.inc | 17 +++++++++++++--- .../NamespaceNameUnitTests/no-basepath.inc | 20 +++++++++---------- .../path-translation-root.inc | 4 ++-- .../path/path-translation-sub1.inc | 4 ++-- .../path/sub-path/path-translation-sub2.inc | 4 ++-- .../secondary/path-translation-secondary.inc | 6 +++--- .../path-translation-secondary-sub-a.inc | 6 +++--- .../src/path-translation-src.inc | 6 +++--- .../src/sub-a/path-translation-src-sub-a.inc | 6 +++--- .../src/sub-b/path-translation-src-sub-b.inc | 7 ++++--- 14 files changed, 55 insertions(+), 62 deletions(-) delete mode 100644 Yoast/Tests/NamingConventions/NamespaceNameUnitTests/mismatch/path-translation-mismatch-illegal.inc diff --git a/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.php b/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.php index 330973c2..af408c3f 100644 --- a/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.php +++ b/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.php @@ -124,6 +124,7 @@ public function getErrorList( string $testFile = '' ): array { case 'path-translation-src-sub-b.inc': return [ 14 => 1, + 15 => 1, ]; // Path translation with multiple items in $src_directory tests. @@ -153,12 +154,8 @@ public function getErrorList( string $testFile = '' ): array { // Path translation with no matching $src_directory. case 'path-translation-mismatch.inc': return [ - 13 => 1, - ]; - - case 'path-translation-mismatch-illegal.inc': - return [ - 12 => 1, + 14 => 1, + 24 => 1, ]; default: diff --git a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/ignore/src/path-translation-ignore-src.inc b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/ignore/src/path-translation-ignore-src.inc index 6c7a78cc..64ff5ae8 100644 --- a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/ignore/src/path-translation-ignore-src.inc +++ b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/ignore/src/path-translation-ignore-src.inc @@ -2,11 +2,11 @@ /* * Testing path translation in combination with multi-level src_directory. + * + * phpcs:set Yoast.NamingConventions.NamespaceName prefixes[] Yoast\WP\Plugin + * phpcs:set Yoast.NamingConventions.NamespaceName src_directory[] ignore/src */ -// phpcs:set Yoast.NamingConventions.NamespaceName prefixes[] Yoast\WP\Plugin -// phpcs:set Yoast.NamingConventions.NamespaceName src_directory[] ignore/src - namespace Yoast\WP\Plugin; // OK. namespace Yoast\WP\Plugin\Ignore\Src; // Error. diff --git a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/ignore/src/sub-path/path-translation-ignore-src-sub-path.inc b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/ignore/src/sub-path/path-translation-ignore-src-sub-path.inc index 19acea65..6db9128b 100644 --- a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/ignore/src/sub-path/path-translation-ignore-src-sub-path.inc +++ b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/ignore/src/sub-path/path-translation-ignore-src-sub-path.inc @@ -2,11 +2,11 @@ /* * Testing path translation in combination with multi-level src_directory. + * + * phpcs:set Yoast.NamingConventions.NamespaceName prefixes[] Yoast\WP\Plugin + * phpcs:set Yoast.NamingConventions.NamespaceName src_directory[] ignore/src */ -// phpcs:set Yoast.NamingConventions.NamespaceName prefixes[] Yoast\WP\Plugin -// phpcs:set Yoast.NamingConventions.NamespaceName src_directory[] ignore/src - namespace Yoast\WP\Plugin\Sub_Path; // OK. namespace Yoast\WP\Plugin; // Error. diff --git a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/mismatch/path-translation-mismatch-illegal.inc b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/mismatch/path-translation-mismatch-illegal.inc deleted file mode 100644 index d144a19e..00000000 --- a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/mismatch/path-translation-mismatch-illegal.inc +++ /dev/null @@ -1,16 +0,0 @@ - max). + * + * @phpcs:set Yoast.NamingConventions.NamespaceName max_levels 2 + * @phpcs:set Yoast.NamingConventions.NamespaceName recommended_max_levels 5 */ -// phpcs:set Yoast.NamingConventions.NamespaceName max_levels 2 -// phpcs:set Yoast.NamingConventions.NamespaceName recommended_max_levels 5 - namespace Yoast\WP\Plugin\Foo\Bar; // OK. namespace Yoast\WP\Plugin\Foo\Bar\Baz; // Error. namespace Yoast\WP\Plugin\Foo\Bar\Baz\Fro; // Error. diff --git a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/path-translation-root.inc b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/path-translation-root.inc index cd40436c..f40e91e2 100644 --- a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/path-translation-root.inc +++ b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/path-translation-root.inc @@ -2,10 +2,10 @@ /* * Testing path translation. + * + * phpcs:set Yoast.NamingConventions.NamespaceName prefixes[] Yoast\WP\Plugin,yoast_plugin */ -// phpcs:set Yoast.NamingConventions.NamespaceName prefixes[] Yoast\WP\Plugin,yoast_plugin - namespace Yoast\WP\Plugin; // OK. namespace Yoast\WP\Plugin\Foo; // Error. diff --git a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/path/path-translation-sub1.inc b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/path/path-translation-sub1.inc index aa01e27b..786bfe27 100644 --- a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/path/path-translation-sub1.inc +++ b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/path/path-translation-sub1.inc @@ -2,10 +2,10 @@ /* * Testing path translation. + * + * @phpcs:set Yoast.NamingConventions.NamespaceName prefixes[] Yoast\WP\Plugin */ -// phpcs:set Yoast.NamingConventions.NamespaceName prefixes[] Yoast\WP\Plugin - namespace Yoast\WP\Plugin\Path; // OK. namespace Yoast\WP\Plugin; // Error. diff --git a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/path/sub-path/path-translation-sub2.inc b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/path/sub-path/path-translation-sub2.inc index 80830a74..e85fbe51 100644 --- a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/path/sub-path/path-translation-sub2.inc +++ b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/path/sub-path/path-translation-sub2.inc @@ -2,10 +2,10 @@ /* * Testing path translation. + * + * @phpcs:set Yoast.NamingConventions.NamespaceName prefixes[] Yoast\WP\Plugin */ -// phpcs:set Yoast.NamingConventions.NamespaceName prefixes[] Yoast\WP\Plugin - namespace Yoast\WP\Plugin\path\sub_path; // OK. namespace Yoast\WP\Plugin\Path\Sub_Path; // OK. diff --git a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/secondary/path-translation-secondary.inc b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/secondary/path-translation-secondary.inc index f660ba53..c0026a59 100644 --- a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/secondary/path-translation-secondary.inc +++ b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/secondary/path-translation-secondary.inc @@ -3,11 +3,11 @@ /* * Testing path translation in combination with multiple src_directories. * Includes testing of correct handling of variations of src_directory settings. + * + * phpcs:set Yoast.NamingConventions.NamespaceName prefixes[] Yoast\WP\Plugin + * phpcs:set Yoast.NamingConventions.NamespaceName src_directory[] ./src/,.\secondary\ */ -// phpcs:set Yoast.NamingConventions.NamespaceName prefixes[] Yoast\WP\Plugin -// phpcs:set Yoast.NamingConventions.NamespaceName src_directory[] ./src/,.\secondary\ - namespace Yoast\WP\Plugin; // OK. namespace Yoast\WP\Plugin\Secondary; // Error. diff --git a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/secondary/sub-a/path-translation-secondary-sub-a.inc b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/secondary/sub-a/path-translation-secondary-sub-a.inc index deaba43e..724718fd 100644 --- a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/secondary/sub-a/path-translation-secondary-sub-a.inc +++ b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/secondary/sub-a/path-translation-secondary-sub-a.inc @@ -2,11 +2,11 @@ /* * Testing path translation in combination with src_directory. + * + * phpcs:set Yoast.NamingConventions.NamespaceName prefixes[] Yoast\WP\Plugin + * phpcs:set Yoast.NamingConventions.NamespaceName src_directory[] src,secondary */ -// phpcs:set Yoast.NamingConventions.NamespaceName prefixes[] Yoast\WP\Plugin -// phpcs:set Yoast.NamingConventions.NamespaceName src_directory[] src,secondary - namespace Yoast\WP\Plugin\Sub_A; // OK. namespace Yoast\WP\Plugin\Secondary\Sub_A; // Error. diff --git a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/src/path-translation-src.inc b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/src/path-translation-src.inc index adba1e7f..38cf4ca0 100644 --- a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/src/path-translation-src.inc +++ b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/src/path-translation-src.inc @@ -2,11 +2,11 @@ /* * Testing path translation in combination with src_directory. + * + * @phpcs:set Yoast.NamingConventions.NamespaceName prefixes[] Yoast\WP\Plugin + * @phpcs:set Yoast.NamingConventions.NamespaceName src_directory[] src */ -// phpcs:set Yoast.NamingConventions.NamespaceName prefixes[] Yoast\WP\Plugin -// phpcs:set Yoast.NamingConventions.NamespaceName src_directory[] src - namespace Yoast\WP\Plugin; // OK. namespace Yoast\WP\Plugin\Src; // Error. diff --git a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/src/sub-a/path-translation-src-sub-a.inc b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/src/sub-a/path-translation-src-sub-a.inc index defe3b83..409f2ab3 100644 --- a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/src/sub-a/path-translation-src-sub-a.inc +++ b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/src/sub-a/path-translation-src-sub-a.inc @@ -3,11 +3,11 @@ /* * Testing path translation in combination with src_directory. * Includes testing of correct handling of variations of src_directory settings. + * + * phpcs:set Yoast.NamingConventions.NamespaceName prefixes[] Yoast\WP\Plugin + * phpcs:set Yoast.NamingConventions.NamespaceName src_directory[] /src/ */ -// phpcs:set Yoast.NamingConventions.NamespaceName prefixes[] Yoast\WP\Plugin -// phpcs:set Yoast.NamingConventions.NamespaceName src_directory[] /src/ - namespace Yoast\WP\Plugin\Sub_A; // OK. namespace Yoast\WP\Plugin\Src\Sub_A; // Error. diff --git a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/src/sub-b/path-translation-src-sub-b.inc b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/src/sub-b/path-translation-src-sub-b.inc index cd4dcf78..7d85044f 100644 --- a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/src/sub-b/path-translation-src-sub-b.inc +++ b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/src/sub-b/path-translation-src-sub-b.inc @@ -4,13 +4,14 @@ * Testing path translation in combination with src_directory. * Includes testing of correct handling of variations of src_directory settings. * Includes testing of matching the longest directory first. + * + * @phpcs:set Yoast.NamingConventions.NamespaceName prefixes[] Yoast\WP\Plugin + * @phpcs:set Yoast.NamingConventions.NamespaceName src_directory[] ./src , ./src/sub-b */ -// phpcs:set Yoast.NamingConventions.NamespaceName prefixes[] Yoast\WP\Plugin -// phpcs:set Yoast.NamingConventions.NamespaceName src_directory[] ./src , ./src/sub-b - namespace Yoast\WP\Plugin; // OK. +namespace Yoast\WP\Plugin\Sub_B; // Error. namespace Yoast\WP\Plugin\Src\Sub_B; // Error. // Reset to default settings. From e4b50d0360db22d622ff1f877d7cfaa360fa26b4 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Tue, 31 Oct 2023 16:27:35 +0100 Subject: [PATCH 04/14] NamingConventions/NamespaceName: add tests with PHP 8.0+ namespace names with reserved keywords These tests should pass without issue since PHPCS 3.7.1, though would have failed on PHP < 8.0 prior to that. Includes adding note to some pre-existing tests about the PHP 8.0 change. --- Yoast/Tests/NamingConventions/NamespaceNameUnitTest.php | 2 ++ .../NamespaceNameUnitTests/no-basepath.inc | 9 ++++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.php b/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.php index af408c3f..6f2047bf 100644 --- a/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.php +++ b/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.php @@ -80,6 +80,7 @@ public function getErrorList( string $testFile = '' ): array { 66 => 1, 70 => 1, 74 => 1, + 81 => 1, ]; case 'no-basepath-scoped.inc': @@ -184,6 +185,7 @@ public function getWarningList( string $testFile = '' ): array { 69 => 1, 72 => 1, 73 => 1, + 80 => 1, ]; case 'no-basepath-scoped.inc': diff --git a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/no-basepath.inc b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/no-basepath.inc index 75ca334f..390184ad 100644 --- a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/no-basepath.inc +++ b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/no-basepath.inc @@ -2,8 +2,8 @@ /* * Testing namespace depth checking, including tolerance for whitespace and comments. + * Note: comments and whitespace in namespace names are a parse error since PHP 8.0. This was fine before that time. */ - namespace Foo \ Bar; // OK. namespace Foo /* comment */ \Bar @@ -73,6 +73,13 @@ namespace Yoast\WP\Plugin\Foo\Tests\Bar; // Warning, `Tests\` counted as not dir namespace Yoast\WP\Plugin\Tests\Foo\Doubles\Bar; // Warning, `Doubles\` counted as not directly after the prefix + `Tests\`. namespace Yoast\WP\Plugin\Doubles\Foo\Bar\Fro; // Error, `Doubles\` counted as not found in combination with `Tests\`. +/* + * Test handling of namespace names with reserved keywords in them (PHP 8.0+). + */ +namespace Yoast\WP\Plugin\Trait\Sub; // OK. +namespace Yoast\WP\Plugin\Foo\Bar\Global; // Warning. +namespace Yoast\WP\Plugin\Foo\Case\Bar\Default; // Error. + // Reset to default settings. // phpcs:set Yoast.NamingConventions.NamespaceName prefixes[] From b0b0ae616c005c41de8443318a6b3575531b5162 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 4 Apr 2020 18:52:22 +0200 Subject: [PATCH 05/14] NamingConventions/NamespaceName: implement use of the PHPCSUtils `Namespaces::getDeclaredName()` method ... to replace a large chunk of code. This also fixes a bug where the original code did not correctly determine the namespace name if it contained a parse error, while the PHPCSUtils method does handle this more robustly. As a consequence of this change, one test expectation changes, but as that test is for a parse error, I'm not concerned about that. --- .../NamingConventions/NamespaceNameSniff.php | 35 +++---------------- .../NamespaceNameUnitTest.php | 1 - .../path/sub-path/path-translation-sub2.inc | 2 +- 3 files changed, 5 insertions(+), 33 deletions(-) diff --git a/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php b/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php index 6fe0d0b2..5cd1775e 100644 --- a/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php +++ b/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php @@ -5,7 +5,7 @@ use PHP_CodeSniffer\Files\File; use PHP_CodeSniffer\Sniffs\Sniff; use PHP_CodeSniffer\Util\Common; -use PHP_CodeSniffer\Util\Tokens; +use PHPCSUtils\Utils\Namespaces; use YoastCS\Yoast\Utils\CustomPrefixesTrait; /** @@ -114,36 +114,9 @@ protected function filter_prefixes( $prefixes ) { */ public function process( File $phpcsFile, $stackPtr ) { - $tokens = $phpcsFile->getTokens(); - - if ( empty( $tokens[ $stackPtr ]['conditions'] ) === false ) { - // Not a namespace declaration. - return; - } - - $next_non_empty = $phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true ); - if ( $tokens[ $next_non_empty ]['code'] === \T_NS_SEPARATOR ) { - // Not a namespace declaration. - return; - } - - // Get the complete namespace name. - $namespace_name = $tokens[ $next_non_empty ]['content']; - for ( $i = ( $next_non_empty + 1 ); $i < $phpcsFile->numTokens; $i++ ) { - if ( isset( Tokens::$emptyTokens[ $tokens[ $i ]['code'] ] ) ) { - continue; - } - - if ( $tokens[ $i ]['code'] !== \T_STRING && $tokens[ $i ]['code'] !== \T_NS_SEPARATOR ) { - // Reached end of the namespace declaration. - break; - } - - $namespace_name .= $tokens[ $i ]['content']; - } - - if ( $i === $phpcsFile->numTokens ) { - // Live coding. + $namespace_name = Namespaces::getDeclaredName( $phpcsFile, $stackPtr ); + if ( empty( $namespace_name ) ) { + // Either not a namespace declaration or global namespace. return; } diff --git a/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.php b/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.php index 6f2047bf..a94e4789 100644 --- a/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.php +++ b/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.php @@ -108,7 +108,6 @@ public function getErrorList( string $testFile = '' ): array { 12 => 1, 13 => 1, 14 => 1, - 15 => 1, ]; // Path translation with $src_directory set tests. diff --git a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/path/sub-path/path-translation-sub2.inc b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/path/sub-path/path-translation-sub2.inc index e85fbe51..a63095fe 100644 --- a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/path/sub-path/path-translation-sub2.inc +++ b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/path/sub-path/path-translation-sub2.inc @@ -12,7 +12,7 @@ namespace Yoast\WP\Plugin\Path\Sub_Path; // OK. namespace Yoast\WP\Plugin; // Error. namespace Yoast\WP\Plugin\Path; // Error. namespace Yoast\WP\Plugin\Path\Bar; // Error. -namespace Yoast\WP\Plugin\Path\Sub-Path; // Error. (Parse error too, illegal dash in namespace). +namespace Yoast\WP\Plugin\Path\Sub-Path; // Parse error, illegal dash in namespace, but that's not the concern of this sniff. // Reset to default settings. // phpcs:set Yoast.NamingConventions.NamespaceName prefixes[] From 1e8202d4eafe5cc039b5b53a4280b2b5c11e63cc Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 18 Oct 2023 02:50:21 +0200 Subject: [PATCH 06/14] NamingConventions/NamespaceName: use PHPCSUtils TextStrings::stripQuotes() --- Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php b/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php index 5cd1775e..8495d714 100644 --- a/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php +++ b/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php @@ -6,6 +6,7 @@ use PHP_CodeSniffer\Sniffs\Sniff; use PHP_CodeSniffer\Util\Common; use PHPCSUtils\Utils\Namespaces; +use PHPCSUtils\Utils\TextStrings; use YoastCS\Yoast\Utils\CustomPrefixesTrait; /** @@ -196,7 +197,7 @@ public function process( File $phpcsFile, $stackPtr ) { $base_path = $this->normalize_directory_separators( $phpcsFile->config->basepath ); // 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; // @codeCoverageIgnore From 0335598b09410c8135b4e00eeca9383c95d9438b Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 17 Nov 2023 01:11:12 +0100 Subject: [PATCH 07/14] NamingConventions/NamespaceName: no need to double trailing slashes Slashes are already being added to the `$name` two lines earlier, so no need to do it again. --- Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php b/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php index 8495d714..25cb6676 100644 --- a/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php +++ b/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php @@ -129,7 +129,7 @@ public function process( File $phpcsFile, $stackPtr ) { if ( ! empty( $this->validated_prefixes ) ) { $name = $namespace_name . '\\'; // Validated prefixes always have a \ at the end. foreach ( $this->validated_prefixes as $prefix ) { - if ( \strpos( $name . '\\', $prefix ) === 0 ) { + if ( \strpos( $name, $prefix ) === 0 ) { $namespace_name_no_prefix = \rtrim( \substr( $name, \strlen( $prefix ) ), '\\' ); $found_prefix = \rtrim( $prefix, '\\' ); break; From 6740e7dc8ef9dab051dc21039ce7f02ff1809f4c Mon Sep 17 00:00:00 2001 From: jrfnl Date: Tue, 31 Oct 2023 03:43:19 +0100 Subject: [PATCH 08/14] NamingConventions/NamespaceName: 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/NamingConventions/NamespaceNameSniff.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php b/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php index 25cb6676..6a79eb88 100644 --- a/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php +++ b/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php @@ -279,7 +279,7 @@ public function process( File $phpcsFile, $stackPtr ) { * * @return void */ - protected function validate_src_directory() { + private function validate_src_directory() { if ( $this->previous_src_directory === $this->src_directory ) { return; } From 69cacbb2c8a4c00196064284a5579eefe26dd5d5 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 2 Nov 2023 13:15:15 +0100 Subject: [PATCH 09/14] NamingConventions/NamespaceName: throw an error if a prefix is expected, but not used The sniff expects a `prefixes` property to be passed from a ruleset and for a valid prefix to be used in a namespace name, however, this was not checked. This commit adds a new error to the sniff which will be thrown if at least one valid prefix has been passed and no valid prefix was used in the namespace name. Includes unit tests + some minor updates to pre-existing tests which will now also throw this error. --- .../NamespaceNameStandard.xml | 21 +++++++- .../NamingConventions/NamespaceNameSniff.php | 17 ++++++ .../NamespaceNameUnitTest.php | 53 +++++++++++-------- .../NamespaceNameUnitTests/no-basepath.inc | 28 +++++++++- .../path-translation-root.inc | 4 +- 5 files changed, 96 insertions(+), 27 deletions(-) diff --git a/Yoast/Docs/NamingConventions/NamespaceNameStandard.xml b/Yoast/Docs/NamingConventions/NamespaceNameStandard.xml index 826f530f..5dbf901d 100644 --- a/Yoast/Docs/NamingConventions/NamespaceNameStandard.xml +++ b/Yoast/Docs/NamingConventions/NamespaceNameStandard.xml @@ -1,10 +1,29 @@ + + + + Yoast\WP\Plugin\Admin\Forms; + ]]> + + + Admin\Forms; + ]]> + + + + validated_prefixes ) && $found_prefix === '' ) { + if ( \count( $this->validated_prefixes ) === 1 ) { + $error = 'A namespace name is required to start with the "%s" prefix.'; + } + else { + $error = 'A namespace name is required to start with one of the following prefixes: "%s"'; + } + + $prefixes = $this->validated_prefixes; + \natcasesort( $prefixes ); + $data = [ \implode( '", "', $prefixes ) ]; + + $phpcsFile->addError( $error, $stackPtr, 'MissingPrefix', $data ); + } + /* * Check the namespace level depth. */ diff --git a/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.php b/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.php index a94e4789..e69f4aa0 100644 --- a/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.php +++ b/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.php @@ -70,17 +70,24 @@ public function getErrorList( string $testFile = '' ): array { // Level check tests. case 'no-basepath.inc': return [ - 12 => 1, - 21 => 1, - 24 => 1, - 33 => 1, - 44 => 1, - 53 => 1, - 54 => 1, - 66 => 1, - 70 => 1, - 74 => 1, - 81 => 1, + 12 => 1, + 21 => 1, + 23 => 1, + 24 => 2, + 33 => 1, + 44 => 1, + 53 => 1, + 54 => 1, + 66 => 1, + 70 => 1, + 74 => 1, + 81 => 1, + 90 => 1, + 91 => 1, + 92 => 1, + 103 => 1, + 104 => 1, + 105 => 1, ]; case 'no-basepath-scoped.inc': @@ -93,7 +100,7 @@ public function getErrorList( string $testFile = '' ): array { case 'path-translation-root.inc': return [ 11 => 1, - 14 => 1, + 14 => 2, ]; case 'path-translation-sub1.inc': @@ -175,16 +182,18 @@ public function getWarningList( string $testFile = '' ): array { // Level check tests. case 'no-basepath.inc': return [ - 8 => 1, - 20 => 1, - 23 => 1, - 32 => 1, - 43 => 1, - 65 => 1, - 69 => 1, - 72 => 1, - 73 => 1, - 80 => 1, + 8 => 1, + 20 => 1, + 23 => 1, + 32 => 1, + 43 => 1, + 65 => 1, + 69 => 1, + 72 => 1, + 73 => 1, + 80 => 1, + 90 => 1, + 103 => 1, ]; case 'no-basepath-scoped.inc': diff --git a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/no-basepath.inc b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/no-basepath.inc index 390184ad..449f6b6b 100644 --- a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/no-basepath.inc +++ b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/no-basepath.inc @@ -20,8 +20,8 @@ namespace Yoast\WP\ /* comment */ Plugin\Foo\Bar; // OK. namespace Yoast\WP\Plugin\Foo\Bar\Baz; // Warning. namespace Yoast \ /* comment */ WP\Plugin\Foo\Bar\Baz\Fro; // Error. -namespace Yoast_Plugin\Foo\Bar; // Warning. Not a namespace prefix, so prefix not taken into account. -namespace Yoast_Plugin\Foo\Bar\Baz\Fro; // Error. +namespace Yoast_Plugin\Foo\Bar; // Warning for object depth. Not a namespace prefix, so prefix not taken into account. Error for not using prefix. +namespace Yoast_Plugin\Foo\Bar\Baz\Fro; // Error for object depth + not using valid prefix. /* * Testing with (correct) prefix. @@ -80,6 +80,30 @@ namespace Yoast\WP\Plugin\Trait\Sub; // OK. namespace Yoast\WP\Plugin\Foo\Bar\Global; // Warning. namespace Yoast\WP\Plugin\Foo\Case\Bar\Default; // Error. +/* + * Testing an error is thrown for not using a prefix from the validated prefixes array - single valid prefix. + * + * phpcs:set Yoast.NamingConventions.NamespaceName prefixes[] Yoast\WP\Plugin + */ + +namespace Yoast\WP\Plugin\Foo; // OK. +namespace Yoast\WP\Foo; // Error (+ warning for level depth as prefix not correct). +namespace Yoast\Foo; // Error. +namespace Foo\Bar; // Error. + +/* + * Testing an error is thrown for not using a prefix from the validated prefixes array - multiple valid prefixes. + * + * phpcs:set Yoast.NamingConventions.NamespaceName prefixes[] Yoast\WP\Plugin\SubA,Yoast\WP\Plugin\SubB,Yoast\WP\Plugin + */ + +namespace Yoast\WP\Plugin\SubA\Foo; // OK. +namespace Yoast\WP\Plugin\SubB; // OK. +namespace Yoast\WP\Plugin\Foo; // OK. +namespace Yoast\WP\Foo; // Error (+ warning for level depth as prefix not correct). +namespace Yoast\Foo; // Error. +namespace Foo\Bar; // Error. + // Reset to default settings. // phpcs:set Yoast.NamingConventions.NamespaceName prefixes[] diff --git a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/path-translation-root.inc b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/path-translation-root.inc index f40e91e2..ad95da90 100644 --- a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/path-translation-root.inc +++ b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/path-translation-root.inc @@ -10,8 +10,8 @@ namespace Yoast\WP\Plugin; // OK. namespace Yoast\WP\Plugin\Foo; // Error. -// Make sure an error is thrown when the wrong prefix is used. -namespace Yoast_Plugin; // Error. +// Make sure an error is thrown when the wrong type of prefix (non-namespace-like) is used. +namespace Yoast_Plugin; // Error for wrong path-to-namespace + error for not using valid prefix. // Reset to default settings. // phpcs:set Yoast.NamingConventions.NamespaceName prefixes[] From cb6e11a372be0c8b666598f629b85aa8944754a9 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 17 Nov 2023 03:53:36 +0100 Subject: [PATCH 10/14] NamingConventions/NamespaceName: make the "incorrect namespace name" error more informative When no prefix was matched, the error would previously read `Expected: "[Plugin\Prefix]..."; Found: "..."`. However, if there is only one known valid prefix, we can already replace the `[Plugin\Prefix]` with the expected prefix. Fixed now. The effect of this fix can be seen in the messages thrown by the unit tests, but can't be safeguarded via the tests at this time. --- Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php b/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php index 4c155787..3be94fb3 100644 --- a/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php +++ b/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php @@ -269,6 +269,9 @@ public function process( File $phpcsFile, $stackPtr ) { if ( $found_prefix !== '' ) { $expected = $found_prefix; } + elseif ( \count( $this->validated_prefixes ) === 1 ) { + $expected = \rtrim( $this->validated_prefixes[0], '\\' ); + } if ( $relative_directory !== '' ) { $levels = \explode( '/', $relative_directory ); From 342e7c03ced8337a93a1f3283997d5199bcb04e1 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 2 Nov 2023 14:58:50 +0100 Subject: [PATCH 11/14] NamingConventions/NamespaceName: allow for more variation is test dir layouts As the test directory layout for the plugins will change from: ``` - .\ - integration-tests\ - doubles\ - tests\ - doubles\ ``` ... to .... ``` - .\ - tests\ - unit\ - doubles\ - wp\ - doubles\ ``` .. the sniff needs to widen its allowance for these extra levels in the namespace name. This commit implements this additional allowance. While the current solution should possibly be made more code base agnostic via a custom property, for now, this solution will do. Includes additional tests. --- .../NamespaceNameStandard.xml | 2 +- .../NamingConventions/NamespaceNameSniff.php | 42 +++++++++++++++---- .../NamespaceNameUnitTest.php | 9 +++- .../NamespaceNameUnitTests/no-basepath.inc | 26 +++++++++++- 4 files changed, 67 insertions(+), 12 deletions(-) diff --git a/Yoast/Docs/NamingConventions/NamespaceNameStandard.xml b/Yoast/Docs/NamingConventions/NamespaceNameStandard.xml index 5dbf901d..c79ca4f4 100644 --- a/Yoast/Docs/NamingConventions/NamespaceNameStandard.xml +++ b/Yoast/Docs/NamingConventions/NamespaceNameStandard.xml @@ -28,7 +28,7 @@ namespace Admin\Forms; It is recommended for the name to be two levels. - For unit test files, `Tests\(Doubles\)` will be regarded as part of the prefix for this check. + For unit test files, `Tests\(Unit|WP)\(Doubles\)` will be regarded as part of the prefix for this check. ]]> diff --git a/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php b/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php index 3be94fb3..1549ad34 100644 --- a/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php +++ b/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php @@ -29,6 +29,17 @@ final class NamespaceNameSniff implements Sniff { use CustomPrefixesTrait; + /** + * Double/Mock/Fixture directories to allow for. + * + * @var array Key is the subdirectory name, value the length of that name. + */ + private const DOUBLE_DIRS = [ + '\Doubles\\' => 9, + '\Mocks\\' => 7, + '\Fixtures\\' => 10, + ]; + /** * Project root(s). * @@ -161,18 +172,31 @@ public function process( File $phpcsFile, $stackPtr ) { if ( $namespace_name_no_prefix !== '' ) { $namespace_for_level_check = $namespace_name_no_prefix; - // Allow for `Tests\` and `Tests\Doubles\` after the prefix. + // Allow for a variation of `Tests\` and `Tests\*\Doubles\` after the prefix. $starts_with_tests = ( \strpos( $namespace_for_level_check, 'Tests\\' ) === 0 ); if ( $starts_with_tests === true ) { - $namespace_for_level_check = \substr( $namespace_for_level_check, 6 ); - } + $stripped = false; + foreach ( self::DOUBLE_DIRS as $dir => $length ) { + if ( \strpos( $namespace_for_level_check, $dir ) !== false ) { + $namespace_for_level_check = \substr( $namespace_for_level_check, ( \strpos( $namespace_for_level_check, $dir ) + $length ) ); + $stripped = true; + break; + } + } - if ( ( $starts_with_tests === true - // Allow for non-conventional test directory layout, like in YoastSEO Free. - || \strpos( $found_prefix, '\\Tests\\' ) !== false ) - && \strpos( $namespace_for_level_check, 'Doubles\\' ) === 0 - ) { - $namespace_for_level_check = \substr( $namespace_for_level_check, 8 ); + if ( $stripped === false ) { + // No double dir found, now check/strip typical test dirs. + if ( \strpos( $namespace_for_level_check, 'Tests\WP\\' ) === 0 ) { + $namespace_for_level_check = \substr( $namespace_for_level_check, 9 ); + } + elseif ( \strpos( $namespace_for_level_check, 'Tests\Unit\\' ) === 0 ) { + $namespace_for_level_check = \substr( $namespace_for_level_check, 11 ); + } + else { + // Okay, so this only has the `Tests` prefix, just strip it. + $namespace_for_level_check = \substr( $namespace_for_level_check, 6 ); + } + } } $parts = \explode( '\\', $namespace_for_level_check ); diff --git a/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.php b/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.php index e69f4aa0..bcad8ac4 100644 --- a/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.php +++ b/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.php @@ -190,10 +190,17 @@ public function getWarningList( string $testFile = '' ): array { 65 => 1, 69 => 1, 72 => 1, - 73 => 1, 80 => 1, 90 => 1, 103 => 1, + 122 => 1, + 123 => 1, + 124 => 1, + 125 => 1, + 126 => 1, + 127 => 1, + 128 => 1, + 129 => 1, ]; case 'no-basepath-scoped.inc': diff --git a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/no-basepath.inc b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/no-basepath.inc index 449f6b6b..abe5e0df 100644 --- a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/no-basepath.inc +++ b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/no-basepath.inc @@ -70,7 +70,7 @@ namespace Yoast\WP\Plugin\Tests\Doubles\Foo\Bar\Baz; // Warning. namespace Yoast\WP\Plugin\Tests\Doubles\Foo\Bar\Baz\Fro; // Error. namespace Yoast\WP\Plugin\Foo\Tests\Bar; // Warning, `Tests\` counted as not directly after the prefix. -namespace Yoast\WP\Plugin\Tests\Foo\Doubles\Bar; // Warning, `Doubles\` counted as not directly after the prefix + `Tests\`. +namespace Yoast\WP\Plugin\Tests\Foo\Doubles\Bar; // OK. namespace Yoast\WP\Plugin\Doubles\Foo\Bar\Fro; // Error, `Doubles\` counted as not found in combination with `Tests\`. /* @@ -104,6 +104,30 @@ namespace Yoast\WP\Foo; // Error (+ warning for level depth as prefix not correc namespace Yoast\Foo; // Error. namespace Foo\Bar; // Error. +/* + * Test allowing for more variations of test directories and deeper double directories. + * + * phpcs:set Yoast.NamingConventions.NamespaceName prefixes[] Yoast\WP\Plugin + */ + +namespace Yoast\WP\Plugin\Tests\WP\Foo\Bar; // OK. +namespace Yoast\WP\Plugin\Tests\Unit\Foo\Bar; // OK. +namespace Yoast\WP\Plugin\Tests\WP\Doubles\Foo\Bar; // OK. +namespace Yoast\WP\Plugin\Tests\Unit\Doubles\Foo\Bar; // OK. +namespace Yoast\WP\Plugin\Tests\WP\Mocks\Foo\Bar; // OK. +namespace Yoast\WP\Plugin\Tests\Unit\Mocks\Foo\Bar; // OK. +namespace Yoast\WP\Plugin\Tests\WP\Fixtures\Foo\Bar; // OK. +namespace Yoast\WP\Plugin\Tests\Unit\Fixtures\Foo\Bar; // OK. + +namespace Yoast\WP\Plugin\Tests\WP\Foo\Bar\Baz; // Warning. +namespace Yoast\WP\Plugin\Tests\Unit\Foo\Bar\Baz; // Warning. +namespace Yoast\WP\Plugin\Tests\WP\Doubles\Foo\Bar\Baz; // Warning. +namespace Yoast\WP\Plugin\Tests\Unit\Doubles\Foo\Bar\Baz; // Warning. +namespace Yoast\WP\Plugin\Tests\WP\Mocks\Foo\Bar\Baz; // Warning. +namespace Yoast\WP\Plugin\Tests\Unit\Mocks\Foo\Bar\Baz; // Warning. +namespace Yoast\WP\Plugin\Tests\WP\Fixtures\Foo\Bar\Baz; // Warning. +namespace Yoast\WP\Plugin\Tests\Unit\Fixtures\Foo\Bar\Baz; // Warning. + // Reset to default settings. // phpcs:set Yoast.NamingConventions.NamespaceName prefixes[] From b2dcb03f9c51a42f76b9cede1b524448385c8671 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 1 Nov 2023 17:05:29 +0100 Subject: [PATCH 12/14] NamingConventions/NamespaceName: improve handling of unconventional directory names As things were, the `NamespaceName` sniff would translate the namespace to the expected directory path and then compare the directory paths. This is the wrong way round as this sniff is about namespace names, not directory names, so the directory name should be translated to a namespace name and then compared with the actual namespace name instead. Along the same lines, there was the possibility that the sniff would recommend an "Expected" namespace name which would be a parse error/invalid syntax in PHP. This commit fixes both these issues. The commit also adds an extra error for directory names which can't (for a certain definition of "can't") be translated to a valid namespace name. Includes additional unit tests demonstrating the problem. Note: if the name in use in the file is causing a problematic parse error (like in the test with the `#` in the namespace name), the sniff will stay silent. This is the normal behaviour for PHPCS check when encountering parse errors. --- .../NamespaceNameStandard.xml | 2 + .../NamingConventions/NamespaceNameSniff.php | 48 +++++++++++++------ .../NamespaceNameUnitTest.php | 13 +++++ .../sub path/path-translation-sub2-space.inc | 12 +++++ .../sub#path/path-translation-sub2-hash.inc | 15 ++++++ .../path/sub-path/path-translation-sub2.inc | 2 +- .../sub.path/path-translation-sub2-dot.inc | 15 ++++++ .../path-translation-sub2-underscore.inc | 15 ++++++ 8 files changed, 107 insertions(+), 15 deletions(-) create mode 100644 Yoast/Tests/NamingConventions/NamespaceNameUnitTests/path/sub path/path-translation-sub2-space.inc create mode 100644 Yoast/Tests/NamingConventions/NamespaceNameUnitTests/path/sub#path/path-translation-sub2-hash.inc create mode 100644 Yoast/Tests/NamingConventions/NamespaceNameUnitTests/path/sub.path/path-translation-sub2-dot.inc create mode 100644 Yoast/Tests/NamingConventions/NamespaceNameUnitTests/path/sub_path/path-translation-sub2-underscore.inc diff --git a/Yoast/Docs/NamingConventions/NamespaceNameStandard.xml b/Yoast/Docs/NamingConventions/NamespaceNameStandard.xml index c79ca4f4..50d595bc 100644 --- a/Yoast/Docs/NamingConventions/NamespaceNameStandard.xml +++ b/Yoast/Docs/NamingConventions/NamespaceNameStandard.xml @@ -51,6 +51,8 @@ namespace Yoast\WP\Plugin\Tests\Foo\Bar\Flo\Sub; diff --git a/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php b/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php index 1549ad34..ce4bfeb0 100644 --- a/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php +++ b/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php @@ -6,6 +6,7 @@ use PHP_CodeSniffer\Sniffs\Sniff; use PHP_CodeSniffer\Util\Common; use PHPCSUtils\Utils\Namespaces; +use PHPCSUtils\Utils\NamingConventions; use PHPCSUtils\Utils\TextStrings; use YoastCS\Yoast\Utils\CustomPrefixesTrait; @@ -279,16 +280,6 @@ public function process( File $phpcsFile, $stackPtr ) { // Now any potential src directory has been stripped, remove the slashes again. $relative_directory = \trim( $relative_directory, '/' ); - $namespace_name_for_translation = \str_replace( - [ '_', '\\' ], // Find. - [ '-', '/' ], // Replace with. - $namespace_name_no_prefix - ); - - if ( \strcasecmp( $relative_directory, $namespace_name_for_translation ) === 0 ) { - return; - } - $expected = '[Plugin\Prefix]'; if ( $found_prefix !== '' ) { $expected = $found_prefix; @@ -297,14 +288,43 @@ public function process( File $phpcsFile, $stackPtr ) { $expected = \rtrim( $this->validated_prefixes[0], '\\' ); } + $clean = []; + $name_for_compare = ''; + if ( $relative_directory !== '' ) { $levels = \explode( '/', $relative_directory ); - $levels = \array_filter( $levels ); // Remove empties. + $levels = \array_filter( $levels ); // Remove empties, just in case. + foreach ( $levels as $level ) { - $words = \explode( '-', $level ); - $words = \array_map( 'ucfirst', $words ); - $expected .= '\\' . \implode( '_', $words ); + $cleaned_level = \preg_replace( '`[[:punct:]]`', '_', $level ); + $words = \explode( '_', $cleaned_level ); + $words = \array_map( 'ucfirst', $words ); + $cleaned_level = \implode( '_', $words ); + + if ( NamingConventions::isValidIdentifierName( $cleaned_level ) === false ) { + $phpcsFile->addError( + 'Translating the directory name to a namespace name would not yield a valid namespace name. Rename the "%s" directory.', + 0, + 'DirectoryInvalid', + [ $level ] + ); + + // Continuing would be useless as the name would be invalid anyway. + return; + } + + $clean[] = $cleaned_level; } + + $name_for_compare = \implode( '\\', $clean ); + } + + if ( \strcasecmp( $name_for_compare, $namespace_name_no_prefix ) === 0 ) { + return; + } + + if ( $name_for_compare !== '' ) { + $expected .= '\\' . $name_for_compare; } $phpcsFile->addError( diff --git a/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.php b/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.php index bcad8ac4..453d10ea 100644 --- a/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.php +++ b/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.php @@ -115,6 +115,19 @@ public function getErrorList( string $testFile = '' ): array { 12 => 1, 13 => 1, 14 => 1, + 15 => 1, + ]; + + // Path translation with unconventional chars in directory name. + case 'path-translation-sub2-dot.inc': + case 'path-translation-sub2-underscore.inc': + return [ + 12 => 1, + ]; + + case 'path-translation-sub2-space.inc': + return [ + 1 => 1, // Invalid dir error. ]; // Path translation with $src_directory set tests. diff --git a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/path/sub path/path-translation-sub2-space.inc b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/path/sub path/path-translation-sub2-space.inc new file mode 100644 index 00000000..bc8dac55 --- /dev/null +++ b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/path/sub path/path-translation-sub2-space.inc @@ -0,0 +1,12 @@ + Date: Thu, 19 Oct 2023 05:43:14 +0200 Subject: [PATCH 13/14] NamingConventions/NamespaceName: implement use of new PathHelper class --- .../NamingConventions/NamespaceNameSniff.php | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php b/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php index ce4bfeb0..908160bc 100644 --- a/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php +++ b/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php @@ -9,6 +9,7 @@ use PHPCSUtils\Utils\NamingConventions; use PHPCSUtils\Utils\TextStrings; use YoastCS\Yoast\Utils\CustomPrefixesTrait; +use YoastCS\Yoast\Utils\PathHelper; /** * Check namespace name declarations. @@ -236,7 +237,7 @@ public function process( File $phpcsFile, $stackPtr ) { return; } - $base_path = $this->normalize_directory_separators( $phpcsFile->config->basepath ); + $base_path = trim( PathHelper::normalize_directory_separators( $phpcsFile->config->basepath ), '//' ); // Stripping potential quotes to ensure `stdin_path` passed by IDEs does not include quotes. $file = TextStrings::stripQuotes( $phpcsFile->getFileName() ); @@ -245,7 +246,7 @@ public function process( File $phpcsFile, $stackPtr ) { return; // @codeCoverageIgnore } - $directory = $this->normalize_directory_separators( \dirname( $file ) ); + $directory = trim( PathHelper::normalize_directory_separators( \dirname( $file ) ), '//' ); $relative_directory = Common::stripBasepath( $directory, $base_path ); if ( $relative_directory === '.' ) { $relative_directory = ''; @@ -366,7 +367,7 @@ private function validate_src_directory() { continue; } - $directory = $this->normalize_directory_separators( $directory ); + $directory = trim( PathHelper::normalize_directory_separators( $directory ), '//' ); if ( $directory === '.' ) { // The basepath/root directory is the default, so ignore. @@ -390,15 +391,4 @@ private function validate_src_directory() { // Set the validated prefixes cache. $this->validated_src_directory = $validated; } - - /** - * Normalize all directory separators to be a forward slash and remove prefixed and suffixed slashes. - * - * @param string $path Path to normalize. - * - * @return string - */ - private function normalize_directory_separators( $path ) { - return \trim( \strtr( $path, '\\', '/' ), '/' ); - } } From d2837d18ce2390bb9c3f3082fb3a7c3e45cb3674 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 19 Oct 2023 09:20:06 +0200 Subject: [PATCH 14/14] NamingConventions/NamespaceName: improve path handling This changes the `src_directory` property handling. Originally, the file paths would always be stripped down to a relative path in relation to the `basepath`. Now, the `src_directory` path comparison(s) will always be based on the absolute path, including the `basepath`. This should have a small performance benefit, in that some of the path manipulation is moved to the `validate_src_directory()` method, the logic of which under normal circumstances, will only be run once, while the `process()` method is run for every `namespace` keyword encountered. This also implements the use of some additional functions from the `PathHelper` and the `PathValidationHelper` classes. Includes updating a pre-existing test to pass duplicate excluded files in different ways. --- .../NamingConventions/NamespaceNameSniff.php | 98 ++++++------------- .../src/sub-b/path-translation-src-sub-b.inc | 2 +- 2 files changed, 33 insertions(+), 67 deletions(-) diff --git a/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php b/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php index 908160bc..e7373646 100644 --- a/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php +++ b/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php @@ -4,12 +4,12 @@ use PHP_CodeSniffer\Files\File; use PHP_CodeSniffer\Sniffs\Sniff; -use PHP_CodeSniffer\Util\Common; use PHPCSUtils\Utils\Namespaces; use PHPCSUtils\Utils\NamingConventions; use PHPCSUtils\Utils\TextStrings; use YoastCS\Yoast\Utils\CustomPrefixesTrait; use YoastCS\Yoast\Utils\PathHelper; +use YoastCS\Yoast\Utils\PathValidationHelper; /** * Check namespace name declarations. @@ -81,14 +81,14 @@ final class NamespaceNameSniff implements Sniff { /** * Project roots after validation. * - * Validated src_directories will look like `src/`, i.e.: - * - have linux slashes; - * - not be prefixed with a slash; - * - have a trailing slash. + * Validated src_directories will look like "$basepath/src/", i.e.: + * - absolute paths; + * - with linux slashes; + * - and a trailing slash. * * @var array */ - private $validated_src_directory = []; + private $validated_src_directory; /** * Cache of previously set project roots. @@ -97,7 +97,7 @@ final class NamespaceNameSniff implements Sniff { * * @var array */ - private $previous_src_directory = []; + private $previous_src_directory; /** * Returns an array of tokens this test wants to listen for. @@ -237,48 +237,32 @@ public function process( File $phpcsFile, $stackPtr ) { return; } - $base_path = trim( PathHelper::normalize_directory_separators( $phpcsFile->config->basepath ), '//' ); - // Stripping potential quotes to ensure `stdin_path` passed by IDEs does not include quotes. $file = TextStrings::stripQuotes( $phpcsFile->getFileName() ); - if ( $file === 'STDIN' ) { return; // @codeCoverageIgnore } - $directory = trim( PathHelper::normalize_directory_separators( \dirname( $file ) ), '//' ); - $relative_directory = Common::stripBasepath( $directory, $base_path ); - if ( $relative_directory === '.' ) { - $relative_directory = ''; - } - else { - if ( $relative_directory[0] !== '/' ) { - /* - * Basepath stripping appears to work differently depending on OS. - * On Windows there still is a slash at the start, on Unix/Mac there isn't. - * Normalize to allow comparison. - */ - $relative_directory = '/' . $relative_directory; - } - - // Add trailing slash to prevent matching '/sub' to '/sub-directory'. - $relative_directory .= '/'; - } + $directory = PathHelper::normalize_absolute_path( \dirname( $file ) ); + $relative_directory = ''; - $this->validate_src_directory(); + $this->validate_src_directory( $phpcsFile ); if ( empty( $this->validated_src_directory ) === false ) { - foreach ( $this->validated_src_directory as $subdirectory ) { - if ( \strpos( $relative_directory, $subdirectory ) !== 0 ) { + foreach ( $this->validated_src_directory as $absolute_src_path ) { + if ( PathHelper::path_starts_with( $directory, $absolute_src_path ) === false ) { continue; } - $relative_directory = \substr( $relative_directory, \strlen( $subdirectory ) ); + $relative_directory = PathHelper::strip_basepath( $directory, $absolute_src_path ); + if ( $relative_directory === '.' ) { + $relative_directory = ''; + } break; } } - // Now any potential src directory has been stripped, remove the slashes again. + // Now any potential src directory has been stripped, remove surrounding slashes. $relative_directory = \trim( $relative_directory, '/' ); $expected = '[Plugin\Prefix]'; @@ -342,9 +326,11 @@ public function process( File $phpcsFile, $stackPtr ) { /** * Validate a $src_directory property when set in a custom ruleset. * + * @param File $phpcsFile The file being scanned. + * * @return void */ - private function validate_src_directory() { + private function validate_src_directory( File $phpcsFile ) { if ( $this->previous_src_directory === $this->src_directory ) { return; } @@ -352,43 +338,23 @@ private function validate_src_directory() { // Set the cache *before* validation so as to not break the above compare. $this->previous_src_directory = $this->src_directory; - $src_directory = (array) $this->src_directory; - $src_directory = \array_filter( \array_map( 'trim', $src_directory ) ); + // Clear out previously validated src directories. + $this->validated_src_directory = []; - if ( empty( $src_directory ) ) { - $this->validated_src_directory = []; - return; - } + // Note: the check whether a basepath is available is done in the main `process()` routine. + $base_path = PathHelper::normalize_absolute_path( $phpcsFile->config->basepath ); - $validated = []; - foreach ( $src_directory as $directory ) { - if ( \strpos( $directory, '..' ) !== false ) { - // Do not allow walking up the directory hierarchy. - continue; - } - - $directory = trim( PathHelper::normalize_directory_separators( $directory ), '//' ); - - if ( $directory === '.' ) { - // The basepath/root directory is the default, so ignore. - continue; - } - - if ( \strpos( $directory, './' ) === 0 ) { - $directory = \substr( $directory, 2 ); - } - - if ( $directory === '' ) { - continue; - } + // Add any src directories. + $absolute_paths = PathValidationHelper::relative_to_absolute( $phpcsFile, $this->src_directory ); - $validated[] = '/' . $directory . '/'; + // The base path is always a valid src directory. + if ( isset( $absolute_paths['.'] ) === false ) { + $absolute_paths['.'] = $base_path; } - // Use reverse natural sorting to get the longest directory first. - \rsort( $validated, ( \SORT_NATURAL | \SORT_FLAG_CASE ) ); + $this->validated_src_directory = \array_unique( $absolute_paths ); - // Set the validated prefixes cache. - $this->validated_src_directory = $validated; + // Use reverse natural sorting to get the longest directory first. + \rsort( $this->validated_src_directory, ( \SORT_NATURAL | \SORT_FLAG_CASE ) ); } } diff --git a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/src/sub-b/path-translation-src-sub-b.inc b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/src/sub-b/path-translation-src-sub-b.inc index 7d85044f..91f698b3 100644 --- a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/src/sub-b/path-translation-src-sub-b.inc +++ b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/src/sub-b/path-translation-src-sub-b.inc @@ -6,7 +6,7 @@ * Includes testing of matching the longest directory first. * * @phpcs:set Yoast.NamingConventions.NamespaceName prefixes[] Yoast\WP\Plugin - * @phpcs:set Yoast.NamingConventions.NamespaceName src_directory[] ./src , ./src/sub-b + * @phpcs:set Yoast.NamingConventions.NamespaceName src_directory[] ./src , ./src/sub-b,src/sub-b,/src */ namespace Yoast\WP\Plugin; // OK.