From 480238a05d4aa80e77959f95fdc45d4809ccf909 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 13 Nov 2020 16:53:42 +0100 Subject: [PATCH 01/37] AlternativeFunctions: add missing test `@covers` tag --- Yoast/Tests/Yoast/AlternativeFunctionsUnitTest.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Yoast/Tests/Yoast/AlternativeFunctionsUnitTest.php b/Yoast/Tests/Yoast/AlternativeFunctionsUnitTest.php index 78c4b149..702eade0 100644 --- a/Yoast/Tests/Yoast/AlternativeFunctionsUnitTest.php +++ b/Yoast/Tests/Yoast/AlternativeFunctionsUnitTest.php @@ -10,6 +10,8 @@ * @package Yoast\YoastCS * * @since 1.3.0 + * + * @covers YoastCS\Yoast\Sniffs\Yoast\AlternativeFunctionsSniff */ class AlternativeFunctionsUnitTest extends AbstractSniffUnitTest { From efa10bcbe3622decc7d2f9de70c88326ae8ed111 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 13 Nov 2020 16:56:48 +0100 Subject: [PATCH 02/37] CodeCoverageIgnoreDeprecated: prevent the sniff from being ignored Even though the `@CodeCoverageIgnore` annotation was not used as a stand-alone tag, PHPUnit was still ignoring the sniff for recording code coverage. Fixed by removing the `@`s. --- Yoast/Sniffs/Commenting/CodeCoverageIgnoreDeprecatedSniff.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Yoast/Sniffs/Commenting/CodeCoverageIgnoreDeprecatedSniff.php b/Yoast/Sniffs/Commenting/CodeCoverageIgnoreDeprecatedSniff.php index 86e78db1..289098ed 100644 --- a/Yoast/Sniffs/Commenting/CodeCoverageIgnoreDeprecatedSniff.php +++ b/Yoast/Sniffs/Commenting/CodeCoverageIgnoreDeprecatedSniff.php @@ -7,7 +7,7 @@ use PHP_CodeSniffer\Util\Tokens; /** - * Verifies functions which are marked as `@deprecated` have a `@codeCoverageIgnore` tag + * Verifies functions which are marked as `deprecated` have a `codeCoverageIgnore` tag * in their docblock. * * @package Yoast\YoastCS From 3991eb3b3e4f8ec735ceb55c9978337c807e123c Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 13 Nov 2020 17:00:45 +0100 Subject: [PATCH 03/37] PHPUnit: add code coverage configuration --- phpunit.xml.dist | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/phpunit.xml.dist b/phpunit.xml.dist index f8172fb7..a3416a92 100644 --- a/phpunit.xml.dist +++ b/phpunit.xml.dist @@ -4,7 +4,21 @@ xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/6.3/phpunit.xsd" backupGlobals="true" beStrictAboutTestsThatDoNotTestAnything="false" - colors="true"> + colors="true" + forceCoversAnnotation="true" + > + + + + ./Yoast/Tests/ + + + + + + Yoast/Sniffs/ + + From a2738a62ccdff9e9830f65047d2a340ee9ea4b33 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 4 Dec 2020 09:45:33 +0100 Subject: [PATCH 04/37] Travis: add build against PHP 8.0 PHP 8.0 has been branched off two months ago, so `nightly` is now PHP 8.1 and in the mean time PHP 8.0 was released last week. As of today, there is a PHP 8.0 image available on Travis. This PR adds two new builds against PHP 8.0 to the matrix and, as PHP 8.0 has been released, these builds are not allowed to fail. --- .travis.yml | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 02fb4cf9..6d763cd7 100644 --- a/.travis.yml +++ b/.travis.yml @@ -84,7 +84,12 @@ jobs: #### TEST STAGE #### # Additional builds against PHP versions which need a different distro. - stage: test - php: 5.5 + php: 8.0 + env: PHPCS_BRANCH="dev-master" WPCS="dev-master" PHPLINT=1 + - php: 8.0 + # PHPCS is only compatible with PHP 8.0 as of version 3.5.7. + env: PHPCS_BRANCH="3.5.7" WPCS="2.2.0" + - php: 5.5 dist: trusty env: PHPCS_BRANCH="dev-master" WPCS="dev-master" PHPLINT=1 - php: 5.5 @@ -150,7 +155,7 @@ before_install: fi - | - if [[ $TRAVIS_PHP_VERSION == "nightly" ]]; then + if [[ $TRAVIS_PHP_VERSION == "nightly" || $TRAVIS_PHP_VERSION == "8.0" ]]; then travis_retry composer install --prefer-dist --no-interaction --ignore-platform-reqs else travis_retry composer install --prefer-dist --no-interaction From ff1e2c4ae5bcbb1d5d0b5f2bba204b5a1b40860b Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 4 Mar 2021 19:43:49 +0100 Subject: [PATCH 05/37] Files/FileName: allow for classes which are named the same as a prefix When a class would be called `Yoast` and `yoast` would be one of the "forbidden" prefixes, the error message would recommend for the file to be called ".php". This fix prevents that from happening and will leave classes which are called _exactly_ the same as a prefix alone. Includes unit test. Includes minor tweak for error message readability. --- Yoast/Sniffs/Files/FileNameSniff.php | 8 ++++---- Yoast/Tests/Files/FileNameUnitTest.php | 1 + Yoast/Tests/Files/FileNameUnitTests/classes/yoast.inc | 8 ++++++++ 3 files changed, 13 insertions(+), 4 deletions(-) create mode 100644 Yoast/Tests/Files/FileNameUnitTests/classes/yoast.inc diff --git a/Yoast/Sniffs/Files/FileNameSniff.php b/Yoast/Sniffs/Files/FileNameSniff.php index 816e56fd..8fa26823 100644 --- a/Yoast/Sniffs/Files/FileNameSniff.php +++ b/Yoast/Sniffs/Files/FileNameSniff.php @@ -147,7 +147,7 @@ public function process( File $phpcsFile, $stackPtr ) { 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. Expected "%s", but found "%s".'; $error_code = 'InvalidInterfaceFileName'; // Don't duplicate "interface" in the filename. @@ -157,7 +157,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. Expected "%s", but found "%s".'; $error_code = 'InvalidTraitFileName'; // Don't duplicate "trait" in the filename. @@ -170,7 +170,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. Expected "%s", but found "%s".'; $error_code = 'InvalidFunctionsFileName'; if ( \substr( $expected, -10 ) !== '-functions' ) { @@ -181,7 +181,7 @@ public function process( File $phpcsFile, $stackPtr ) { } // Throw the error. - if ( $file_name !== $expected ) { + if ( $expected !== '' && $file_name !== $expected ) { $phpcsFile->addError( $error, 0, diff --git a/Yoast/Tests/Files/FileNameUnitTest.php b/Yoast/Tests/Files/FileNameUnitTest.php index 2de8fd24..c6165daf 100644 --- a/Yoast/Tests/Files/FileNameUnitTest.php +++ b/Yoast/Tests/Files/FileNameUnitTest.php @@ -45,6 +45,7 @@ class FileNameUnitTest extends AbstractSniffUnitTest { 'some-class.inc' => 0, 'wpseo-some-class.inc' => 1, // Prefix 'wpseo' not necessary. 'yoast-plugin-some-class.inc' => 1, // Prefix 'yoast-plugin' not necessary. + '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. diff --git a/Yoast/Tests/Files/FileNameUnitTests/classes/yoast.inc b/Yoast/Tests/Files/FileNameUnitTests/classes/yoast.inc new file mode 100644 index 00000000..375a8fb2 --- /dev/null +++ b/Yoast/Tests/Files/FileNameUnitTests/classes/yoast.inc @@ -0,0 +1,8 @@ + +phpcs:set Yoast.Files.FileName oo_prefixes[] yoast + + Date: Wed, 10 Mar 2021 10:33:16 +0100 Subject: [PATCH 06/37] Set minimum_supported_version to WP 5.6 --- Yoast/ruleset.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Yoast/ruleset.xml b/Yoast/ruleset.xml index 48d80d8a..51e25834 100644 --- a/Yoast/ruleset.xml +++ b/Yoast/ruleset.xml @@ -31,7 +31,7 @@ Ref: https://github.com/WordPress/WordPress-Coding-Standards/wiki/Customizable-sniff-properties#minimum-wp-version-to-check-for-usage-of-deprecated-functions-classes-and-function-parameters --> - + From c6a3640ce93c2e516f3cc25b86c4f3cf7d4b3d13 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Tue, 27 Apr 2021 03:04:49 +0200 Subject: [PATCH 07/37] Composer: update runtime dependencies PHP_CodeSniffer has released version 3.6.0 Highlights: * First version which supports all PHP 8.0 syntaxes, though not all utilities and sniffs have been fully updated yet. * Large range of bugfixes. WPCS 2.3.0 was released quite while back and while it as was already allowed via the version constraints and in practice used, let's formalize it as the minimum WPCS requirement. Refs: * https://github.com/squizlabs/PHP_CodeSniffer/releases/tag/3.6.0 * https://github.com/WordPress/WordPress-Coding-Standards/releases/tag/2.3.0 --- composer.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/composer.json b/composer.json index a35d9b5a..9e1e1101 100644 --- a/composer.json +++ b/composer.json @@ -23,8 +23,8 @@ }, "require": { "php": ">=5.4", - "squizlabs/php_codesniffer": "^3.5.0", - "wp-coding-standards/wpcs": "^2.2.0", + "squizlabs/php_codesniffer": "^3.6.0", + "wp-coding-standards/wpcs": "^2.3.0", "phpcompatibility/phpcompatibility-wp": "^2.1.0", "dealerdirect/phpcodesniffer-composer-installer": "^0.5 || ^0.6.2 || ^0.7" }, From ec753edb685a4cfbb8add39b89a8547843a95449 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Tue, 27 Apr 2021 03:07:59 +0200 Subject: [PATCH 08/37] Composer: update dev dependencies Update version constraints for PHPCompatibility and PHP Parallel Lint. Refs: * https://github.com/PHPCompatibility/PHPCompatibility/releases/ * https://github.com/php-parallel-lint/PHP-Parallel-Lint/releases/tag/v1.3.0 --- composer.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/composer.json b/composer.json index 9e1e1101..25f2be0e 100644 --- a/composer.json +++ b/composer.json @@ -29,10 +29,10 @@ "dealerdirect/phpcodesniffer-composer-installer": "^0.5 || ^0.6.2 || ^0.7" }, "require-dev": { - "phpcompatibility/php-compatibility": "^9.2.0", + "phpcompatibility/php-compatibility": "^9.3.5", "roave/security-advisories": "dev-master", "phpunit/phpunit": "^4.0 || ^5.0 || ^6.0 || ^7.0", - "php-parallel-lint/php-parallel-lint": "^1.2", + "php-parallel-lint/php-parallel-lint": "^1.3", "php-parallel-lint/php-console-highlighter": "^0.5", "phpcsstandards/phpcsdevtools": "^1.0" }, From 1d72b4e0ff035bf6aa9a9ff43eb104bb1d1d59be Mon Sep 17 00:00:00 2001 From: jrfnl Date: Tue, 27 Apr 2021 03:27:09 +0200 Subject: [PATCH 09/37] Files/FileName: improve tests Some tests (interfaces/traits) were using the same file names and as the data provider is an array, that meant that array values were being overwritten. This wasn't necessarily problematic as the values for those keys were the same in both cases, so all tests were still being run correctly, but is still something which should be fixed for improved future stability. --- Yoast/Tests/Files/FileNameUnitTest.php | 6 +++--- ...utline-something-trait.inc => outline-mything-trait.inc} | 2 +- .../traits/{outline-something.inc => outline-mything.inc} | 2 +- ...oast-outline-something.inc => yoast-outline-mything.inc} | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) rename Yoast/Tests/Files/FileNameUnitTests/traits/{outline-something-trait.inc => outline-mything-trait.inc} (87%) rename Yoast/Tests/Files/FileNameUnitTests/traits/{outline-something.inc => outline-mything.inc} (87%) rename Yoast/Tests/Files/FileNameUnitTests/traits/{yoast-outline-something.inc => yoast-outline-mything.inc} (87%) diff --git a/Yoast/Tests/Files/FileNameUnitTest.php b/Yoast/Tests/Files/FileNameUnitTest.php index c6165daf..f885e01a 100644 --- a/Yoast/Tests/Files/FileNameUnitTest.php +++ b/Yoast/Tests/Files/FileNameUnitTest.php @@ -58,10 +58,10 @@ class FileNameUnitTest extends AbstractSniffUnitTest { 'excluded-interface-file.inc' => 0, // Trait file names. - 'outline-something-trait.inc' => 0, + 'outline-mything-trait.inc' => 0, 'different-trait.inc' => 1, // Filename not in line with trait name. - 'outline-something.inc' => 1, // Missing '-trait' suffix. - 'yoast-outline-something.inc' => 1, // Prefix 'yoast' not needed. + 'outline-mything.inc' => 1, // Missing '-trait' suffix. + 'yoast-outline-mything.inc' => 1, // Prefix 'yoast' not needed. 'no-duplicate-trait.inc' => 0, // Check that 'Trait' in trait name does not cause duplication in filename. 'excluded-trait-file.inc' => 0, diff --git a/Yoast/Tests/Files/FileNameUnitTests/traits/outline-something-trait.inc b/Yoast/Tests/Files/FileNameUnitTests/traits/outline-mything-trait.inc similarity index 87% rename from Yoast/Tests/Files/FileNameUnitTests/traits/outline-something-trait.inc rename to Yoast/Tests/Files/FileNameUnitTests/traits/outline-mything-trait.inc index 2328951c..8132ecde 100644 --- a/Yoast/Tests/Files/FileNameUnitTests/traits/outline-something-trait.inc +++ b/Yoast/Tests/Files/FileNameUnitTests/traits/outline-mything-trait.inc @@ -3,6 +3,6 @@ phpcs:set Yoast.Files.FileName oo_prefixes[] wpseo,yoast Date: Thu, 27 May 2021 03:16:09 +0200 Subject: [PATCH 10/37] ValidHookName: improve handling of escaped slashes in prefixes This commit is the result of a review of the sniff for correct handling of double slashes in hook names. When the "namespace-like" prefix type is used and the hook name is in double quotes, the "namespace separator" slash should be escaped to prevent it from accidentally escaping the **next** character. To that end: * Slash-escaped backslashes in prefixes will now be properly recognized and handled by the sniff and will not lead to false positives due to unrecognized prefixes. * If the hook name is passed as a double quoted string, an extra check will be executed to verify that all backslashes are slash-escaped. If this is not the case, a warning will be thrown. Includes a minor efficiency fix as the prefix is now only checked and saved to the property earlier instead of the hook name being analyzed twice for this. Includes unit tests for the functionality and some minor adjustments to the existing unit tests. Fixes 242 In a further iteration on this sniff, the new warning could potentially be made auto-fixable. --- .../NamingConventions/ValidHookNameSniff.php | 113 ++++++++++-------- .../ValidHookNameUnitTest.inc | 28 ++++- .../ValidHookNameUnitTest.php | 3 + 3 files changed, 87 insertions(+), 57 deletions(-) diff --git a/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php b/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php index 3cca3676..7f1f5e77 100644 --- a/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php +++ b/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php @@ -60,14 +60,6 @@ class ValidHookNameSniff extends WPCS_ValidHookNameSniff { */ public $recommended_max_words = 4; - /** - * Whether this is the first text string passed to the `transform()` - * method, i.e. the text string which should have the prefix. - * - * @var bool - */ - private $remove_prefix = false; - /** * The prefix found (if any). * @@ -83,6 +75,17 @@ class ValidHookNameSniff extends WPCS_ValidHookNameSniff { */ private $first_string = ''; + /** + * The quote style used for the prefix part of the hook name. + * + * A double quoted string without variable interpolation will be tokenized as + * `T_CONSTANT_ENCAPSED_STRING`, but due to the double quotes, will still need + * for namespace separators to be escaped, i.e. double-slashed. + * + * @var string + */ + private $prefix_quote_style = ''; + /** * Process the parameters of a matched function. * @@ -101,9 +104,9 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p * Reset the properties which help manage this each time a new function call * is encountered. */ - $this->remove_prefix = true; - $this->found_prefix = ''; - $this->first_string = ''; + $this->found_prefix = ''; + $this->first_string = ''; + $this->prefix_quote_style = ''; $this->validate_prefixes(); /* @@ -115,12 +118,22 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p $found_prefix = ''; if ( isset( Tokens::$stringTokens[ $this->tokens[ $first_non_empty ]['code'] ] ) ) { - $content = \trim( $this->strip_quotes( $this->tokens[ $first_non_empty ]['content'] ) ); + $this->prefix_quote_style = $this->tokens[ $first_non_empty ]['content'][0]; + $content = \trim( $this->strip_quotes( $this->tokens[ $first_non_empty ]['content'] ) ); + foreach ( $this->validated_prefixes as $prefix ) { - if ( \strpos( $content, $prefix ) === 0 ) { + if ( \strpos( $prefix, '\\' ) === false + && \strpos( $content, $prefix ) === 0 + ) { $found_prefix = $prefix; break; } + + $prefix = \str_replace( '\\\\', '[\\\\]+', \preg_quote( $prefix, '`' ) ); + if ( \preg_match( '`^' . $prefix . '`', $content, $matches ) === 1 ) { + $found_prefix = $matches[0]; + break; + } } } @@ -132,6 +145,8 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p */ return; } + + $this->found_prefix = $found_prefix; } // Do the WPCS native hook name check. @@ -154,60 +169,28 @@ public function process_parameters( $stackPtr, $group_name, $matched_content, $p * * @param string $string The target string. * @param string $regex The punctuation regular expression to use. - * @param string $transform_type Whether to a partial or complete transform. + * @param string $transform_type Whether to do a partial or complete transform. * Valid values are: 'full', 'case', 'punctuation'. * @return string */ protected function transform( $string, $regex, $transform_type = 'full' ) { if ( empty( $this->validated_prefixes ) ) { - $this->remove_prefix = false; return parent::transform( $string, $regex, $transform_type ); } + if ( $this->first_string === '' ) { + $this->first_string = $string; + } + // Not the first text string. - if ( $this->remove_prefix === false - && $string !== $this->first_string - ) { + if ( $string !== $this->first_string ) { return parent::transform( $string, $regex, $transform_type ); } // Repeated call for the first text string. - if ( $this->remove_prefix === false - && $string === $this->first_string - ) { - if ( $this->found_prefix !== '' ) { - $string = \substr( $string, \strlen( $this->found_prefix ) ); - } - - return $this->found_prefix . parent::transform( $string, $regex, $transform_type ); - } - - // First call for first text string. - if ( $this->remove_prefix === true ) { - $this->first_string = $string; - - foreach ( $this->validated_prefixes as $prefix ) { - if ( \strpos( $string, $prefix ) === 0 ) { - $string = \substr( $string, \strlen( $prefix ) ); - $this->found_prefix = $prefix; - - /* - * Handle case where the prefix is the only content in a single quoted string, - * which would necessitate an extra backslash to escape the end backslash. - * I.e. 'Yoast\WP\Plugin\\'. - */ - if ( $string === '\\' ) { - $string = ''; - $this->found_prefix .= '\\'; - } - - break; - } - } - - // Don't do this again until the next time the sniff gets triggered. - $this->remove_prefix = false; + if ( $this->found_prefix !== '' ) { + $string = \substr( $string, \strlen( $this->found_prefix ) ); } return $this->found_prefix . parent::transform( $string, $regex, $transform_type ); @@ -255,6 +238,30 @@ public function verify_yoast_hook_name( $stackPtr, $parameters ) { } else { $this->phpcsFile->recordMetric( $stackPtr, 'Hook name prefix type', 'new\style' ); + + /* + * Check whether the namespace separator slashes are correctly escaped. + */ + if ( $this->prefix_quote_style === '"' ) { + \preg_match_all( '`[\\\\]+`', $this->found_prefix, $matches ); + if ( empty( $matches ) === false ) { + $counter = 0; + foreach ( $matches[0] as $match ) { + if ( $match === '\\' ) { + ++$counter; + } + } + + if ( $counter > 0 ) { + $this->phpcsFile->addWarning( + 'When using a double quoted string for the hook name, it is strongly recommended to escape the backslashes in the hook name (prefix). Found %s unescaped backslashes.', + $first_non_empty, + 'EscapeSlashes', + [ $counter ] + ); + } + } + } } /* diff --git a/Yoast/Tests/NamingConventions/ValidHookNameUnitTest.inc b/Yoast/Tests/NamingConventions/ValidHookNameUnitTest.inc index c5d0e34e..80882350 100644 --- a/Yoast/Tests/NamingConventions/ValidHookNameUnitTest.inc +++ b/Yoast/Tests/NamingConventions/ValidHookNameUnitTest.inc @@ -45,7 +45,7 @@ do_action( 'Yoast\WP\Plugin\hook_' . $type . 'Yoast\WP\Plugin\hook' ); // Error apply_filters( 'Yoast\WP\Plugin\\' . $type . '_' . $sub, $var ); // Warning at severity 3 compound name. // Test handling of escaped slash in double quotes string. -$var = apply_filters( "Yoast\WP\Plugin\\{$obj->prop['type']}_details", $var ); // Warning at severity 3 compound name. +$var = apply_filters( "Yoast\\WP\\Plugin\\{$obj->prop['type']}_details", $var ); // Warning at severity 3 compound name. // phpcs:enable @@ -69,7 +69,7 @@ do_action( */ // phpcs:set Yoast.NamingConventions.ValidHookName prefixes[] Yoast\WP\Plugin,yoast_plugin -do_action( "Yoast\WP\Plugin\some_{$variable}_hook_name"); // Warning at severity 3. +do_action( "Yoast\\WP\\Plugin\\some_{$variable}_hook_name"); // Warning at severity 3. do_action( 'yoast_plugin_some_' . 'hook_' . 'name' ); // Warning at severity 3 + warning wrong prefix. /* @@ -118,7 +118,27 @@ do_action_ref_array( 'yoast_plugin_some_hook_name_which_is_too_long', $var ); // do_action( 'Yoast\WP\Plugin\some_hook_name', $var ); // Error. -// Reset to default settings. -// phpcs:set Yoast.NamingConventions.ValidHookName prefixes[] +// Reset word maximums. // phpcs:set Yoast.NamingConventions.ValidHookName max_words 4 // phpcs:set Yoast.NamingConventions.ValidHookName recommended_max_words 4 + +/* + * Test handling of double slashes in prefixes. + * - Double slashes in single quoted strings are generally not needed, but will not cause problems. + * - Single slashed in double quoted strings could be problematic, so escaping the slashes is recommended. + */ + +// Correct (single quoted). +apply_filters( 'Yoast\WP\Plugin\hookname', $var); // OK. +apply_filters( 'Yoast\\WP\\Plugin\\hookname', $var); // OK. + +// Correct (double quoted). +apply_filters( "Yoast\\WP\\Plugin\\hook{$name}", $var); // OK, warning at severity 3 for word count. +apply_filters( "Yoast\\WP\\Plugin\\hookname", $var); // OK. + +// Incorrect (double quoted - the `\` needs to be escaped). +apply_filters( "Yoast\WP\Plugin\hookname", $var); // Warning, missing escaping x 3. +apply_filters( "Yoast\WP\\Plugin\hook{$name}", $var); // Warning, missing escaping x 2 + warning at severity 3 for word count. + +// Reset to default settings. +// phpcs:set Yoast.NamingConventions.ValidHookName prefixes[] diff --git a/Yoast/Tests/NamingConventions/ValidHookNameUnitTest.php b/Yoast/Tests/NamingConventions/ValidHookNameUnitTest.php index 1133dba0..b49dd619 100644 --- a/Yoast/Tests/NamingConventions/ValidHookNameUnitTest.php +++ b/Yoast/Tests/NamingConventions/ValidHookNameUnitTest.php @@ -78,6 +78,9 @@ public function getWarningList() { 107 => 1, 110 => 2, 111 => 1, + 136 => 1, // Severity: 3. + 140 => 1, + 141 => 2, // Severity: 3 + 5. ]; } } From d4a41d59075fb32c38ead339627620fb91fdf8b3 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 27 May 2021 04:02:04 +0200 Subject: [PATCH 11/37] Travis: update the matrix Follow up to PR 240 which updated the minimum supported PHPCS and WPCS versions. --- .travis.yml | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/.travis.yml b/.travis.yml index 6d763cd7..6d891d15 100644 --- a/.travis.yml +++ b/.travis.yml @@ -23,9 +23,9 @@ env: # Test against the highest/lowest supported PHPCS and WPCS versions. # Using WPCS 'master' for the last stable release. - PHPCS_BRANCH="dev-master" WPCS="dev-master" PHPLINT=1 - - PHPCS_BRANCH="dev-master" WPCS="2.2.0" - - PHPCS_BRANCH="3.5.0" WPCS="dev-master" - - PHPCS_BRANCH="3.5.0" WPCS="2.2.0" + - PHPCS_BRANCH="dev-master" WPCS="2.3.0" + - PHPCS_BRANCH="3.6.0" WPCS="dev-master" + - PHPCS_BRANCH="3.6.0" WPCS="2.3.0" # Define the stages used. # For non-PRs, only the sniff, ruleset and quicktest stages are run. @@ -73,13 +73,13 @@ jobs: php: 7.4 env: PHPCS_BRANCH="dev-master" WPCS="dev-master" PHPLINT=1 - php: 7.4 - env: PHPCS_BRANCH="3.5.0" WPCS="2.2.0" + env: PHPCS_BRANCH="3.6.0" WPCS="2.3.0" - php: 5.4 dist: trusty - env: PHPCS_BRANCH="dev-master" WPCS="2.2.0" PHPLINT=1 + env: PHPCS_BRANCH="dev-master" WPCS="2.3.0" PHPLINT=1 - php: 5.4 dist: trusty - env: PHPCS_BRANCH="3.5.0" WPCS="dev-master" + env: PHPCS_BRANCH="3.6.0" WPCS="dev-master" #### TEST STAGE #### # Additional builds against PHP versions which need a different distro. @@ -87,32 +87,32 @@ jobs: php: 8.0 env: PHPCS_BRANCH="dev-master" WPCS="dev-master" PHPLINT=1 - php: 8.0 - # PHPCS is only compatible with PHP 8.0 as of version 3.5.7. - env: PHPCS_BRANCH="3.5.7" WPCS="2.2.0" + # PHPCS is only compatible with PHP 8.0 as of version 3.6.0. + env: PHPCS_BRANCH="3.6.0" WPCS="2.3.0" - php: 5.5 dist: trusty env: PHPCS_BRANCH="dev-master" WPCS="dev-master" PHPLINT=1 - php: 5.5 dist: trusty - env: PHPCS_BRANCH="dev-master" WPCS="2.2.0" + env: PHPCS_BRANCH="dev-master" WPCS="2.3.0" - php: 5.5 dist: trusty - env: PHPCS_BRANCH="3.5.0" WPCS="dev-master" + env: PHPCS_BRANCH="3.6.0" WPCS="dev-master" - php: 5.5 dist: trusty - env: PHPCS_BRANCH="3.5.0" WPCS="2.2.0" + env: PHPCS_BRANCH="3.6.0" WPCS="2.3.0" - php: 5.4 dist: trusty env: PHPCS_BRANCH="dev-master" WPCS="dev-master" PHPLINT=1 - php: 5.4 dist: trusty - env: PHPCS_BRANCH="dev-master" WPCS="2.2.0" + env: PHPCS_BRANCH="dev-master" WPCS="2.3.0" - php: 5.4 dist: trusty - env: PHPCS_BRANCH="3.5.0" WPCS="dev-master" + env: PHPCS_BRANCH="3.6.0" WPCS="dev-master" - php: 5.4 dist: trusty - env: PHPCS_BRANCH="3.5.0" WPCS="2.2.0" + env: PHPCS_BRANCH="3.6.0" WPCS="2.3.0" # Test against WPCS unstable. - php: 7.4 From 44d58e9f1828671ae634818fc4401a0ae60b4d2b Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 28 May 2021 19:10:19 +0200 Subject: [PATCH 12/37] Add a custom "Threshold" report for use with PHPCS This commit adds a custom report for use with PHPCS to compare the run results with "threshold" settings. The report will look in the runtime environment for the following two environment variables: * `YOASTCS_THRESHOLD_ERRORS` * `YOASTCS_THRESHOLD_WARNINGS` ... and will take the values of those as the thresholds to compare the PHPCS run results against. If the environment variables are not set, they will default to `0` for both, i.e. no errors or warnings allowed. To use this report, run PHPCS with the following command-line argument: `--report=YoastCS\Yoast\Reports\Threshold`. After the report has run, a global `YOASTCS_ABOVE_THRESHOLD` constant (boolean) will be available which can be used in calling scripts, to, for instance, exit with a different exit code if the thresholds were not matched. Note: the messaging in the report - _"update the threshold in the composer.json"_ - is strongly based on existing practices in the Yoast environment. Includes minor tweaks to the CS rules applied to the YoastCS repo itself as this code is not run within the context of WordPress and therefore doesn't have to (and often can't) comply with WordPress specific rules. --- .phpcs.xml.dist | 4 ++ Yoast/Reports/Threshold.php | 125 ++++++++++++++++++++++++++++++++++++ 2 files changed, 129 insertions(+) create mode 100644 Yoast/Reports/Threshold.php diff --git a/.phpcs.xml.dist b/.phpcs.xml.dist index 405c9482..ea690839 100644 --- a/.phpcs.xml.dist +++ b/.phpcs.xml.dist @@ -50,6 +50,10 @@ + + + + diff --git a/Yoast/Reports/Threshold.php b/Yoast/Reports/Threshold.php new file mode 100644 index 00000000..b360192b --- /dev/null +++ b/Yoast/Reports/Threshold.php @@ -0,0 +1,125 @@ + $error_threshold ) { + $color = "\033[31m"; // Red. + } + echo "{$color}Coding standards ERRORS: $totalErrors/$error_threshold.\033[0m" . \PHP_EOL; + + $color = "\033[32m"; + if ( $totalWarnings > $warning_threshold ) { + $color = "\033[33m"; // Orange. + } + echo "{$color}Coding standards WARNINGS: $totalWarnings/$warning_threshold.\033[0m" . \PHP_EOL; + echo \PHP_EOL; + + $above_threshold = false; + + if ( $totalErrors > $error_threshold ) { + echo "\033[31mPlease fix any errors introduced in your code and run PHPCS again to verify.\033[0m" . \PHP_EOL; + $above_threshold = true; + } + elseif ( $totalErrors < $error_threshold ) { + echo "\033[32mFound less errors than the threshold, great job!\033[0m" . \PHP_EOL; + echo "Please update the ERRORS threshold in the composer.json file to \033[32m$totalErrors\033[0m." . \PHP_EOL; + } + + if ( $totalWarnings > $warning_threshold ) { + echo "\033[33mPlease fix any warnings introduced in your code and run PHPCS again to verify.\033[0m" . \PHP_EOL; + $above_threshold = true; + } + elseif ( $totalWarnings < $warning_threshold ) { + echo "\033[32mFound less warnings than the threshold, great job!\033[0m" . \PHP_EOL; + echo "Please update the WARNINGS threshold in the composer.json file to \033[32m$totalWarnings\033[0m." . \PHP_EOL; + } + + if ( $above_threshold === false ) { + echo \PHP_EOL; + echo 'Coding standards checks have passed!' . \PHP_EOL; + } + + // Make the threshold comparison outcome available to the calling script. + \define( 'YOASTCS_ABOVE_THRESHOLD', $above_threshold ); + } +} From c6a14a58149e770636d1c760138f2d739afe99b7 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 2 Jun 2021 18:04:05 +0200 Subject: [PATCH 13/37] Threshold report: use colour constants This changes the colourization of the text messages to be more descriptive by using class constants for the colour codes. As a lot of the `echo` statements can now no longer use double quoted strings (due to the references to class constants), this also changes the `echo` statements to use comma separated arguments instead of concatenation for creating the output. --- Yoast/Reports/Threshold.php | 65 ++++++++++++++++++++++++++++--------- 1 file changed, 50 insertions(+), 15 deletions(-) diff --git a/Yoast/Reports/Threshold.php b/Yoast/Reports/Threshold.php index b360192b..024359a3 100644 --- a/Yoast/Reports/Threshold.php +++ b/Yoast/Reports/Threshold.php @@ -22,6 +22,41 @@ */ class Threshold implements Report { + /** + * Escape sequence for making text white on the command-line. + * + * @var string + */ + const WHITE = "\033[1m"; + + /** + * Escape sequence for making text red on the command-line. + * + * @var string + */ + const RED = "\033[31m"; + + /** + * Escape sequence for making text green on the command-line. + * + * @var string + */ + const GREEN = "\033[32m"; + + /** + * Escape sequence for making text orange/yellow on the command-line. + * + * @var string + */ + const YELLOW = "\033[33m"; + + /** + * Escape sequence for resetting the text colour. + * + * @var string + */ + const RESET = "\033[0m"; + /** * Generate a partial report for a single processed file. * @@ -78,45 +113,45 @@ public function generate( $error_threshold = (int) \getenv( 'YOASTCS_THRESHOLD_ERRORS' ); $warning_threshold = (int) \getenv( 'YOASTCS_THRESHOLD_WARNINGS' ); - echo \PHP_EOL . "\033[1m" . 'PHP CODE SNIFFER THRESHOLD COMPARISON' . "\033[0m" . \PHP_EOL; - echo \str_repeat( '-', $width ) . \PHP_EOL; + echo \PHP_EOL, self::WHITE, 'PHP CODE SNIFFER THRESHOLD COMPARISON', self::RESET, \PHP_EOL; + echo \str_repeat( '-', $width ), \PHP_EOL; - $color = "\033[32m"; // Green. + $color = self::GREEN; if ( $totalErrors > $error_threshold ) { - $color = "\033[31m"; // Red. + $color = self::RED; } - echo "{$color}Coding standards ERRORS: $totalErrors/$error_threshold.\033[0m" . \PHP_EOL; + echo "{$color}Coding standards ERRORS: $totalErrors/$error_threshold.", self::RESET, \PHP_EOL; - $color = "\033[32m"; + $color = self::GREEN; if ( $totalWarnings > $warning_threshold ) { - $color = "\033[33m"; // Orange. + $color = self::YELLOW; } - echo "{$color}Coding standards WARNINGS: $totalWarnings/$warning_threshold.\033[0m" . \PHP_EOL; + echo "{$color}Coding standards WARNINGS: $totalWarnings/$warning_threshold.", self::RESET, \PHP_EOL; echo \PHP_EOL; $above_threshold = false; if ( $totalErrors > $error_threshold ) { - echo "\033[31mPlease fix any errors introduced in your code and run PHPCS again to verify.\033[0m" . \PHP_EOL; + echo self::RED, 'Please fix any errors introduced in your code and run PHPCS again to verify.', self::RESET, \PHP_EOL; $above_threshold = true; } elseif ( $totalErrors < $error_threshold ) { - echo "\033[32mFound less errors than the threshold, great job!\033[0m" . \PHP_EOL; - echo "Please update the ERRORS threshold in the composer.json file to \033[32m$totalErrors\033[0m." . \PHP_EOL; + echo self::GREEN, 'Found less errors than the threshold, great job!', self::RESET, \PHP_EOL; + echo 'Please update the ERRORS threshold in the composer.json file to ', self::GREEN, $totalErrors, '.', self::RESET, \PHP_EOL; } if ( $totalWarnings > $warning_threshold ) { - echo "\033[33mPlease fix any warnings introduced in your code and run PHPCS again to verify.\033[0m" . \PHP_EOL; + echo self::YELLOW, 'Please fix any warnings introduced in your code and run PHPCS again to verify.', self::RESET, \PHP_EOL; $above_threshold = true; } elseif ( $totalWarnings < $warning_threshold ) { - echo "\033[32mFound less warnings than the threshold, great job!\033[0m" . \PHP_EOL; - echo "Please update the WARNINGS threshold in the composer.json file to \033[32m$totalWarnings\033[0m." . \PHP_EOL; + echo self::GREEN, 'Found less warnings than the threshold, great job!', self::RESET, \PHP_EOL; + echo 'Please update the WARNINGS threshold in the composer.json file to ', self::GREEN, $totalWarnings, '.', self::RESET, \PHP_EOL; } if ( $above_threshold === false ) { echo \PHP_EOL; - echo 'Coding standards checks have passed!' . \PHP_EOL; + echo 'Coding standards checks have passed!', \PHP_EOL; } // Make the threshold comparison outcome available to the calling script. From a0f2a2ea52c9a59c0fa8bbea8e260c3d2965aad6 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 1 Dec 2019 18:19:19 +0100 Subject: [PATCH 14/37] Ruleset: enforce consistent placement of boolean operators in multi-line control structure conditions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a control structure has multiple conditions, it is often formatted over multiple lines. ```php if ( isset( $foo, $bar ) && $foo !== $bar ) {} ``` The boolean operators in such a multi-line set of conditions can be freely placed. I.e. the below is perfectly valid PHP, but not great for readability: ```php if ( isset( $foo, $bar ) && $foo !== $bar ) {} ``` The sniff addition proposed in this PR enforces that the boolean operators between conditions in multi-line control structures are consistently at the start òr at the end of the line, but not a mix of both. I.e. this would be valid: ```php if ( isset( $foo, $bar ) && $foo !== $bar && $foo > $baz ) {} ``` ... while this wouldn't be: ```php if ( isset( $foo, $bar ) && $foo !== $bar && $foo > $baz ) {} ``` Multiple conditions on the same line will still be allowed: ```php if ( isset( $foo, $bar ) && $foo !== $bar ) {} ``` In addition to that, the PHPCS 3.5.4 property `$allowOnly` gives us the change to enforce that the operators are always at the start or at the end of the line. I'm proposing for only allowing them at the start of the line as that will generally make for a smaller diff if/when a new condition is added to the control structure or when a condition is removed, which makes for easier code reviews. Refs: * https://github.com/squizlabs/PHP_CodeSniffer/releases/tag/3.5.4 * https://github.com/squizlabs/PHP_CodeSniffer/pull/2680 --- Yoast/ruleset.xml | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/Yoast/ruleset.xml b/Yoast/ruleset.xml index 51e25834..c8b6d019 100644 --- a/Yoast/ruleset.xml +++ b/Yoast/ruleset.xml @@ -160,6 +160,15 @@ + + + + + + + + From 53d9baf62799a9cde441c77ee5d29b21193432d6 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 19 Jun 2021 17:06:18 +0200 Subject: [PATCH 15/37] PHPCS: use PHPCompatibility proper, not PHPCompatibilityWP The YoastCS code is not run in the context of WP, but the Yoast standard uses the `PHPCompatibilityWP` ruleset which excludes PHP features polyfilled in WP from being flagged. By setting the `severity` of all the sniffs in the `PHPCompatibilility` ruleset back to `5`, the `exclude`s from the `PHPCompatibilityWP` ruleset are effectively "undone" and we are back to using `PHPCompatibility` proper. --- .phpcs.xml.dist | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.phpcs.xml.dist b/.phpcs.xml.dist index ea690839..d76353ec 100644 --- a/.phpcs.xml.dist +++ b/.phpcs.xml.dist @@ -56,6 +56,14 @@ + + + 5 + + From 82196ff2f1a597a1e224c2946cb472ed71b5c4ed Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 3 Jun 2021 16:12:15 +0200 Subject: [PATCH 16/37] README: minor text improvements --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 74bc766a..892e221b 100644 --- a/README.md +++ b/README.md @@ -54,7 +54,7 @@ To obtain a list of all sniffs used within YoastCS: Not all sniffs have documentation available about what they sniff for, but for those which do, this documentation can be viewed from the command-line: ```bash -"vendor/bin/phpcs" --standard=Yoast --generator=text +"vendor/bin/phpcs" --standard=Yoast --generator=Text ``` ### Running the sniffs @@ -69,7 +69,7 @@ For more command-line options, please have a read through the [PHP_CodeSniffer d #### Yoast plugin repositories -All Yoast plugin repositories contain a `[.]phpcs.xml.dist` file contain the repository specific configuration. +All Yoast plugin repositories contain a `[.]phpcs.xml.dist` file which contains the repository specific configuration. From the root of these repositories, you can run PHPCS by using: ```bash From 68b78022e7d42bfd60d714304c35447f6918e366 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 17 Jun 2021 22:06:54 +0200 Subject: [PATCH 17/37] CI: switch to GitHub Actions - step 1: sniff stage This commit: * Adds a GH Actions workflow for the CI checks which were previously run on Travis in the `sniff` stage. While these aren't 100% CS (= code style) checks, for the badge and workflow display, `CS` still seemed the most descriptive name. Note: includes moving the `composer validate` check to this workflow. * Removes these parts of the `.travis.yml` configuration. Notes: 1. Builds will run on all pushes and on pull requests, with the exception of those just modifying files which are irrelevant to this workflow. --- .github/workflows/basics.yml | 85 ++++++++++++++++++++++++++++++++++++ .travis.yml | 38 ++-------------- 2 files changed, 88 insertions(+), 35 deletions(-) create mode 100644 .github/workflows/basics.yml diff --git a/.github/workflows/basics.yml b/.github/workflows/basics.yml new file mode 100644 index 00000000..1631ec74 --- /dev/null +++ b/.github/workflows/basics.yml @@ -0,0 +1,85 @@ +name: CS + +on: + # Run on all pushes and on all pull requests. + # Prevent the build from running when there are only irrelevant changes. + push: + paths-ignore: + - '**.md' + pull_request: + # Allow manually triggering the workflow. + workflow_dispatch: + +jobs: + checkcs: + name: 'Basic CS and QA checks' + runs-on: ubuntu-latest + + env: + XMLLINT_INDENT: ' ' + + steps: + - name: Checkout code + uses: actions/checkout@v2 + + - name: Install PHP + uses: shivammathur/setup-php@v2 + with: + php-version: '7.4' + coverage: none + tools: cs2pr + + # Validate the composer.json file. + # @link https://getcomposer.org/doc/03-cli.md#validate + - name: Validate Composer installation + run: composer validate --no-check-all --strict + + - name: 'Composer: adjust dependencies' + run: | + # The sniff stage doesn't run the unit tests, so no need for PHPUnit. + composer remove --no-update --dev phpunit/phpunit --no-scripts + # Using PHPCS `master` as an early detection system for bugs upstream. + composer require --no-update --no-scripts squizlabs/php_codesniffer:"dev-master" + # Using WPCS `master` (=stable). This can be changed back to `dev-develop` after the WPCS 3.0.0 release. + composer require --no-update --no-scripts wp-coding-standards/wpcs:"dev-master" + + # Install dependencies and handle caching in one go. + # @link https://github.com/marketplace/actions/install-composer-dependencies + - name: Install Composer dependencies + uses: ramsey/composer-install@v1 + + - name: Install xmllint + run: sudo apt-get install --no-install-recommends -y libxml2-utils + + # Show XML violations inline in the file diff. + # @link https://github.com/marketplace/actions/xmllint-problem-matcher + - uses: korelstar/xmllint-problem-matcher@v1 + + # Validate the ruleset XML file. + # @link http://xmlsoft.org/xmllint.html + - name: Validate ruleset against XML schema + run: xmllint --noout --schema ./vendor/squizlabs/php_codesniffer/phpcs.xsd ./Yoast/ruleset.xml + + # Validate the Docs XML files. + # @link http://xmlsoft.org/xmllint.html + # For the build to properly error when validating against a scheme, these each have to be in their own condition. + - name: Lint the XML sniff docs + run: xmllint --noout ./Yoast/Docs/*/*Standard.xml + + # Check the code-style consistency of the XML ruleset files. + - name: Check XML ruleset code style + run: diff -B --tabsize=4 ./Yoast/ruleset.xml <(xmllint --format "./Yoast/ruleset.xml") + + # Check the codestyle of the files within YoastCS. + # The results of the CS check will be shown inline in the PR via the CS2PR tool. + # @link https://github.com/staabm/annotate-pull-request-from-checkstyle/ + - name: Check PHP code style + continue-on-error: true + run: composer check-cs -- --report-full --report-checkstyle=./phpcs-report.xml + + - name: Show PHPCS results in PR + run: cs2pr ./phpcs-report.xml + + # Check that the sniffs available are feature complete. + - name: Check sniff feature completeness + run: composer check-complete diff --git a/.travis.yml b/.travis.yml index 6d891d15..b455bfcf 100644 --- a/.travis.yml +++ b/.travis.yml @@ -33,7 +33,6 @@ env: # Note: for pull requests, "develop" should be the base branch name. # See: https://docs.travis-ci.com/user/conditions-v1 stages: - - name: sniff - name: quicktest if: type = push AND branch NOT IN (master, develop) - name: test @@ -42,30 +41,6 @@ stages: jobs: fast_finish: true include: - #### SNIFF STAGE #### - - stage: sniff - php: 7.4 - env: PHPCS_BRANCH="dev-master" WPCS="dev-master" - addons: - apt: - packages: - - libxml2-utils - script: - # Check the codestyle of the files within YoastCS. - - composer check-cs - - # Validate the xml files. - # @link http://xmlsoft.org/xmllint.html - # For the build to properly error when validating against a scheme, these each have to be in their own condition. - - xmllint --noout --schema ./vendor/squizlabs/php_codesniffer/phpcs.xsd ./Yoast/ruleset.xml - - xmllint --noout ./Yoast/Docs/*/*Standard.xml - - # Check the code-style consistency of the xml files. - - diff -B --tabsize=4 ./Yoast/ruleset.xml <(xmllint --format "./Yoast/ruleset.xml") - - # Check that the sniffs available are feature complete. - - composer check-complete - #### QUICK TEST STAGE #### # This is a much quicker test which only runs the unit tests and linting against low/high # supported PHP/PHPCS/WPCS combinations. @@ -137,7 +112,7 @@ before_install: # On stable PHPCS versions, allow for PHP deprecation notices. # Unit tests don't need to fail on those for stable releases where those issues won't get fixed anymore. - | - if [[ "${TRAVIS_BUILD_STAGE_NAME^}" != "Sniff" && $PHPCS_BRANCH != "dev-master" && $WPCS != "dev-develop" ]]; then + if [[ $PHPCS_BRANCH != "dev-master" && $WPCS != "dev-develop" ]]; then echo 'error_reporting = E_ALL & ~E_DEPRECATED' >> ~/.phpenv/versions/$(phpenv version-name)/etc/conf.d/travis.ini fi @@ -148,11 +123,8 @@ before_install: - travis_retry composer require squizlabs/php_codesniffer:${PHPCS_BRANCH} --no-update --no-suggest --no-scripts # Set the WPCS version to test against. - travis_retry composer require wp-coding-standards/wpcs:${WPCS} --no-update --no-suggest --no-scripts - - | - if [[ "${TRAVIS_BUILD_STAGE_NAME^}" != "Sniff" ]]; then - # For testing the YoastCS native sniffs, the rest of the packages aren't needed. - travis_retry composer remove phpcompatibility/phpcompatibility-wp phpcompatibility/php-compatibility --no-update - fi + # For testing the YoastCS native sniffs, the rest of the packages aren't needed. + - travis_retry composer remove phpcompatibility/phpcompatibility-wp phpcompatibility/php-compatibility --no-update - | if [[ $TRAVIS_PHP_VERSION == "nightly" || $TRAVIS_PHP_VERSION == "8.0" ]]; then @@ -170,7 +142,3 @@ script: # Run the unit tests. - vendor/bin/phpunit --filter Yoast --bootstrap="$PHPCS_DIR/tests/bootstrap.php" $PHPCS_DIR/tests/AllTests.php - - # Validate the composer.json file. - # @link https://getcomposer.org/doc/03-cli.md#validate - - if [[ "$PHPLINT" == "1" ]]; then composer validate --no-check-all --strict; fi From 257d439dc6a296b148b9d9ff42c8199c052403bd Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 17 Jun 2021 22:25:24 +0200 Subject: [PATCH 18/37] CI: switch to GitHub Actions - step 2: quick test stage This commit: * Adds a GH Actions workflow for the CI checks which were previously run on Travis in the `quicktest` stage. * Removes that part of the `.travis.yml` configuration. Notes: 1. Previously, this "stage" would run on all `push` events, except when merging to `master` or `develop`. The current set-up still does so, with one exception: pushes to `develop` will now use this quicktest instead of the full test. `develop` is a protected branch anyhow, so all merges will already have had the full test run in the pull request. For merges to `master`, the full Test will still be used as that will generally mean we're getting ready to tag a release and some extra safety checking before a release is not a bad thing. 2. This also updates the "high" PHP version for the "quicktest" against PHPCS `dev-master` from PHP 7.4 to PHP 8.0 by using `latest` which translates to "latest stable PHP release". --- .github/workflows/quicktest.yml | 88 +++++++++++++++++++++++++++++++++ .travis.yml | 26 ---------- 2 files changed, 88 insertions(+), 26 deletions(-) create mode 100644 .github/workflows/quicktest.yml diff --git a/.github/workflows/quicktest.yml b/.github/workflows/quicktest.yml new file mode 100644 index 00000000..ca98c0d5 --- /dev/null +++ b/.github/workflows/quicktest.yml @@ -0,0 +1,88 @@ +name: Quicktest + +on: + # Run on pushes, including merges, to all branches except `master`. + push: + branches-ignore: + - master + paths-ignore: + - '**.md' + # Allow manually triggering the workflow. + workflow_dispatch: + +jobs: + #### QUICK TEST STAGE #### + # This is a much quicker test which only runs the unit tests and linting against low/high + # supported PHP/PHPCS/WPCS combinations. + quicktest: + runs-on: ubuntu-latest + + strategy: + matrix: + include: + - php_version: 'latest' + phpcs_version: 'dev-master' + wpcs_version: 'dev-master' + - php_version: 'latest' + phpcs_version: '3.6.0' + wpcs_version: '2.3.0' + - php_version: '5.4' + phpcs_version: 'dev-master' + wpcs_version: '2.3.0' + - php_version: '5.4' + phpcs_version: '3.6.0' + wpcs_version: 'dev-master' + + name: "QTest${{ matrix.phpcs_version == 'dev-master' && ' + Lint' || '' }}: PHP ${{ matrix.php_version }} - PHPCS ${{ matrix.phpcs_version }} - WPCS ${{ matrix.wpcs_version }}" + + steps: + - name: Checkout code + uses: actions/checkout@v2 + + # On stable PHPCS versions, allow for PHP deprecation notices. + # Unit tests don't need to fail on those for stable releases where those issues won't get fixed anymore. + - name: Setup ini config + id: set_ini + run: | + if [ "${{ matrix.phpcs_version }}" != "dev-master" ]; then + echo '::set-output name=PHP_INI::error_reporting=E_ALL & ~E_DEPRECATED, display_errors=On' + else + echo '::set-output name=PHP_INI::error_reporting=E_ALL, display_errors=On' + fi + + - name: Install PHP + uses: shivammathur/setup-php@v2 + with: + php-version: ${{ matrix.php_version }} + ini-values: ${{ steps.set_ini.outputs.PHP_INI }} + coverage: none + + - name: 'Composer: adjust dependencies' + run: | + # Set the PHPCS version to test against. + composer require --no-update --no-scripts squizlabs/php_codesniffer:"${{ matrix.phpcs_version }}" + # Set the WPCS version to test against. + composer require --no-update --no-scripts wp-coding-standards/wpcs:"${{ matrix.wpcs_version }}" + + # Install dependencies and handle caching in one go. + # @link https://github.com/marketplace/actions/install-composer-dependencies + - name: Install Composer dependencies - normal + if: ${{ startsWith( matrix.php_version, '8' ) == false && matrix.php_version != 'latest' }} + uses: ramsey/composer-install@v1 + + # For the PHP 8.0 and higher, we need to install with ignore platform reqs as not all dependencies allow it. + - name: Install Composer dependencies - with ignore platform + if: ${{ startsWith( matrix.php_version, '8' ) || matrix.php_version == 'latest' }} + uses: ramsey/composer-install@v1 + with: + composer-options: --ignore-platform-reqs + + - name: Verify installed standards + run: vendor/bin/phpcs -i + + - name: Lint against parse errors + if: matrix.phpcs_version == 'dev-master' + run: composer lint + + - name: Run the unit tests + run: composer test diff --git a/.travis.yml b/.travis.yml index b455bfcf..29474836 100644 --- a/.travis.yml +++ b/.travis.yml @@ -27,35 +27,9 @@ env: - PHPCS_BRANCH="3.6.0" WPCS="dev-master" - PHPCS_BRANCH="3.6.0" WPCS="2.3.0" -# Define the stages used. -# For non-PRs, only the sniff, ruleset and quicktest stages are run. -# For pull requests and merges, the full script is run (skipping quicktest). -# Note: for pull requests, "develop" should be the base branch name. -# See: https://docs.travis-ci.com/user/conditions-v1 -stages: - - name: quicktest - if: type = push AND branch NOT IN (master, develop) - - name: test - if: branch IN (master, develop) - jobs: fast_finish: true include: - #### QUICK TEST STAGE #### - # This is a much quicker test which only runs the unit tests and linting against low/high - # supported PHP/PHPCS/WPCS combinations. - - stage: quicktest - php: 7.4 - env: PHPCS_BRANCH="dev-master" WPCS="dev-master" PHPLINT=1 - - php: 7.4 - env: PHPCS_BRANCH="3.6.0" WPCS="2.3.0" - - php: 5.4 - dist: trusty - env: PHPCS_BRANCH="dev-master" WPCS="2.3.0" PHPLINT=1 - - php: 5.4 - dist: trusty - env: PHPCS_BRANCH="3.6.0" WPCS="dev-master" - #### TEST STAGE #### # Additional builds against PHP versions which need a different distro. - stage: test From e37ae12f4184c6139a40f64458df3b41b2f95c14 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 17 Jun 2021 23:06:02 +0200 Subject: [PATCH 19/37] CI: switch to GitHub Actions - step 3: test stage This commit: * Adds a GH Actions workflow for the CI checks which were previously run on Travis in the `test` stage. * Removes the, now redundant, `.travis.yml` configuration. * Updates the `.gitattributes` file. Notes: 1. Previously, this "stage" would run on all `pull requests` events. The current set-up still does so, with one addition: pushes to `master` (merges) will now also use this workflow instead of the quicktest. This replaces the full run on tagging a release, meaning that things will essentially be the same. 2. As there are a couple of jobs which are "allowed to fail" (`experimental` = true), the build status may unfortunately show as "failed", even though all non-experimental jobs have succeeded. This is a known issue in GHA: https://github.com/actions/toolkit/issues/399 --- .gitattributes | 1 - .github/workflows/test.yml | 135 +++++++++++++++++++++++++++++++++++++ .travis.yml | 118 -------------------------------- 3 files changed, 135 insertions(+), 119 deletions(-) create mode 100644 .github/workflows/test.yml delete mode 100644 .travis.yml diff --git a/.gitattributes b/.gitattributes index 5679c2ae..0a102227 100644 --- a/.gitattributes +++ b/.gitattributes @@ -9,7 +9,6 @@ /.gitignore export-ignore /.phpcs.xml export-ignore /.phpcs.xml.dist export-ignore -/.travis.yml export-ignore /phpcs.xml export-ignore /phpcs.xml.dist export-ignore /phpunit.xml export-ignore diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml new file mode 100644 index 00000000..a57d163b --- /dev/null +++ b/.github/workflows/test.yml @@ -0,0 +1,135 @@ +name: Test + +on: + # Run on pushes to `master` and on all pull requests. + push: + branches: + - master + paths-ignore: + - '**.md' + pull_request: + # Allow manually triggering the workflow. + workflow_dispatch: + +jobs: + #### TEST STAGE #### + test: + runs-on: ubuntu-latest + + strategy: + # Keys: + # - experimental: Whether the build is "allowed to fail". + matrix: + # The GHA matrix works different from Travis. + # You can define jobs here and then augment them with extra variables in `include`, + # as well as add extra jobs in `include`. + # @link https://docs.github.com/en/free-pro-team@latest/actions/reference/workflow-syntax-for-github-actions#jobsjob_idstrategymatrix + # + # Note: while WPCS 3.0.0 is under development, the matrix will use `dev-master`. + # Once it has been released and YoastCS has been made compatible, the matrix should switch (back) + # WPCS `dev-master` to `dev-develop`. + php_version: ['5.4', '5.5', '5.6', '7.0', '7.1', '7.2', '7.3', '7.4', '8.0'] + phpcs_version: ['3.6.0', 'dev-master'] + wpcs_version: ['2.3.0', 'dev-master'] + experimental: [false] + + include: + # Experimental builds. These are allowed to fail. + + # PHP nightly + - php_version: '8.1' + phpcs_version: 'dev-master' + wpcs_version: 'dev-master' + experimental: true + # Test against WPCS unstable. Re-enable when WPCS is not in dev for the next major. + #- php_version: '8.0' + # phpcs_version: 'dev-master' + # wpcs_version: 'dev-develop' + # experimental: true + + # Test against the next major of PHPCS. Temporarily disabled due to upstream bugs. + #- php_version: '7.4' + # phpcs_version: '4.0.x-dev' + # wpcs_version: 'dev-develop' + # experimental: true + + name: "Test${{ matrix.phpcs_version == 'dev-master' && matrix.wpcs_version == 'dev-master' && ' + Lint' || '' }}: PHP ${{ matrix.php_version }} - PHPCS ${{ matrix.phpcs_version }} - WPCS ${{ matrix.wpcs_version }}" + + continue-on-error: ${{ matrix.experimental }} + + steps: + - name: Checkout code + uses: actions/checkout@v2 + + # On stable PHPCS versions, allow for PHP deprecation notices. + # Unit tests don't need to fail on those for stable releases where those issues won't get fixed anymore. + - name: Setup ini config + id: set_ini + run: | + if [[ "${{ matrix.phpcs_version }}" != "dev-master" && "${{ matrix.phpcs_version }}" != "4.0.x-dev" ]]; then + echo '::set-output name=PHP_INI::error_reporting=E_ALL & ~E_DEPRECATED, display_errors=On' + else + echo '::set-output name=PHP_INI::error_reporting=E_ALL, display_errors=On' + fi + + - name: Install PHP + uses: shivammathur/setup-php@v2 + with: + php-version: ${{ matrix.php_version }} + ini-values: ${{ steps.set_ini.outputs.PHP_INI }} + coverage: none + tools: cs2pr + env: + # Token is needed for the PHPCS 4.x run against source. + COMPOSER_TOKEN: ${{ secrets.GITHUB_TOKEN }} + + # Set Composer up to download only PHPCS from source for PHPCS 4.x. + # The source is needed to get the base testcase from PHPCS. + # All other jobs can use `auto`, which is Composer's default value. + - name: 'Composer: conditionally prefer source for PHPCS' + if: ${{ startsWith( matrix.phpcs_version, '4' ) }} + run: composer config preferred-install.squizlabs/php_codesniffer source + + - name: 'Composer: adjust dependencies' + run: | + # Set the PHPCS version to test against. + composer require --no-update --no-scripts squizlabs/php_codesniffer:"${{ matrix.phpcs_version }}" + # Set the WPCS version to test against. + composer require --no-update --no-scripts wp-coding-standards/wpcs:"${{ matrix.wpcs_version }}" + + - name: 'Composer: conditionally remove PHPCSDevtools' + if: ${{ startsWith( matrix.phpcs_version, '4' ) }} + # Remove devtools as it will not (yet) install on PHPCS 4.x. + run: composer remove --no-update --dev phpcsstandards/phpcsdevtools + + # Install dependencies and handle caching in one go. + # @link https://github.com/marketplace/actions/install-composer-dependencies + - name: Install Composer dependencies - normal + if: ${{ startsWith( matrix.php_version, '8' ) == false }} + uses: ramsey/composer-install@v1 + + # For the PHP 8/"nightly", we need to install with ignore platform reqs as we're still using PHPUnit 7. + - name: Install Composer dependencies - with ignore platform + if: ${{ startsWith( matrix.php_version, '8' ) }} + uses: ramsey/composer-install@v1 + with: + composer-options: --ignore-platform-reqs + + - name: Verify installed standards + run: vendor/bin/phpcs -i + + # The results of the linting will be shown inline in the PR via the CS2PR tool. + # @link https://github.com/staabm/annotate-pull-request-from-checkstyle/ + - name: Lint against parse errors + if: ${{ matrix.phpcs_version == 'dev-master' && matrix.wpcs_version == 'dev-master' }} + run: composer lint -- --checkstyle | cs2pr + + - name: Run the unit tests - PHP 5.4 - 8.0 + if: ${{ matrix.php_version != '8.1' }} + run: composer test + + - name: Run the unit tests - PHP 8.1 + if: ${{ matrix.php_version == '8.1' }} + run: composer test -- --no-configuration --dont-report-useless-tests + env: + PHPCS_IGNORE_TESTS: 'PHPCompatibility,WordPress' diff --git a/.travis.yml b/.travis.yml deleted file mode 100644 index 29474836..00000000 --- a/.travis.yml +++ /dev/null @@ -1,118 +0,0 @@ -os: linux - -language: php - -cache: - apt: true - directories: - - .cache - # Cache directory for older Composer versions. - - $HOME/.composer/cache/files - # Cache directory for more recent Composer versions. - - $HOME/.cache/composer/files - -php: - - 5.6 - - 7.0 - - 7.1 - - 7.2 - - 7.3 - - 7.4 - -env: - # Test against the highest/lowest supported PHPCS and WPCS versions. - # Using WPCS 'master' for the last stable release. - - PHPCS_BRANCH="dev-master" WPCS="dev-master" PHPLINT=1 - - PHPCS_BRANCH="dev-master" WPCS="2.3.0" - - PHPCS_BRANCH="3.6.0" WPCS="dev-master" - - PHPCS_BRANCH="3.6.0" WPCS="2.3.0" - -jobs: - fast_finish: true - include: - #### TEST STAGE #### - # Additional builds against PHP versions which need a different distro. - - stage: test - php: 8.0 - env: PHPCS_BRANCH="dev-master" WPCS="dev-master" PHPLINT=1 - - php: 8.0 - # PHPCS is only compatible with PHP 8.0 as of version 3.6.0. - env: PHPCS_BRANCH="3.6.0" WPCS="2.3.0" - - php: 5.5 - dist: trusty - env: PHPCS_BRANCH="dev-master" WPCS="dev-master" PHPLINT=1 - - php: 5.5 - dist: trusty - env: PHPCS_BRANCH="dev-master" WPCS="2.3.0" - - php: 5.5 - dist: trusty - env: PHPCS_BRANCH="3.6.0" WPCS="dev-master" - - php: 5.5 - dist: trusty - env: PHPCS_BRANCH="3.6.0" WPCS="2.3.0" - - php: 5.4 - dist: trusty - env: PHPCS_BRANCH="dev-master" WPCS="dev-master" PHPLINT=1 - - php: 5.4 - dist: trusty - env: PHPCS_BRANCH="dev-master" WPCS="2.3.0" - - php: 5.4 - dist: trusty - env: PHPCS_BRANCH="3.6.0" WPCS="dev-master" - - php: 5.4 - dist: trusty - env: PHPCS_BRANCH="3.6.0" WPCS="2.3.0" - - # Test against WPCS unstable. - - php: 7.4 - env: PHPCS_BRANCH="dev-master" WPCS="dev-develop" - - # Test against PHP nightly. - - php: "nightly" - env: PHPCS_BRANCH="dev-master" WPCS="dev-master" PHPLINT=1 - - allow_failures: - # Allow failures for unstable builds. - - php: "nightly" - - env: PHPCS_BRANCH="dev-master" WPCS="dev-develop" - - -before_install: - # Speed up build time by disabling Xdebug. - # https://johnblackbourn.com/reducing-travis-ci-build-times-for-wordpress-projects/ - # https://twitter.com/kelunik/status/954242454676475904 - - phpenv config-rm xdebug.ini || echo 'No xdebug config.' - - # On stable PHPCS versions, allow for PHP deprecation notices. - # Unit tests don't need to fail on those for stable releases where those issues won't get fixed anymore. - - | - if [[ $PHPCS_BRANCH != "dev-master" && $WPCS != "dev-develop" ]]; then - echo 'error_reporting = E_ALL & ~E_DEPRECATED' >> ~/.phpenv/versions/$(phpenv version-name)/etc/conf.d/travis.ini - fi - - - export XMLLINT_INDENT=" " - - export PHPCS_DIR=$(pwd)/vendor/squizlabs/php_codesniffer - - export PHPCS_BIN=$(pwd)/vendor/bin/phpcs - # Set the PHPCS version to test against. - - travis_retry composer require squizlabs/php_codesniffer:${PHPCS_BRANCH} --no-update --no-suggest --no-scripts - # Set the WPCS version to test against. - - travis_retry composer require wp-coding-standards/wpcs:${WPCS} --no-update --no-suggest --no-scripts - # For testing the YoastCS native sniffs, the rest of the packages aren't needed. - - travis_retry composer remove phpcompatibility/phpcompatibility-wp phpcompatibility/php-compatibility --no-update - - - | - if [[ $TRAVIS_PHP_VERSION == "nightly" || $TRAVIS_PHP_VERSION == "8.0" ]]; then - travis_retry composer install --prefer-dist --no-interaction --ignore-platform-reqs - else - travis_retry composer install --prefer-dist --no-interaction - fi - - # The DealerDirect Composer plugin script takes care of the installed_paths. - - $PHPCS_BIN -i - -script: - # Lint the PHP files against parse errors. - - if [[ "$PHPLINT" == "1" ]]; then composer lint; fi - - # Run the unit tests. - - vendor/bin/phpunit --filter Yoast --bootstrap="$PHPCS_DIR/tests/bootstrap.php" $PHPCS_DIR/tests/AllTests.php From ebf0a86583f1bfe87d078152ee674d84926bd736 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 28 Jul 2021 13:00:39 +0200 Subject: [PATCH 20/37] Commenting/CoversTag: make more code-style independent The regex used in the `Yoast.Commenting.CoversTag` sniff was - in part - based on the known naming conventions used in WP projects. As the Yoast standard will now also be used outside of WP projects (special projects team), the assumption that the WP naming conventions will be used needs to be removed. Fixed now. Includes updated and additional unit tests which would not pass without this fix. --- Yoast/Sniffs/Commenting/CoversTagSniff.php | 4 +--- Yoast/Tests/Commenting/CoversTagUnitTest.inc | 15 +++++++++++++-- .../Tests/Commenting/CoversTagUnitTest.inc.fixed | 15 +++++++++++++-- Yoast/Tests/Commenting/CoversTagUnitTest.php | 2 -- 4 files changed, 27 insertions(+), 9 deletions(-) diff --git a/Yoast/Sniffs/Commenting/CoversTagSniff.php b/Yoast/Sniffs/Commenting/CoversTagSniff.php index c5d26ee3..dfdff6c0 100644 --- a/Yoast/Sniffs/Commenting/CoversTagSniff.php +++ b/Yoast/Sniffs/Commenting/CoversTagSniff.php @@ -24,11 +24,9 @@ class CoversTagSniff implements Sniff { /** * Regex to check for valid content of a @covers tags. * - * Takes the WP naming conventions into account (up to a point). - * * @var string */ - const VALID_CONTENT_REGEX = '(?:\\\\?(?:(?[A-Z][a-zA-Z0-9_\x7f-\xff]*)\\\\)*(?P>OOName)(?:|::<[!]?(?:public|protected|private)>|::(?(?!public$|protected$|private$)[a-z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*))?|::(?P>functionName)|\\\\?(?:(?P>OOName)\\\\)+(?P>functionName))'; + const VALID_CONTENT_REGEX = '(?:\\\\?(?:(?[a-zA-Z_\x80-\xff][a-zA-Z0-9_\x80-\xff]*)\\\\)*(?P>OOName)(?:|::<[!]?(?:public|protected|private)>|::(?(?!public$|protected$|private$)(?P>OOName)))?|::(?P>functionName)|\\\\?(?:(?P>OOName)\\\\)+(?P>functionName))'; /** * Base error message. diff --git a/Yoast/Tests/Commenting/CoversTagUnitTest.inc b/Yoast/Tests/Commenting/CoversTagUnitTest.inc index 419d2c6b..326e26b3 100644 --- a/Yoast/Tests/Commenting/CoversTagUnitTest.inc +++ b/Yoast/Tests/Commenting/CoversTagUnitTest.inc @@ -29,13 +29,13 @@ class ClassNameTest { * @covers \Class_name::method_name * @covers Name\Space\Class_Name::method_name * @covers \Name\Space\Class_name::method_name + * @covers global_function + * @covers self::method_name * * Incorrect: - * @covers global_function * @covers ::global_func() * @covers Name\Space\func_name() * @covers My_Class {} - * @covers self::method_name * @covers My_Class::another_method_name() * @covers Name\Space\My_Class::another_method_name() */ @@ -153,4 +153,15 @@ class ClassNameTest { * @param int $int Description. */ public function testDuplicateCoversTagFixable($int) {} + + /** + * Docblock. + * + * Correct: + * @covers ::GlobalFunction + * @covers ClassName::MethodName + * @covers \Classname::MethodName + * @covers \Name\Space\ClassName::MethodName + */ + public function testRecognizingNamesNotFollowingWPNamingConventions() {} } diff --git a/Yoast/Tests/Commenting/CoversTagUnitTest.inc.fixed b/Yoast/Tests/Commenting/CoversTagUnitTest.inc.fixed index f355236f..53a0b1a0 100644 --- a/Yoast/Tests/Commenting/CoversTagUnitTest.inc.fixed +++ b/Yoast/Tests/Commenting/CoversTagUnitTest.inc.fixed @@ -29,13 +29,13 @@ class ClassNameTest { * @covers \Class_name::method_name * @covers Name\Space\Class_Name::method_name * @covers \Name\Space\Class_name::method_name + * @covers global_function + * @covers self::method_name * * Incorrect: - * @covers global_function * @covers ::global_func * @covers Name\Space\func_name * @covers My_Class - * @covers self::method_name * @covers My_Class::another_method_name * @covers Name\Space\My_Class::another_method_name */ @@ -152,4 +152,15 @@ class ClassNameTest { * @param int $int Description. */ public function testDuplicateCoversTagFixable($int) {} + + /** + * Docblock. + * + * Correct: + * @covers ::GlobalFunction + * @covers ClassName::MethodName + * @covers \Classname::MethodName + * @covers \Name\Space\ClassName::MethodName + */ + public function testRecognizingNamesNotFollowingWPNamingConventions() {} } diff --git a/Yoast/Tests/Commenting/CoversTagUnitTest.php b/Yoast/Tests/Commenting/CoversTagUnitTest.php index b4a2c1b1..08be552c 100644 --- a/Yoast/Tests/Commenting/CoversTagUnitTest.php +++ b/Yoast/Tests/Commenting/CoversTagUnitTest.php @@ -22,8 +22,6 @@ class CoversTagUnitTest extends AbstractSniffUnitTest { */ public function getErrorList() { return [ - 34 => 1, - 35 => 1, 36 => 1, 37 => 1, 38 => 1, From 67033a642f484ddc60a8c9f4d2033b2d17d05716 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 28 Jul 2021 13:29:41 +0200 Subject: [PATCH 21/37] Commenting/FileComment: efficiency tweak While reviewing the sniffs for WP-code style independence (for use in special projects), I noticed that the `FileComment` sniff may walk all the way to the end of a file before deciding to pass off to the parent sniff. While for namespaced files, this will never happen as a namespace declaration will (normally) be the first statement in a file - with the possible exception of a `declare` statement. For non-namespaced files, this made the sniff highly inefficient as, especially for long files, it would have to walk through all the tokens in the file before deciding it is a non-namespaced file. Fixed now by only walking as far as the first non-empty (not whitespace, not comment), non-declare token and deciding based on that. While a file _may_ contain a namespace declaration later on in the file (multi-namespace files), this rule is not intended for those type of files anyway, so it is correct for the "must-have file comment" rule to be applied in that case. This change is already covered by the existing unit tests. Includes additional unit tests specifically for the handling of the `declare` statements though. --- Yoast/Sniffs/Commenting/FileCommentSniff.php | 34 +++++++++++++++---- .../Commenting/FileCommentUnitTest.11.inc | 24 +++++++++++++ .../Commenting/FileCommentUnitTest.12.inc | 8 +++++ .../Commenting/FileCommentUnitTest.13.inc | 10 ++++++ .../Commenting/FileCommentUnitTest.14.inc | 17 ++++++++++ .../Tests/Commenting/FileCommentUnitTest.php | 2 ++ 6 files changed, 89 insertions(+), 6 deletions(-) create mode 100644 Yoast/Tests/Commenting/FileCommentUnitTest.11.inc create mode 100644 Yoast/Tests/Commenting/FileCommentUnitTest.12.inc create mode 100644 Yoast/Tests/Commenting/FileCommentUnitTest.13.inc create mode 100644 Yoast/Tests/Commenting/FileCommentUnitTest.14.inc diff --git a/Yoast/Sniffs/Commenting/FileCommentSniff.php b/Yoast/Sniffs/Commenting/FileCommentSniff.php index 88289362..59fb4668 100644 --- a/Yoast/Sniffs/Commenting/FileCommentSniff.php +++ b/Yoast/Sniffs/Commenting/FileCommentSniff.php @@ -39,14 +39,36 @@ class FileCommentSniff extends Squiz_FileCommentSniff { */ public function process( File $phpcsFile, $stackPtr ) { - $namespace_token = $phpcsFile->findNext( \T_NAMESPACE, $stackPtr ); - if ( $namespace_token === false ) { - // No namespace found, fall through to parent sniff. - return parent::process( $phpcsFile, $stackPtr ); - } - $tokens = $phpcsFile->getTokens(); + $namespace_token = $stackPtr; + do { + $namespace_token = $phpcsFile->findNext( Tokens::$emptyTokens, ( $namespace_token + 1 ), null, true ); + if ( $namespace_token === false ) { + // No non-empty token found, fall through to parent sniff. + return parent::process( $phpcsFile, $stackPtr ); + } + + if ( $tokens[ $namespace_token ]['code'] === \T_DECLARE ) { + // Declare statement found. Find the end of it and skip over it. + $end = $phpcsFile->findNext( [ \T_SEMICOLON, \T_OPEN_CURLY_BRACKET ], ( $namespace_token + 1 ), null, false, null, true ); + + if ( $end !== false ) { + $namespace_token = $end; + } + + continue; + } + + if ( $tokens[ $namespace_token ]['code'] !== \T_NAMESPACE ) { + // No namespace found, fall through to parent sniff. + return parent::process( $phpcsFile, $stackPtr ); + } + + // Stop searching if the next non-empty token wasn't a namespace token. + break; + } while ( true ); + $next_non_empty = $phpcsFile->findNext( Tokens::$emptyTokens, ( $namespace_token + 1 ), null, true ); if ( $next_non_empty === false || $tokens[ $next_non_empty ]['code'] === \T_SEMICOLON diff --git a/Yoast/Tests/Commenting/FileCommentUnitTest.11.inc b/Yoast/Tests/Commenting/FileCommentUnitTest.11.inc new file mode 100644 index 00000000..2ae010d8 --- /dev/null +++ b/Yoast/Tests/Commenting/FileCommentUnitTest.11.inc @@ -0,0 +1,24 @@ + + * @copyright 2018 Squiz Pty Ltd (ABN 77 084 670 600) + */ + +declare(strict_types=1); + +/** + * Class docblock. + */ +class Testing { + public function test() { + echo namespace\SomeClass::$static_property; // This is not a namespace declaration, but use of the namespace operator. + } +} diff --git a/Yoast/Tests/Commenting/FileCommentUnitTest.12.inc b/Yoast/Tests/Commenting/FileCommentUnitTest.12.inc new file mode 100644 index 00000000..a0afacb0 --- /dev/null +++ b/Yoast/Tests/Commenting/FileCommentUnitTest.12.inc @@ -0,0 +1,8 @@ + 1, ]; @@ -47,6 +48,7 @@ public function getWarningList( $testFile = '' ) { switch ( $testFile ) { case 'FileCommentUnitTest.4.inc': case 'FileCommentUnitTest.6.inc': + case 'FileCommentUnitTest.14.inc': return [ 2 => 1, ]; From f7826524b92caa655440c81d2ef571b8be9b22e0 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 28 Jul 2021 13:36:23 +0200 Subject: [PATCH 22/37] Commenting/FileComment: minor bug fix The PHPCS `File::findNext()` function will return the stack pointer to the token found or `false` if no matching token is found. The check for the `$comment_start` token could therefore return `false` when no comment token is found between a PHP open tag and the first namespace token, which would mean that the `$tokens[ $comment_start ]['code']` check in the condition would effectively look at `$tokens[ false ]['code']`, which (in most cases) would translate to the PHP open tag. While this doesn't lead to any problems in practice, it is still wrong, so a check against `false` has been added to the condition now. --- Yoast/Sniffs/Commenting/FileCommentSniff.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Yoast/Sniffs/Commenting/FileCommentSniff.php b/Yoast/Sniffs/Commenting/FileCommentSniff.php index 59fb4668..8dd34386 100644 --- a/Yoast/Sniffs/Commenting/FileCommentSniff.php +++ b/Yoast/Sniffs/Commenting/FileCommentSniff.php @@ -82,7 +82,7 @@ public function process( File $phpcsFile, $stackPtr ) { $comment_start = $phpcsFile->findNext( \T_WHITESPACE, ( $stackPtr + 1 ), $namespace_token, true ); - if ( $tokens[ $comment_start ]['code'] === \T_DOC_COMMENT_OPEN_TAG ) { + if ( $comment_start !== false && $tokens[ $comment_start ]['code'] === \T_DOC_COMMENT_OPEN_TAG ) { $phpcsFile->addWarning( 'A file containing a (named) namespace declaration does not need a file docblock', $comment_start, From 7a745985b325a7425b8e4490c0dec68ff32e3a34 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 28 Jul 2021 15:46:26 +0200 Subject: [PATCH 23/37] Commenting/TestsHaveCoversTag: prevent false negatives with attributes PHP 8.0+ attributes can be placed between a docblock and the class/function declaration it applies to. The `Yoast.Commenting.TestsHaveCoversTag` sniff did not yet take this into account. This could result in both false positives as well as false negatives: * False positives for individual functions not having a `@covers` tag if the sniff did not find a class docblock containing a `@covers` tag. * False negatives for functions which do have a docblock, but without a `@covers` tag in either the function or the class docblock, where the sniff did not find function docblock due to the attribute. Fixed now. Includes unit tests. --- .../Commenting/TestsHaveCoversTagSniff.php | 54 +++++++++++++------ .../Commenting/TestsHaveCoversTagUnitTest.inc | 30 +++++++++++ .../Commenting/TestsHaveCoversTagUnitTest.php | 5 +- 3 files changed, 70 insertions(+), 19 deletions(-) diff --git a/Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php b/Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php index f09ecabf..fd27ac11 100644 --- a/Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php +++ b/Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php @@ -74,21 +74,30 @@ protected function process_class( File $phpcsFile, $stackPtr ) { return; } - // @todo: Once PHPCS 3.5.0 is out, replace with call to new findCommentAboveOOStructure() method. - $find = [ - \T_WHITESPACE, - \T_ABSTRACT, - \T_FINAL, + // @todo: Once PHPCSUtils is out, replace with call to new findCommentAboveOOStructure() method. + $ignore = [ + \T_WHITESPACE => \T_WHITESPACE, + \T_ABSTRACT => \T_ABSTRACT, + \T_FINAL => \T_FINAL, ]; $commentEnd = $stackPtr; - do { - $commentEnd = $phpcsFile->findPrevious( $find, ( $commentEnd - 1 ), null, true ); - } while ( $tokens[ $commentEnd ]['line'] === $tokens[ $stackPtr ]['line'] ); + for ( $commentEnd = ( $stackPtr - 1 ); $commentEnd >= 0; $commentEnd-- ) { + if ( isset( $ignore[ $tokens[ $commentEnd ]['code'] ] ) === true ) { + continue; + } - if ( $tokens[ $commentEnd ]['code'] !== \T_DOC_COMMENT_CLOSE_TAG - || $tokens[ $commentEnd ]['line'] !== ( $tokens[ $stackPtr ]['line'] - 1 ) - ) { + if ( $tokens[ $commentEnd ]['code'] === \T_ATTRIBUTE_END + && isset( $tokens[ $commentEnd ]['attribute_opener'] ) === true + ) { + $commentEnd = $tokens[ $commentEnd ]['attribute_opener']; + continue; + } + + break; + } + + if ( $tokens[ $commentEnd ]['code'] !== \T_DOC_COMMENT_CLOSE_TAG ) { // Class without (proper) docblock. Not our concern. return; } @@ -126,14 +135,25 @@ protected function process_class( File $phpcsFile, $stackPtr ) { protected function process_function( File $phpcsFile, $stackPtr ) { $tokens = $phpcsFile->getTokens(); - // @todo: Once PHPCS 3.5.0 is out, replace with call to new findCommentAboveOOStructure() method. - $find = Tokens::$methodPrefixes; - $find[] = \T_WHITESPACE; + // @todo: Once PHPCSUtils is out, replace with call to new findCommentAboveFunction() method. + $ignore = Tokens::$methodPrefixes; + $ignore[ \T_WHITESPACE ] = \T_WHITESPACE; $commentEnd = $stackPtr; - do { - $commentEnd = $phpcsFile->findPrevious( $find, ( $commentEnd - 1 ), null, true ); - } while ( $tokens[ $commentEnd ]['line'] === $tokens[ $stackPtr ]['line'] ); + for ( $commentEnd = ( $stackPtr - 1 ); $commentEnd >= 0; $commentEnd-- ) { + if ( isset( $ignore[ $tokens[ $commentEnd ]['code'] ] ) === true ) { + continue; + } + + if ( $tokens[ $commentEnd ]['code'] === \T_ATTRIBUTE_END + && isset( $tokens[ $commentEnd ]['attribute_opener'] ) === true + ) { + $commentEnd = $tokens[ $commentEnd ]['attribute_opener']; + continue; + } + + break; + } if ( $tokens[ $commentEnd ]['code'] !== \T_DOC_COMMENT_CLOSE_TAG || $tokens[ $commentEnd ]['line'] !== ( $tokens[ $stackPtr ]['line'] - 1 ) diff --git a/Yoast/Tests/Commenting/TestsHaveCoversTagUnitTest.inc b/Yoast/Tests/Commenting/TestsHaveCoversTagUnitTest.inc index 555aec50..7bc15554 100644 --- a/Yoast/Tests/Commenting/TestsHaveCoversTagUnitTest.inc +++ b/Yoast/Tests/Commenting/TestsHaveCoversTagUnitTest.inc @@ -103,3 +103,33 @@ class ClassNameTest { */ public function annotatedTestHasCoversTag() {} } + +/** + * @covers ClassName + */ +#[Some_Attribute] +class ClassLevelCoversWithAttributeTest { + + /** + * Docblock. + */ + public function testMissingCoversTag() {} +} + +class ClassWithMethodsWithAttributesTest { + + /** + * Docblock. + */ + #[AttributeA] + #[AttributeB] + public function testMissingCoversTag() {} + + /** + * Docblock. + * + * @covers ::globalFunction + */ + #[AttributeA] + public function testHasCoversTag() {} +} diff --git a/Yoast/Tests/Commenting/TestsHaveCoversTagUnitTest.php b/Yoast/Tests/Commenting/TestsHaveCoversTagUnitTest.php index 201f9b65..02825311 100644 --- a/Yoast/Tests/Commenting/TestsHaveCoversTagUnitTest.php +++ b/Yoast/Tests/Commenting/TestsHaveCoversTagUnitTest.php @@ -22,8 +22,9 @@ class TestsHaveCoversTagUnitTest extends AbstractSniffUnitTest { */ public function getErrorList() { return [ - 59 => 1, - 88 => 1, + 59 => 1, + 88 => 1, + 126 => 1, ]; } From 2b0084659f667da95dcd001f442ca55dce1df707 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 28 Jul 2021 16:01:32 +0200 Subject: [PATCH 24/37] Commenting/TestsHaveCoversTag: report missing covers tag when docblock is missing The sniff as it was, contained an assumption that all methods should have a docblock, as per the WordPress Coding Standards. For a `@covers` tag to be found, a docblock is necessary, but as-it-was, if the docblock was not found, the sniff would bow out and defer to the WPCS Docs sniff to report the missing docblock. For the sniff to be properly independent of other sniffs, it should not bow out when there is no docblock, but report the missing `@covers` tag. Fixed now. Includes adjusting the expected result for one of the existing unit tests (which already covered this case). --- .../Commenting/TestsHaveCoversTagSniff.php | 59 ++++++++++--------- .../Commenting/TestsHaveCoversTagUnitTest.php | 1 + 2 files changed, 33 insertions(+), 27 deletions(-) diff --git a/Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php b/Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php index fd27ac11..0c9793f5 100644 --- a/Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php +++ b/Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php @@ -155,32 +155,28 @@ protected function process_function( File $phpcsFile, $stackPtr ) { break; } - if ( $tokens[ $commentEnd ]['code'] !== \T_DOC_COMMENT_CLOSE_TAG - || $tokens[ $commentEnd ]['line'] !== ( $tokens[ $stackPtr ]['line'] - 1 ) - ) { - // Function without (proper) docblock. Not our concern. - return; - } - - $commentStart = $tokens[ $commentEnd ]['comment_opener']; - $foundTest = false; $foundCovers = false; $foundCoversNothing = false; - foreach ( $tokens[ $commentStart ]['comment_tags'] as $tag ) { - if ( $tokens[ $tag ]['content'] === '@test' ) { - $foundTest = true; - continue; - } - if ( $tokens[ $tag ]['content'] === '@covers' ) { - $foundCovers = true; - continue; - } + if ( $tokens[ $commentEnd ]['code'] === \T_DOC_COMMENT_CLOSE_TAG ) { + $commentStart = $tokens[ $commentEnd ]['comment_opener']; - if ( $tokens[ $tag ]['content'] === '@coversNothing' ) { - $foundCoversNothing = true; - continue; + foreach ( $tokens[ $commentStart ]['comment_tags'] as $tag ) { + if ( $tokens[ $tag ]['content'] === '@test' ) { + $foundTest = true; + continue; + } + + if ( $tokens[ $tag ]['content'] === '@covers' ) { + $foundCovers = true; + continue; + } + + if ( $tokens[ $tag ]['content'] === '@coversNothing' ) { + $foundCoversNothing = true; + continue; + } } } @@ -195,11 +191,20 @@ protected function process_function( File $phpcsFile, $stackPtr ) { return; } - $phpcsFile->addError( - 'Each test function should have at least one @covers tag annotating which class/method/function is being tested. Tag missing for function %s()', - $stackPtr, - 'Missing', - [ $name ] - ); + $msg = 'Each test function should have at least one @covers tag annotating which class/method/function is being tested.'; + $data = [ $name ]; + + if ( $tokens[ $commentEnd ]['code'] === \T_DOC_COMMENT_CLOSE_TAG ) { + $msg .= ' Tag missing for function %s().'; + $code = 'Missing'; + $data = [ $name ]; + } + else { + $msg .= ' Test function %s() does not have a docblock with a @covers tag.'; + $code = 'NoDocblock'; + $data = [ $name ]; + } + + $phpcsFile->addError( $msg, $stackPtr, $code, $data ); } } diff --git a/Yoast/Tests/Commenting/TestsHaveCoversTagUnitTest.php b/Yoast/Tests/Commenting/TestsHaveCoversTagUnitTest.php index 02825311..58cc10e6 100644 --- a/Yoast/Tests/Commenting/TestsHaveCoversTagUnitTest.php +++ b/Yoast/Tests/Commenting/TestsHaveCoversTagUnitTest.php @@ -22,6 +22,7 @@ class TestsHaveCoversTagUnitTest extends AbstractSniffUnitTest { */ public function getErrorList() { return [ + 49 => 1, 59 => 1, 88 => 1, 126 => 1, From 6d9885e4e3dc4514c400970735b0cd7c2f725690 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 28 Jul 2021 16:13:17 +0200 Subject: [PATCH 25/37] Commenting/TestsHaveCoversTag: allow for different test class naming conventions PHPUnit has strict naming conventions for test methods: * The method has to start with `test`; * Or have a `@test` docblock annotation. However, for test classes, no such naming conventions exist. though it is customary to have the class either prefixed or suffixed with the word `Test`. Additionally, it is customary for abstract classes in test suites to be suffixed with `TestCase`. The `Yoast.Commenting.TestsHaveCoversTag` sniff only took test classes suffixed with `Test` into account, which complies with the naming conventions adhered to in most Yoast projects. However, for the sniff to be code-style independent, other conventions, like using a `Test` prefix (WordPress Core) should also be taken into account. Similarly, abstract test classes can contain concrete tests - which would then be run for each child test class -, so this sniff should also allow for a `TestCase` suffix and search those classes for test methods. Fixed now. Includes additional unit tests. --- .../Commenting/TestsHaveCoversTagSniff.php | 5 ++++- .../Commenting/TestsHaveCoversTagUnitTest.inc | 16 ++++++++++++++++ .../Commenting/TestsHaveCoversTagUnitTest.php | 2 ++ 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php b/Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php index 0c9793f5..fd7f979f 100644 --- a/Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php +++ b/Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php @@ -64,7 +64,10 @@ protected function process_class( File $phpcsFile, $stackPtr ) { $tokens = $phpcsFile->getTokens(); $name = $phpcsFile->getDeclarationName( $stackPtr ); - if ( \substr( $name, -4 ) !== 'Test' ) { + if ( \substr( $name, -4 ) !== 'Test' + && \substr( $name, -8 ) !== 'TestCase' + && \substr( $name, 0, 4 ) !== 'Test' + ) { // Not a test class. if ( isset( $tokens[ $stackPtr ]['scope_closer'] ) ) { // No need to examine the methods in this class. diff --git a/Yoast/Tests/Commenting/TestsHaveCoversTagUnitTest.inc b/Yoast/Tests/Commenting/TestsHaveCoversTagUnitTest.inc index 7bc15554..c5ed3dbf 100644 --- a/Yoast/Tests/Commenting/TestsHaveCoversTagUnitTest.inc +++ b/Yoast/Tests/Commenting/TestsHaveCoversTagUnitTest.inc @@ -133,3 +133,19 @@ class ClassWithMethodsWithAttributesTest { #[AttributeA] public function testHasCoversTag() {} } + +class TestPrefixNotSuffix { + + /** + * Docblock. + */ + function testSomething() {} +} + +abstract class ContainsTestMethodsTestCase { + + /** + * Docblock - this concrete test should have a covers tag. + */ + function testSomething() {} +} diff --git a/Yoast/Tests/Commenting/TestsHaveCoversTagUnitTest.php b/Yoast/Tests/Commenting/TestsHaveCoversTagUnitTest.php index 58cc10e6..123a5d30 100644 --- a/Yoast/Tests/Commenting/TestsHaveCoversTagUnitTest.php +++ b/Yoast/Tests/Commenting/TestsHaveCoversTagUnitTest.php @@ -26,6 +26,8 @@ public function getErrorList() { 59 => 1, 88 => 1, 126 => 1, + 142 => 1, + 150 => 1, ]; } From 7a31c3b3f1212973e835769cf6288e134ba8a421 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 28 Jul 2021 16:21:23 +0200 Subject: [PATCH 26/37] Commenting/TestsHaveCoversTag: allow for abstract test methods in TestCases Now, `TestCase` classes are taken into account, we also need to account for the possibility of those containing `abstract` test methods. In that case, no `@covers` tag is needed as the method is not implemented (yet) and the `@covers` tag should only be added with the concrete implementation in the child class. Fixed now. Includes additional unit test. --- Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php | 6 ++++++ Yoast/Tests/Commenting/TestsHaveCoversTagUnitTest.inc | 5 +++++ 2 files changed, 11 insertions(+) diff --git a/Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php b/Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php index fd7f979f..4531a183 100644 --- a/Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php +++ b/Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php @@ -189,6 +189,12 @@ protected function process_function( File $phpcsFile, $stackPtr ) { return; } + $method_props = $phpcsFile->getMethodProperties( $stackPtr ); + if ( $method_props['is_abstract'] === true ) { + // Abstract test method, not implemented. + return; + } + if ( $foundCovers === true || $foundCoversNothing === true ) { // Docblock contains one or more @covers tags. return; diff --git a/Yoast/Tests/Commenting/TestsHaveCoversTagUnitTest.inc b/Yoast/Tests/Commenting/TestsHaveCoversTagUnitTest.inc index c5ed3dbf..63123a2b 100644 --- a/Yoast/Tests/Commenting/TestsHaveCoversTagUnitTest.inc +++ b/Yoast/Tests/Commenting/TestsHaveCoversTagUnitTest.inc @@ -148,4 +148,9 @@ abstract class ContainsTestMethodsTestCase { * Docblock - this concrete test should have a covers tag. */ function testSomething() {} + + /** + * Docblock - this abstract method can be ignored. + */ + abstract function testToBeImplementedInChildClass(); } From 7a8f54720a315d4bf803f46d34d6272c1ca3d511 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 28 Jul 2021 16:41:26 +0200 Subject: [PATCH 27/37] NamingConventions/ObjectNameDepth: prevent false positives with attributes PHP 8.0+ attributes can be placed between a docblock and the class/function declaration it applies to. The `Yoast.NamingConventions.ObjectNameDepth` sniff did not yet take this into account. This could result in false positives when a class is marked as `@deprecated`. Fixed now. Includes unit tests. --- .../ObjectNameDepthSniff.php | 19 +++++++++++++++++-- .../ObjectNameDepthUnitTest.2.inc | 17 +++++++++++++++++ .../ObjectNameDepthUnitTest.php | 9 +++++---- 3 files changed, 39 insertions(+), 6 deletions(-) diff --git a/Yoast/Sniffs/NamingConventions/ObjectNameDepthSniff.php b/Yoast/Sniffs/NamingConventions/ObjectNameDepthSniff.php index a03dd77e..ac58f40b 100644 --- a/Yoast/Sniffs/NamingConventions/ObjectNameDepthSniff.php +++ b/Yoast/Sniffs/NamingConventions/ObjectNameDepthSniff.php @@ -110,13 +110,28 @@ public function process_token( $stackPtr ) { } // Check if the class is deprecated. - $find = [ + $ignore = [ \T_ABSTRACT => \T_ABSTRACT, \T_FINAL => \T_FINAL, \T_WHITESPACE => \T_WHITESPACE, ]; - $comment_end = $this->phpcsFile->findPrevious( $find, ( $stackPtr - 1 ), null, true ); + $comment_end = $stackPtr; + for ( $comment_end = ( $stackPtr - 1 ); $comment_end >= 0; $comment_end-- ) { + if ( isset( $ignore[ $this->tokens[ $comment_end ]['code'] ] ) === true ) { + continue; + } + + if ( $this->tokens[ $comment_end ]['code'] === \T_ATTRIBUTE_END + && isset( $this->tokens[ $comment_end ]['attribute_opener'] ) === true + ) { + $comment_end = $this->tokens[ $comment_end ]['attribute_opener']; + continue; + } + + break; + } + if ( $this->tokens[ $comment_end ]['code'] === \T_DOC_COMMENT_CLOSE_TAG ) { // Only check if the class has a docblock. $comment_start = $this->tokens[ $comment_end ]['comment_opener']; diff --git a/Yoast/Tests/NamingConventions/ObjectNameDepthUnitTest.2.inc b/Yoast/Tests/NamingConventions/ObjectNameDepthUnitTest.2.inc index ddd50afd..1d378dc2 100644 --- a/Yoast/Tests/NamingConventions/ObjectNameDepthUnitTest.2.inc +++ b/Yoast/Tests/NamingConventions/ObjectNameDepthUnitTest.2.inc @@ -64,6 +64,23 @@ class Active_Class_With_Too_Long_Class_Name {} // Error. */ class Deprecated_Class_With_Too_Long_Class_Name {} // OK. +/** + * Class description, no @deprecated tag. + * + * @since x.x.x + */ +#[Some_Attribute] +class Active_Class_With_Attribute_With_Too_Long_Class_Name {} // Error. + +/** + * Class description. + * + * @deprecated x.x.x + */ +#[Attribute_One] +#[Attribute_Two] +class Deprecated_Class_With_Attribute_With_Too_Long_Class_Name {} // OK. + /* * Allow for a `_Test` suffix in classes within the unit test suite. */ diff --git a/Yoast/Tests/NamingConventions/ObjectNameDepthUnitTest.php b/Yoast/Tests/NamingConventions/ObjectNameDepthUnitTest.php index ebb91041..7c4f4191 100644 --- a/Yoast/Tests/NamingConventions/ObjectNameDepthUnitTest.php +++ b/Yoast/Tests/NamingConventions/ObjectNameDepthUnitTest.php @@ -34,10 +34,11 @@ public function getErrorList( $testFile = '' ) { 42 => 1, 43 => 1, 58 => 1, - 70 => 1, - 75 => 1, - 78 => 1, - 79 => 1, + 73 => 1, + 87 => 1, + 92 => 1, + 95 => 1, + 96 => 1, ]; default: From 789efe3d660d4fac497b6930f2bab54d6b45a37b Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 28 Jul 2021 16:50:09 +0200 Subject: [PATCH 28/37] NamingConventions/ObjectNameDepth: disregard underscore prefixed class names While unconventional, underscore prefixed class names are allowed in PHP and should not affect the object name word count. Fixed now. Includes unit tests. --- .../ObjectNameDepthSniff.php | 2 ++ .../ObjectNameDepthUnitTest.2.inc | 6 +++++ .../ObjectNameDepthUnitTest.php | 25 ++++++++++--------- 3 files changed, 21 insertions(+), 12 deletions(-) diff --git a/Yoast/Sniffs/NamingConventions/ObjectNameDepthSniff.php b/Yoast/Sniffs/NamingConventions/ObjectNameDepthSniff.php index ac58f40b..ec5cd308 100644 --- a/Yoast/Sniffs/NamingConventions/ObjectNameDepthSniff.php +++ b/Yoast/Sniffs/NamingConventions/ObjectNameDepthSniff.php @@ -85,6 +85,8 @@ public function process_token( $stackPtr ) { return; } + $object_name = \ltrim( $object_name, '_' ); + $parts = \explode( '_', $object_name ); $part_count = \count( $parts ); diff --git a/Yoast/Tests/NamingConventions/ObjectNameDepthUnitTest.2.inc b/Yoast/Tests/NamingConventions/ObjectNameDepthUnitTest.2.inc index 1d378dc2..2518dfae 100644 --- a/Yoast/Tests/NamingConventions/ObjectNameDepthUnitTest.2.inc +++ b/Yoast/Tests/NamingConventions/ObjectNameDepthUnitTest.2.inc @@ -97,3 +97,9 @@ class Three_Word_Name_Mock {} // Error. class Three_Word_Name_Double extends Three_Word_Name {} // OK. class Three_Word_Name_Mock extends Some_Class {} // OK. + +/* + * Make sure underscore prefixed names have a correct count. + */ +class __Three_Word_Name {} // OK. +class __Four_Four_Word_Name {} // Error. diff --git a/Yoast/Tests/NamingConventions/ObjectNameDepthUnitTest.php b/Yoast/Tests/NamingConventions/ObjectNameDepthUnitTest.php index 7c4f4191..4a42487f 100644 --- a/Yoast/Tests/NamingConventions/ObjectNameDepthUnitTest.php +++ b/Yoast/Tests/NamingConventions/ObjectNameDepthUnitTest.php @@ -27,18 +27,19 @@ public function getErrorList( $testFile = '' ) { switch ( $testFile ) { case 'ObjectNameDepthUnitTest.2.inc': return [ - 21 => 1, - 22 => 1, - 23 => 1, - 33 => 1, - 42 => 1, - 43 => 1, - 58 => 1, - 73 => 1, - 87 => 1, - 92 => 1, - 95 => 1, - 96 => 1, + 21 => 1, + 22 => 1, + 23 => 1, + 33 => 1, + 42 => 1, + 43 => 1, + 58 => 1, + 73 => 1, + 87 => 1, + 92 => 1, + 95 => 1, + 96 => 1, + 105 => 1, ]; default: From 0c2420a6e6c791a314fcb356cf2aea48e74ee954 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 28 Jul 2021 17:06:28 +0200 Subject: [PATCH 29/37] NamingConventions/ObjectNameDepth: allow for class names in CamelCaps The WP Coding Standards and by extension YoastCS, expects `Snake_Caps` class names. For non-WP projects, however, `CamelCaps` class names are more common. This change makes this sniff more code-style independent by allowing for `CamelCaps` class names when examining the word count. Includes unit tests. Note: one unit test is failing due to an acronym being used in the class name. This should be fixed at a later point in time. Also note: as PHPCS has strict directory structure conventions, it is not possible for sniffs to be placed in a deeper subdirectory/namespace (which is what should normally be done for violations against this sniff). With that in mind, this sniff is disabled for this repository. --- .phpcs.xml.dist | 3 +++ .../Sniffs/NamingConventions/ObjectNameDepthSniff.php | 9 +++++++-- .../NamingConventions/ObjectNameDepthUnitTest.2.inc | 11 +++++++++++ .../NamingConventions/ObjectNameDepthUnitTest.php | 4 ++++ 4 files changed, 25 insertions(+), 2 deletions(-) diff --git a/.phpcs.xml.dist b/.phpcs.xml.dist index d76353ec..7f70a828 100644 --- a/.phpcs.xml.dist +++ b/.phpcs.xml.dist @@ -48,6 +48,9 @@ + + + diff --git a/Yoast/Sniffs/NamingConventions/ObjectNameDepthSniff.php b/Yoast/Sniffs/NamingConventions/ObjectNameDepthSniff.php index ec5cd308..872ff12b 100644 --- a/Yoast/Sniffs/NamingConventions/ObjectNameDepthSniff.php +++ b/Yoast/Sniffs/NamingConventions/ObjectNameDepthSniff.php @@ -85,9 +85,14 @@ public function process_token( $stackPtr ) { return; } - $object_name = \ltrim( $object_name, '_' ); + $snakecase_object_name = \ltrim( $object_name, '_' ); - $parts = \explode( '_', $object_name ); + // Handle names which are potentially in CamelCaps. + if ( \strpos( $snakecase_object_name, '_' ) === false ) { + $snakecase_object_name = self::get_snake_case_name_suggestion( $snakecase_object_name ); + } + + $parts = \explode( '_', $snakecase_object_name ); $part_count = \count( $parts ); /* diff --git a/Yoast/Tests/NamingConventions/ObjectNameDepthUnitTest.2.inc b/Yoast/Tests/NamingConventions/ObjectNameDepthUnitTest.2.inc index 2518dfae..62ceeef6 100644 --- a/Yoast/Tests/NamingConventions/ObjectNameDepthUnitTest.2.inc +++ b/Yoast/Tests/NamingConventions/ObjectNameDepthUnitTest.2.inc @@ -103,3 +103,14 @@ class Three_Word_Name_Mock extends Some_Class {} // OK. */ class __Three_Word_Name {} // OK. class __Four_Four_Word_Name {} // Error. + +/* + * Make sure CamelCaps class names are also handled by this sniff. + */ +class ThreePartName {} +interface MyInterfaceName {} +trait SomeACRONYMName {} // Error - false positive, this will be fixed in a later iteration on this sniff. + +class TooLongClassName {} // Error. +interface ThisInterfaceNameIsTooLong {} // Error. +trait TraitNameIsTooLong {} // Error. diff --git a/Yoast/Tests/NamingConventions/ObjectNameDepthUnitTest.php b/Yoast/Tests/NamingConventions/ObjectNameDepthUnitTest.php index 4a42487f..b12f9bfb 100644 --- a/Yoast/Tests/NamingConventions/ObjectNameDepthUnitTest.php +++ b/Yoast/Tests/NamingConventions/ObjectNameDepthUnitTest.php @@ -40,6 +40,10 @@ public function getErrorList( $testFile = '' ) { 95 => 1, 96 => 1, 105 => 1, + 112 => 1, // False positive due to acronym. + 114 => 1, + 115 => 1, + 116 => 1, ]; default: From e50409866d850daf5a111a0c813925f0f948cbef Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 6 Aug 2021 15:01:48 +0200 Subject: [PATCH 30/37] Ruleset: update minimum_supported_version to WP 5.7 ... after the release of WP 5.8. --- Yoast/ruleset.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Yoast/ruleset.xml b/Yoast/ruleset.xml index c8b6d019..8e61b473 100644 --- a/Yoast/ruleset.xml +++ b/Yoast/ruleset.xml @@ -31,7 +31,7 @@ Ref: https://github.com/WordPress/WordPress-Coding-Standards/wiki/Customizable-sniff-properties#minimum-wp-version-to-check-for-usage-of-deprecated-functions-classes-and-function-parameters --> - + From 331384915a30dfefa39e00137b03fd28ab6733b9 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 28 Jul 2021 12:25:37 +0200 Subject: [PATCH 31/37] Commenting/CodeCoverageIgnoreDeprecated: prevent false negatives with attributes PHP 8.0+ attributes can be placed between a docblock and the function declaration it applies to. The `Yoast.Commenting.CodeCoverageIgnoreDeprecated` sniff did not yet take this into account. This would result in a false negatives as the sniff would bow out for not finding a docblock, even when the docblock contained a `@deprecated` tag. Fixed now. Includes unit tests. --- .../CodeCoverageIgnoreDeprecatedSniff.php | 28 +++++++++++++------ .../CodeCoverageIgnoreDeprecatedUnitTest.inc | 14 ++++++++++ ...CoverageIgnoreDeprecatedUnitTest.inc.fixed | 14 ++++++++++ .../CodeCoverageIgnoreDeprecatedUnitTest.php | 1 + 4 files changed, 48 insertions(+), 9 deletions(-) diff --git a/Yoast/Sniffs/Commenting/CodeCoverageIgnoreDeprecatedSniff.php b/Yoast/Sniffs/Commenting/CodeCoverageIgnoreDeprecatedSniff.php index 289098ed..026045a2 100644 --- a/Yoast/Sniffs/Commenting/CodeCoverageIgnoreDeprecatedSniff.php +++ b/Yoast/Sniffs/Commenting/CodeCoverageIgnoreDeprecatedSniff.php @@ -39,17 +39,27 @@ public function register() { public function process( File $phpcsFile, $stackPtr ) { $tokens = $phpcsFile->getTokens(); - $find = Tokens::$methodPrefixes; - $find[] = \T_WHITESPACE; - $commentEnd = $stackPtr; - do { - $commentEnd = $phpcsFile->findPrevious( $find, ( $commentEnd - 1 ), null, true ); - } while ( $tokens[ $commentEnd ]['line'] === $tokens[ $stackPtr ]['line'] ); + $ignore = Tokens::$methodPrefixes; + $ignore[ \T_WHITESPACE ] = \T_WHITESPACE; - if ( $tokens[ $commentEnd ]['code'] !== \T_DOC_COMMENT_CLOSE_TAG - || $tokens[ $commentEnd ]['line'] !== ( $tokens[ $stackPtr ]['line'] - 1 ) - ) { + for ( $commentEnd = ( $stackPtr - 1 ); $commentEnd >= 0; $commentEnd-- ) { + if ( isset( $ignore[ $tokens[ $commentEnd ]['code'] ] ) === true ) { + continue; + } + + if ( $tokens[ $commentEnd ]['code'] === \T_ATTRIBUTE_END + && isset( $tokens[ $commentEnd ]['attribute_opener'] ) === true + ) { + $commentEnd = $tokens[ $commentEnd ]['attribute_opener']; + continue; + } + + break; + } + + + if ( $tokens[ $commentEnd ]['code'] !== \T_DOC_COMMENT_CLOSE_TAG ) { // Function without (proper) docblock. Not our concern. return; } diff --git a/Yoast/Tests/Commenting/CodeCoverageIgnoreDeprecatedUnitTest.inc b/Yoast/Tests/Commenting/CodeCoverageIgnoreDeprecatedUnitTest.inc index b686e7a0..9d064623 100644 --- a/Yoast/Tests/Commenting/CodeCoverageIgnoreDeprecatedUnitTest.inc +++ b/Yoast/Tests/Commenting/CodeCoverageIgnoreDeprecatedUnitTest.inc @@ -53,4 +53,18 @@ class SomeThing { * @deprecated x.x */ public function missingCodeCoverageIgnore() {} + + /** + * @deprecated x.x + * @codeCoverageIgnore + */ + #[ReturnTypeWillChange] + private static function withAttributeCorrectDocblock() {} + + /** + * @deprecated x.x + */ + #[ReturnTypeWillChange] + #[AnotherAttribute] + public function withAttributesMissingCodeCoverageIgnore() {} } diff --git a/Yoast/Tests/Commenting/CodeCoverageIgnoreDeprecatedUnitTest.inc.fixed b/Yoast/Tests/Commenting/CodeCoverageIgnoreDeprecatedUnitTest.inc.fixed index a8a80c8d..0d85192e 100644 --- a/Yoast/Tests/Commenting/CodeCoverageIgnoreDeprecatedUnitTest.inc.fixed +++ b/Yoast/Tests/Commenting/CodeCoverageIgnoreDeprecatedUnitTest.inc.fixed @@ -53,4 +53,18 @@ class SomeThing { * @deprecated x.x */ public function missingCodeCoverageIgnore() {} + + /** + * @deprecated x.x + * @codeCoverageIgnore + */ + #[ReturnTypeWillChange] + private static function withAttributeCorrectDocblock() {} + + /** + * @deprecated x.x + */ + #[ReturnTypeWillChange] + #[AnotherAttribute] + public function withAttributesMissingCodeCoverageIgnore() {} } diff --git a/Yoast/Tests/Commenting/CodeCoverageIgnoreDeprecatedUnitTest.php b/Yoast/Tests/Commenting/CodeCoverageIgnoreDeprecatedUnitTest.php index e28989dc..83a050df 100644 --- a/Yoast/Tests/Commenting/CodeCoverageIgnoreDeprecatedUnitTest.php +++ b/Yoast/Tests/Commenting/CodeCoverageIgnoreDeprecatedUnitTest.php @@ -25,6 +25,7 @@ public function getErrorList() { 41 => 1, 50 => 1, 55 => 1, + 69 => 1, ]; } From 0cb187db762b98d1b2fb5a79c9e541eab21189c7 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Mon, 6 Sep 2021 11:02:50 +0200 Subject: [PATCH 32/37] CodeCoverageIgnoreDeprecated: add extra unit tests ... documenting the sniff's behaviour when there are inline comments related to attributes. --- .../CodeCoverageIgnoreDeprecatedUnitTest.inc | 21 +++++++++++++++++++ ...CoverageIgnoreDeprecatedUnitTest.inc.fixed | 21 +++++++++++++++++++ .../CodeCoverageIgnoreDeprecatedUnitTest.php | 1 + 3 files changed, 43 insertions(+) diff --git a/Yoast/Tests/Commenting/CodeCoverageIgnoreDeprecatedUnitTest.inc b/Yoast/Tests/Commenting/CodeCoverageIgnoreDeprecatedUnitTest.inc index 9d064623..d9165e9e 100644 --- a/Yoast/Tests/Commenting/CodeCoverageIgnoreDeprecatedUnitTest.inc +++ b/Yoast/Tests/Commenting/CodeCoverageIgnoreDeprecatedUnitTest.inc @@ -67,4 +67,25 @@ class SomeThing { #[ReturnTypeWillChange] #[AnotherAttribute] public function withAttributesMissingCodeCoverageIgnore() {} + + /** + * Docblock which can't be found as the inline comments for the attribute confuse the sniff. + */ + #[fietsbel] // added a fietsbel decorator + public function unreachableDocblockTrailingComment() {} + + /** + * Docblock which can't be found as the inline comments for the attribute confuse the sniff. + */ + // I added a fietsbel decorator + #[fietsbel] + public function unreachableDocblockInlineCommentAboveAttribute() {} + + /** + * Docblock which is found as the comment is within the attribute itself. + * + * @deprecated x.x + */ + #[fietsbel /*comment*/] + public function inlineCommentWithinAttribute() {} } diff --git a/Yoast/Tests/Commenting/CodeCoverageIgnoreDeprecatedUnitTest.inc.fixed b/Yoast/Tests/Commenting/CodeCoverageIgnoreDeprecatedUnitTest.inc.fixed index 0d85192e..572e5dca 100644 --- a/Yoast/Tests/Commenting/CodeCoverageIgnoreDeprecatedUnitTest.inc.fixed +++ b/Yoast/Tests/Commenting/CodeCoverageIgnoreDeprecatedUnitTest.inc.fixed @@ -67,4 +67,25 @@ class SomeThing { #[ReturnTypeWillChange] #[AnotherAttribute] public function withAttributesMissingCodeCoverageIgnore() {} + + /** + * Docblock which can't be found as the inline comments for the attribute confuse the sniff. + */ + #[fietsbel] // added a fietsbel decorator + public function unreachableDocblockTrailingComment() {} + + /** + * Docblock which can't be found as the inline comments for the attribute confuse the sniff. + */ + // I added a fietsbel decorator + #[fietsbel] + public function unreachableDocblockInlineCommentAboveAttribute() {} + + /** + * Docblock which is found as the comment is within the attribute itself. + * + * @deprecated x.x + */ + #[fietsbel /*comment*/] + public function inlineCommentWithinAttribute() {} } diff --git a/Yoast/Tests/Commenting/CodeCoverageIgnoreDeprecatedUnitTest.php b/Yoast/Tests/Commenting/CodeCoverageIgnoreDeprecatedUnitTest.php index 83a050df..225d1cae 100644 --- a/Yoast/Tests/Commenting/CodeCoverageIgnoreDeprecatedUnitTest.php +++ b/Yoast/Tests/Commenting/CodeCoverageIgnoreDeprecatedUnitTest.php @@ -26,6 +26,7 @@ public function getErrorList() { 50 => 1, 55 => 1, 69 => 1, + 90 => 1, ]; } From 7b344c3a0569aa4b9acd741d1213be451d0c2313 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Tue, 3 Aug 2021 16:03:17 +0200 Subject: [PATCH 33/37] TestsHaveCoversTag: minor tidying up `$data` is the same for both conditions and already defined above the condition block, so can be removed here. --- Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php b/Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php index 4531a183..b3b102e8 100644 --- a/Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php +++ b/Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php @@ -206,12 +206,10 @@ protected function process_function( File $phpcsFile, $stackPtr ) { if ( $tokens[ $commentEnd ]['code'] === \T_DOC_COMMENT_CLOSE_TAG ) { $msg .= ' Tag missing for function %s().'; $code = 'Missing'; - $data = [ $name ]; } else { $msg .= ' Test function %s() does not have a docblock with a @covers tag.'; $code = 'NoDocblock'; - $data = [ $name ]; } $phpcsFile->addError( $msg, $stackPtr, $code, $data ); From 337483c86666d3399056cb7d48f926a5e9ec178a Mon Sep 17 00:00:00 2001 From: jrfnl Date: Tue, 7 Sep 2021 00:03:16 +0200 Subject: [PATCH 34/37] Composer: update Parallel Lint Update the Parallel Lint `dev` dependency to use v `1.3.1` as a minimum. The new release has improved PHP 8.1 support. Ref: https://github.com/php-parallel-lint/PHP-Parallel-Lint/releases/tag/v1.3.1 --- composer.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/composer.json b/composer.json index 25f2be0e..cc6ee56d 100644 --- a/composer.json +++ b/composer.json @@ -32,7 +32,7 @@ "phpcompatibility/php-compatibility": "^9.3.5", "roave/security-advisories": "dev-master", "phpunit/phpunit": "^4.0 || ^5.0 || ^6.0 || ^7.0", - "php-parallel-lint/php-parallel-lint": "^1.3", + "php-parallel-lint/php-parallel-lint": "^1.3.1", "php-parallel-lint/php-console-highlighter": "^0.5", "phpcsstandards/phpcsdevtools": "^1.0" }, From 42d1bd7daea92aa795654d0c3d1a8072235d44ee Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 22 Sep 2021 11:17:00 +0200 Subject: [PATCH 35/37] Composer: provide Parallel Lint via YoastCS This moves the dependency on PHP Parallel Lint and the associated Console Highlighter library from `require-dev` to `require` as most Yoast projects already use and have a dependency on PHP Parallel Lint. Providing PHP Parallel Lint via YoastCS means that all projects using YoastCS will automatically inherit the dependency and that the version constraints will only need to be managed in one place as of now. Includes adding a section about PHP Parallel Lint to the README. --- README.md | 24 ++++++++++++++++++++++++ composer.json | 6 +++--- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 892e221b..295626de 100644 --- a/README.md +++ b/README.md @@ -24,6 +24,30 @@ composer require --dev yoast/yoastcs:^2.0 Composer will automatically install dependencies and register the YoastCS and other external standards with PHP_CodeSniffer. +## Tools provided via YoastCS + +* PHP Parallel Lint +* PHP_CodeSniffer and select standards for PHP_CodeSniffer, including a number of Yoast native sniffs. + + +## PHP Parallel Lint + +[PHP Parallel Lint](https://github.com/php-parallel-lint/PHP-Parallel-Lint/) is a tool to lint PHP files against parse errors. + +PHP Parallel Lint does not use a configuration file, so [command-line options](https://github.com/php-parallel-lint/PHP-Parallel-Lint/#command-line-options) need to be passed to configure what files to scan. + +It is best practice within the Yoast projects, to add a script to the `composer.json` file which encapsules the command with the appropriate command-line options to ensure that running the tool will yield the same results each time. + +Typically, (a variation on) the following snippet would be added to the `composer.json` file for a project: +```json + "scripts" : { + "lint": [ + "@php ./vendor/php-parallel-lint/php-parallel-lint/parallel-lint . -e php --exclude vendor --exclude .git" + ] + } +``` + + ## PHP Code Sniffer Set of [PHP_CodeSniffer](https://github.com/squizlabs/PHP_CodeSniffer) rules. diff --git a/composer.json b/composer.json index cc6ee56d..6b6febd8 100644 --- a/composer.json +++ b/composer.json @@ -26,14 +26,14 @@ "squizlabs/php_codesniffer": "^3.6.0", "wp-coding-standards/wpcs": "^2.3.0", "phpcompatibility/phpcompatibility-wp": "^2.1.0", - "dealerdirect/phpcodesniffer-composer-installer": "^0.5 || ^0.6.2 || ^0.7" + "dealerdirect/phpcodesniffer-composer-installer": "^0.5 || ^0.6.2 || ^0.7", + "php-parallel-lint/php-parallel-lint": "^1.3.1", + "php-parallel-lint/php-console-highlighter": "^0.5.0" }, "require-dev": { "phpcompatibility/php-compatibility": "^9.3.5", "roave/security-advisories": "dev-master", "phpunit/phpunit": "^4.0 || ^5.0 || ^6.0 || ^7.0", - "php-parallel-lint/php-parallel-lint": "^1.3.1", - "php-parallel-lint/php-console-highlighter": "^0.5", "phpcsstandards/phpcsdevtools": "^1.0" }, "minimum-stability": "dev", From 07aa2af3cb8e76ca4c0924b0d1569e862a314f78 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 17 Jun 2021 21:49:48 +0200 Subject: [PATCH 36/37] Add initial CONTRIBUTING file ... with information about how to test PRs to this repository. --- .github/CONTRIBUTING.md | 116 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 116 insertions(+) create mode 100644 .github/CONTRIBUTING.md diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md new file mode 100644 index 00000000..cd7c7191 --- /dev/null +++ b/.github/CONTRIBUTING.md @@ -0,0 +1,116 @@ +# Contributing to YoastCS + + +## Acceptance testing + +There are various ways to acceptance test a sniff change: +1. Based on the tests included with the change. +2. Based on a code sample with "good" and "bad" code. +3. Based on an existing plugin code base. + +Note: the above list is ordered by difficulty. Testing via method 1 will generally be easiest, method 3 most complicated. + +In the fold-outs below are step by step instructions on how to acceptance test via each of these methods. + +--- +
+ Step by step: acceptance test based on the integration tests included with the change + +1. Check out a local copy of this repo using your favourite git tool. +2. Check out the branch containing the change. +3. Run `composer install`. +4. Revert the changes to the sniff file (do not commit!), but leave the changes to the test file(s) in place. +5. Run the tests using `composer test`. The tests should fail in the expected places (see the commit message of the change for what to expect and the line numbers to expect errors and warnings on in the `*Test.php` file for the sniff). +6. Reset the sniff file to its committed state. +7. Run the tests again using `composer test`. The tests should now pass. + +
+ +
+ Step by step: acceptance test based on a code sample with "good" and "bad" code + +1. Check out a local copy of this repo using your favourite git tool. +2. Check out the `develop` branch. +3. Run `composer install`. +4. Create a simple PHP file with some code which should be flagged and some code which shouldn't be flagged. +5. Save this file in a `temp` subdirectory in your local YoastCS copy and make sure to add this `temp` directory to your `.git/info/exclude` file. + Do not commmit this file! +6. Run `vendor/bin/phpcs -ps ./temp/yourfilename.php --standard=Yoast --report=full,source`. + This should fail in the expected places (see the commit message of the change for what to expect), i.e. things not getting flagged which should get flagged or things getting flagged, which shouldn't get flagged. + Pro-tip: if the test is just for a single sniff, you can limit the output to just that sniff by adding `--sniffs=Yoast.Category.SniffName` (replace `Category` and `SniffName` with the applicable names). +7. Check out the branch containing the change. +8. Run the command from [6] again. The sniff should now flag things correctly. + +If the sniff change includes changes to/adding of auto-fixers, the fixing should also be tested. + +9. Check out the `develop` branch again. +10. Run `vendor/bin/phpcbf -ps ./temp/yourfilename.php --standard=Yoast --suffix=.fixed`. + This will create a copy of your test file named `yourfilename.php.fixed`. + Pro-tip: you can use the `--sniffs=Yoast.Category.SniffName` addition to the command for this step too. +11. Examine the fixes made and expect them to be incorrect. +12. Check out the branch containing the change. +13. Repeat step 10 and 11. The sniff should now fix things correctly. + +If you like, you can now delete the `temp` directory and your test file(s), but leaving them in place for the next round of testing shouldn't do any harm. + +
+ +
+ Step by step: acceptance test based on a existing plugin code base + +1. Create a simple PHP file with some code which should be flagged and some code which shouldn't be flagged (or adjust an existing file). +2. Save this file somewhere in the plugin. + Do not commmit this file (or the changes made to an existing file)! +3. Run `vendor/bin/phpcs --report=full,source`. + This should fail in the expected places (see the commit message of the change for what to expect), i.e. things not getting flagged which should get flagged or things getting flagged, which shouldn't get flagged. + Pro-tip: if the test is just for a single sniff, you can limit the output to just that sniff by adding `--sniffs=Yoast.Category.SniffName` (replace `Category` and `SniffName` with the applicable names). +4. Run `composer update yoast/yoastcs:"dev-featurebranchname"` from the root of the plugin in which you are testing the change. + I.e. a branch in YoastCS named `feature/sniffname-change` should be referenced as `dev-feature/sniffname-change`. + Again: do not commit this change to the `composer.json` and `composer.lock` files! +5. Run the command from [3] again. The sniff should now flag things correctly. + +If the sniff change includes changes to/adding of auto-fixers, the fixing should also be tested. + +6. Reset the changes to the `composer.json` and `composer.lock` file, but do **not** reset the changes to your test file. +7. Run `composer install`. +8. Run `vendor/bin/phpcbf --suffix=.fixed`. + This will create a copy of your test file with `fixed` at the end. So a file originally named `src/admin/classname.php` will now have a second copy named `src/admin/classname.php.fixed`. + Pro-tip: you can use the `--sniffs=Yoast.Category.SniffName` addition to the command for this step too. +9. Examine the fixes made and expect them to be incorrect. +10. Run `composer update yoast/yoastcs:"dev-featurebranchname"` again from the root of the plugin in which you are testing the change. + Again: do not commit this change to the `composer.json` and `composer.lock` files! +11. Repeat step 8 and 9. The sniff should now fix things correctly. + +Clean up: make sure to reset all the file changes made during testing! + +
+ +--- + +### Gotcha's when testing sniff changes + +#### Public properties + +Some sniffs will behave differently based on the value of the sniff's `public` properties. +These properties can be set [from a custom ruleset](https://github.com/squizlabs/PHP_CodeSniffer/wiki/Annotated-Ruleset) or in a test situation, in-line using `// phpcs:set Yoast.Category.SniffName propertyName propertyValue`. + +If the results you are getting when testing are different from what you expected, first thing to do is to check whether the sniff has `public` properties, what those properties are set to (in your custom ruleset or in a test file in-line) and whether that setting is interferring with the results. + +#### Severity + +The default severity level in PHPCS to show notices is `5`. + +Occassionaly (not very often), certain notices will be given a lower severity to prevent "noise" in the CI, while still allowing for something to be (manually) checked. + +If the results you are getting when testing are different from what you expected, check if the `$severity` is set for the error message you are missing (fifth parameter in a call to `addError()` or `addWarning()`) and if so, add `--severity=1` to the command line command you are running to get those messages to show up. + + +### For regular sniff testers + +If you regularly test sniffs and/or want to contribute to YoastCS with sniff additions or sniff changes, all the setup needed for the above testing methods can get tedious. + +In that case, I'd recommend adding the `vendor/squizlabs/php_codesniffer/bin` directory within `YoastCS` to your system path. This should make the `phpcs` and `phpcbf` commands available anywhere on your system. You can then use those commands anywhere and whichever branch you have checked-out in your YoastCS clone will be used to run the requested scans. + +In effect, this means that if you run a scan using `phpcs` within a plugin directory, the ruleset of that plugin will still be respected, but instead of using the YoastCS install in the `vendor` directory of the plugin, your cloned YoastCS install will be used. + +If you also (want to) contribute to WordPressCS, PHPCS and/or other standards, contact [@jrfnl](http://github.com/jrfnl/) to discuss an even more robust setup. From cc5dcf9073217ed0ffed9d13652263c0db13301f Mon Sep 17 00:00:00 2001 From: jrfnl Date: Tue, 7 Sep 2021 00:19:23 +0200 Subject: [PATCH 37/37] Changelog for the YoastCS 2.2.0 release Includes all changes in the [2.2.0 milestone](https://github.com/Yoast/yoastcs/milestone/16). --- CHANGELOG.md | 47 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 46 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f2b4ff2..197e5967 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,49 @@ All notable changes to this project will be documented in this file. This project adheres to [Semantic Versioning](https://semver.org/) and [Keep a CHANGELOG](https://keepachangelog.com/). +### [2.2.0] - 2021-09-22 + +#### Added +* [PHP Parallel Lint] will now be provided via YoastCS. + If `php-parallel-lint/php-parallel-lint` and `php-parallel-lint/php-console-highlighter` are included in the `require-dev` of your `composer.json` file, you can remove these after updating the version constraint for YoastCS to `"yoast/yoastcs": "^2.2.0"`. +* PHPCS: A custom `YoastCS\Yoast\Reports\Threshold` report. + This commit adds a custom report for use with PHPCS to compare the run results with "threshold" settings. + - The report will look in the runtime environment for the following two environment variables and will take the values of those as the thresholds to compare the PHPCS run results against: + * `YOASTCS_THRESHOLD_ERRORS` + * `YOASTCS_THRESHOLD_WARNINGS` + - If the environment variables are not set, they will default to 0 for both, i.e. no errors or warnings allowed. + - After the report has run, a global `YOASTCS_ABOVE_THRESHOLD` constant (boolean) will be available which can be used in calling scripts. + - To use this report, run PHPCS with the following command-line argument: `--report=YoastCS\Yoast\Reports\Threshold`. + Note: depending on the OS the command is run on, the backslashes in the report name may need to be escaped (doubled). +* PHPCS: The `PSR12.ControlStructures.BooleanOperatorPlacement` sniff. + Enforces that boolean operators in multi-line control structures are always placed at the start of a line. +* PHPCS: `Yoast.Commenting.CodeCoverageIgnoreDeprecated`: Support for attributes (PHP 8.0+) placed between a function or class declaration and the associated docblock. +* PHPCS: `Yoast.Commenting.TestHaveCoversTag`: Support for attributes (PHP 8.0+) placed between a function or class declaration and the associated docblock. +* PHPCS: `Yoast.NamingConventions.ObjectNameDepth`: Support for attributes (PHP 8.0+) placed between a function or class declaration and the associated docblock. +* PHPCS: `Yoast.NamingConventions.ObjectNameDepth`: Support for examining the word count in CamelCaps class names. +* PHPCS: `Yoast.NamingConventions.ValidHookName`: Verification that backslashes in namespace-like prefixes in double quoted strings are slash-escaped. +* An initial CONTRIBUTING file with guidelines on acceptance testing changes to the sniffs in this repository. + +#### Changed +* PHPCS: The default value for the `minimum_supported_wp_version` property which is used by various WPCS sniffs has been updated to WP `5.7` (was `5.4`). +* Composer: Supported version of [PHP_CodeSniffer] has been changed from `^3.5.0` to `^3.6.0`. +* Composer: Supported version of [WordPressCS] has been changed from `^2.2.0` to `^2.3.0`. +* Composer: Updated the supported versions of dev dependencies. +* Readme: Minor documentation improvements. +* Continuous Integration: CI has been moved from Travis to GitHub Actions. +* Various housekeeping. + +#### Fixed +* PHPCS: `Yoast.Commenting.CoversTag`: `@covers` tags refering to classes and functions which don't follow the WordPressCS naming conventions will now be regarded as valid. +* PHPCS: `Yoast.Commenting.TestsHaveCoversTag`: the sniff will now also report missing `@covers` tags for test methods without docblock. +* PHPCS: `Yoast.Commenting.TestsHaveCoversTag`: the determination whether a class or method is a test class or method has been made more flexible to allow for different test naming conventions. +* PHPCS: `Yoast.Commenting.TestsHaveCoversTag`: will no longer expect a `@covers` tag for abstract test methods. +* PHPCS: `Yoast.Files.FileComment`: fixed performance issue. +* PHPCS: `Yoast.Files.FileName`: will no longer throw an error when a class names is an exact match for one of the "removable" prefixes (as there would be nothing left to name the file as). +* PHPCS: `Yoast.NamingConventions.ObjectNameDepth`: the object name depth for underscore prefixed class names will now be calculated correctly. +* PHPCS: `Yoast.NamingConventions.ValidHookName`: will now recognize slash-escaped backslashes in namespace-like prefixes correctly when in a double quoted string. + + ### [2.1.0] - 2020-10-27 #### Added @@ -42,7 +85,7 @@ This project adheres to [Semantic Versioning](https://semver.org/) and [Keep a C * Various housekeeping. #### Fixed -* `Yoast.NamingConventions.NamespaceName`: fixed a potential "undefined index" notice. +* PHPCS: `Yoast.NamingConventions.NamespaceName`: fixed a potential "undefined index" notice. ### [2.0.0] - 2019-12-17 @@ -398,7 +441,9 @@ Initial public release as a stand-alone package. [PHP Mess Detector]: https://github.com/phpmd/phpmd/blob/master/CHANGELOG [DealerDirect Composer PHPCS plugin]: https://github.com/Dealerdirect/phpcodesniffer-composer-installer/releases [Parallel-Lint]: https://packagist.org/packages/jakub-onderka/php-parallel-lint +[PHP Parallel Lint]: https://github.com/php-parallel-lint/PHP-Parallel-Lint/ +[2.2.0]: https://github.com/Yoast/yoastcs/compare/2.1.0...2.2.0 [2.1.0]: https://github.com/Yoast/yoastcs/compare/2.0.2...2.1.0 [2.0.2]: https://github.com/Yoast/yoastcs/compare/2.0.1...2.0.2 [2.0.1]: https://github.com/Yoast/yoastcs/compare/2.0.0...2.0.1