From 86ee859dc3fe315f5656a1702c0565f4317a1ab8 Mon Sep 17 00:00:00 2001 From: jrfnl <663378+jrfnl@users.noreply.github.com> Date: Wed, 31 Jul 2019 12:38:17 +0200 Subject: [PATCH 01/29] Travis: run the code style related checks in separate stages Travis now offers stages. Using stages we can: - Run the code style related checks before running any unit tests and stop the build early if any are detected. - Remove the duplicate unit test runs - i.e. previously we had an extra (third) build against PHP 7.1 (now changed to 7.3) which would run the code style related checks, but would also re-run the unit tests. This extra build will now no longer run the unit tests. - Add a separate "quicktest" stage for non-PR/merge builds. The "quicktest" stage will only run a CS check, ruleset check, linting and the unit tests against low/high PHP/PHPCS/WPCS combinations. This should catch most issues. The more comprehensive complete build against a larger combination of PHP/PHPCS/WPCS combination will now only be run on PRs and merges to `develop`/`master`. While this does mean that the unit tests will run with a slight delay (the `Sniff` stage has to finish before they start), it also means that we: * Get code style errors reported earlier as it's been moved to be the first stage and the build will just stop if any are found. * We won't be wasting Travis's resources on builds which will have to be re-run anyway. Ref: https://docs.travis-ci.com/user/build-stages/ Note that `Allowed failures` is no longer listed as a separate block in the Travis result overview, but is _is_ respected. For more discussion about this: * https://github.com/travis-ci/travis-ci/issues/7789 * https://travis-ci.community/t/always-show-allow-failures-allowed-failures-when-build-stages-are-used/217 * https://travis-ci.community/t/work-out-kinks-in-interactions-between-stages-allow-fail-and-fast-finish/1090 * https://github.com/travis-ci/travis-ci/issues/9677 --- .travis.yml | 71 ++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 51 insertions(+), 20 deletions(-) diff --git a/.travis.yml b/.travis.yml index 23a24ac6..8f7fc4e2 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,5 +1,3 @@ -sudo: false - language: - php @@ -30,16 +28,55 @@ env: - PHPCS_BRANCH="3.4.2" WPCS="dev-develop" - PHPCS_BRANCH="3.4.2" WPCS="2.1.1" +# 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: sniff + - name: rulesets + - name: quicktest + if: type = push AND branch NOT IN (master, develop) + - name: test + if: branch IN (master, develop) + matrix: fast_finish: true include: - # Extra build to check for XML codestyle. - - php: 7.1 - env: PHPCS_BRANCH="dev-master" WPCS="^2.1.1" SNIFF=1 + #### SNIFF STAGE #### + - stage: sniff + php: 7.3 + env: PHPCS_BRANCH="dev-master" WPCS="dev-master" addons: - apt: - packages: - - libxml2-utils + 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") + + #### 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.3 + env: PHPCS_BRANCH="dev-master" WPCS="dev-develop" PHPLINT=1 + - php: 7.3 + env: PHPCS_BRANCH="3.3.1" WPCS="2.1.1" + - php: 5.4 + env: PHPCS_BRANCH="dev-master" WPCS="2.1.1" PHPLINT=1 + - php: 5.4 + env: PHPCS_BRANCH="3.3.1" WPCS="dev-develop" allow_failures: # Allow failures for unstable builds. @@ -54,11 +91,9 @@ 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 [[ $PHPCS_BRANCH != "dev-master" && $WPCS != "dev-develop" ]]; then + if [[ "$TRAVIS_BUILD_STAGE_NAME" != "Sniff" && $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 - - php -r "echo ini_get('error_reporting');" - - export XMLLINT_INDENT=" " - export PHPCS_DIR=$(pwd)/vendor/squizlabs/php_codesniffer @@ -68,7 +103,7 @@ before_install: # Set the WPCS version to test against. - composer require wp-coding-standards/wpcs:${WPCS} --no-update --no-suggest --no-scripts - | - if [[ "$SNIFF" == "1" ]]; then + if [[ "$TRAVIS_BUILD_STAGE_NAME" == "Sniff" ]]; then composer install --dev --no-suggest # The DealerDirect Composer plugin script takes care of the installed_paths. else @@ -83,21 +118,17 @@ before_install: - $PHPCS_BIN -i script: + # Lint the PHP files against parse errors. - if [[ "$PHPLINT" == "1" ]]; then if find . -path ./vendor -prune -o -name "*.php" -exec php -l {} \; | grep "^[Parse error|Fatal error]"; then exit 1; fi; fi + + # Run the unit tests. - | if [[ ${TRAVIS_PHP_VERSION:0:3} > "7.1" ]]; then vendor/bin/phpunit --filter Yoast --bootstrap="$PHPCS_DIR/tests/bootstrap.php" $PHPCS_DIR/tests/AllTests.php else phpunit --filter Yoast --bootstrap="$PHPCS_DIR/tests/bootstrap.php" $PHPCS_DIR/tests/AllTests.php fi - # Check the codestyle of the files within YoastCS. - - if [[ "$SNIFF" == "1" ]]; then composer check-cs; fi - # Validate the xml files. - # @link http://xmlsoft.org/xmllint.html - - if [[ "$SNIFF" == "1" ]]; then xmllint --noout --schema ./vendor/squizlabs/php_codesniffer/phpcs.xsd ./Yoast/ruleset.xml; fi - - if [[ "$SNIFF" == "1" ]]; then xmllint --noout ./Yoast/Docs/*/*Standard.xml; fi - # Check the code-style consistency of the xml files. - - if [[ "$SNIFF" == "1" ]]; then diff -B --tabsize=4 ./Yoast/ruleset.xml <(xmllint --format "./Yoast/ruleset.xml"); fi + # 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 c2c1203bab0199c08dfd12ab9523d5db180db41d Mon Sep 17 00:00:00 2001 From: jrfnl Date: Tue, 6 Aug 2019 15:42:23 +0200 Subject: [PATCH 02/29] CS: use short arrays WPCS will start forbidding the use of short arrays as of WPCS 2.2.0. For YoastCS the decision has been taken to not comply with this rule and to enforce short arrays instead. As the minimum supported PHP version is still PHP 5.2 for most repos, we cannot yet enforce this through the `YoastCS` ruleset. So, for now, this will only be enforced for PHP 5.4+ directories in those repos which have them. As YoastCS itself has a minimum PHP requirement of PHP 5.4, we _can_ start enforcing it in the code of YoastCS itself. This PR implements that. --- .phpcs.xml.dist | 8 +++ .../CodeCoverageIgnoreDeprecatedSniff.php | 4 +- Yoast/Sniffs/Commenting/CoversTagSniff.php | 18 +++---- .../Commenting/TestsHaveCoversTagSniff.php | 6 +-- .../IfElseDeclarationSniff.php | 12 ++--- Yoast/Sniffs/Files/FileNameSniff.php | 16 +++--- Yoast/Sniffs/Files/TestDoublesSniff.php | 22 ++++---- .../Namespaces/NamespaceDeclarationSniff.php | 10 ++-- .../Yoast/AlternativeFunctionsSniff.php | 16 +++--- .../CodeCoverageIgnoreDeprecatedUnitTest.php | 6 +-- Yoast/Tests/Commenting/CoversTagUnitTest.php | 6 +-- .../Tests/Commenting/FileCommentUnitTest.php | 12 ++--- .../Commenting/TestsHaveCoversTagUnitTest.php | 6 +-- .../IfElseDeclarationUnitTest.php | 6 +-- Yoast/Tests/Files/FileNameUnitTest.php | 18 +++---- Yoast/Tests/Files/TestDoublesUnitTest.php | 50 +++++++++---------- .../NamespaceDeclarationUnitTest.php | 8 +-- .../WhiteSpace/FunctionSpacingUnitTest.php | 6 +-- .../Yoast/AlternativeFunctionsUnitTest.php | 6 +-- 19 files changed, 122 insertions(+), 114 deletions(-) diff --git a/.phpcs.xml.dist b/.phpcs.xml.dist index 612a0033..9c697007 100644 --- a/.phpcs.xml.dist +++ b/.phpcs.xml.dist @@ -27,9 +27,17 @@ + + + + + + + diff --git a/Yoast/Sniffs/Commenting/CodeCoverageIgnoreDeprecatedSniff.php b/Yoast/Sniffs/Commenting/CodeCoverageIgnoreDeprecatedSniff.php index 6738f8b7..7da47be2 100644 --- a/Yoast/Sniffs/Commenting/CodeCoverageIgnoreDeprecatedSniff.php +++ b/Yoast/Sniffs/Commenting/CodeCoverageIgnoreDeprecatedSniff.php @@ -23,9 +23,9 @@ class CodeCoverageIgnoreDeprecatedSniff implements Sniff { * @return array */ public function register() { - return array( + return [ T_FUNCTION, - ); + ]; } /** diff --git a/Yoast/Sniffs/Commenting/CoversTagSniff.php b/Yoast/Sniffs/Commenting/CoversTagSniff.php index 3ff0fd55..173ca4b0 100644 --- a/Yoast/Sniffs/Commenting/CoversTagSniff.php +++ b/Yoast/Sniffs/Commenting/CoversTagSniff.php @@ -45,9 +45,9 @@ class CoversTagSniff implements Sniff { * @return array */ public function register() { - return array( + return [ T_DOC_COMMENT_OPEN_TAG, - ); + ]; } /** @@ -148,17 +148,17 @@ public function process( File $phpcsFile, $stackPtr ) { // Throw a generic error for all other invalid annotations. $error = self::ERROR_MSG; $error .= ' Check the PHPUnit documentation to see which annotations are supported. Found: %s'; - $data = array( $annotation ); + $data = [ $annotation ]; $phpcsFile->addError( $error, $next, 'Invalid', $data ); } $coversNothingCount = count( $coversNothingTags ); if ( $firstCoversTag !== false && $coversNothingCount > 0 ) { $error = 'A test can\'t both cover something as well as cover nothing. First @coversNothing tag encountered on line %d; first @covers tag encountered on line %d'; - $data = array( + $data = [ $tokens[ $coversNothingTags[0] ]['line'], $tokens[ $firstCoversTag ]['line'], - ); + ]; $phpcsFile->addError( $error, $tokens[ $stackPtr ]['comment_closer'], 'Contradictory', $data ); } @@ -167,7 +167,7 @@ public function process( File $phpcsFile, $stackPtr ) { $error = 'Only one @coversNothing tag allowed per test'; $code = 'DuplicateCoversNothing'; $fixable = true; - $removeTags = array(); + $removeTags = []; foreach ( $coversNothingTags as $position => $ptr ) { $next = ( $ptr + 1 ); if ( $tokens[ $next ]['code'] === T_DOC_COMMENT_WHITESPACE @@ -232,7 +232,7 @@ public function process( File $phpcsFile, $stackPtr ) { if ( ! isset( $first ) ) { $first = explode( '-', $ptrs ); - $data = array( $tokens[ $first[0] ]['line'] ); + $data = [ $tokens[ $first[0] ]['line'] ]; continue; } @@ -288,10 +288,10 @@ protected function fixSimpleError( File $phpcsFile, $stackPtr, $expected, $error } $error = self::ERROR_MSG . "\nExpected: `%s`\nFound: `%s`"; - $data = array( + $data = [ $expected, $annotation, - ); + ]; $fix = $phpcsFile->addFixableError( $error, $stackPtr, $errorCode, $data ); if ( $fix === true ) { diff --git a/Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php b/Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php index 4f002d9a..e48cd7c6 100644 --- a/Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php +++ b/Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php @@ -22,10 +22,10 @@ class TestsHaveCoversTagSniff implements Sniff { * @return array */ public function register() { - return array( + return [ T_CLASS, T_FUNCTION, - ); + ]; } /** @@ -182,7 +182,7 @@ protected function process_function( File $phpcsFile, $stackPtr ) { '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', - array( $name ) + [ $name ] ); } } diff --git a/Yoast/Sniffs/ControlStructures/IfElseDeclarationSniff.php b/Yoast/Sniffs/ControlStructures/IfElseDeclarationSniff.php index f9bf9abd..d9e68782 100644 --- a/Yoast/Sniffs/ControlStructures/IfElseDeclarationSniff.php +++ b/Yoast/Sniffs/ControlStructures/IfElseDeclarationSniff.php @@ -23,10 +23,10 @@ class IfElseDeclarationSniff implements Sniff { * @return array */ public function register() { - return array( + return [ T_ELSE, T_ELSEIF, - ); + ]; } /** @@ -64,7 +64,7 @@ public function process( File $phpcsFile, $stackPtr ) { '%s statement must be on a new line', $stackPtr, 'NewLine', - array( ucfirst( $tokens[ $stackPtr ]['content'] ) ) + [ ucfirst( $tokens[ $stackPtr ]['content'] ) ] ); } elseif ( $tokens[ $previous_scope_closer ]['column'] !== $tokens[ $stackPtr ]['column'] ) { @@ -72,7 +72,7 @@ public function process( File $phpcsFile, $stackPtr ) { '%s statement not aligned with previous part of the control structure', $stackPtr, 'Alignment', - array( ucfirst( $tokens[ $stackPtr ]['content'] ) ) + [ ucfirst( $tokens[ $stackPtr ]['content'] ) ] ); } @@ -80,10 +80,10 @@ public function process( File $phpcsFile, $stackPtr ) { if ( $previous_scope_closer !== $previous_non_empty ) { $error = 'Nothing but whitespace and comments allowed between closing bracket and %s statement, found "%s"'; - $data = array( + $data = [ $tokens[ $stackPtr ]['content'], trim( $phpcsFile->getTokensAsString( ( $previous_scope_closer + 1 ), ( $stackPtr - ( $previous_scope_closer + 1 ) ) ) ), - ); + ]; $phpcsFile->addError( $error, $stackPtr, 'StatementFound', $data ); } } diff --git a/Yoast/Sniffs/Files/FileNameSniff.php b/Yoast/Sniffs/Files/FileNameSniff.php index 506fe309..f05a4115 100644 --- a/Yoast/Sniffs/Files/FileNameSniff.php +++ b/Yoast/Sniffs/Files/FileNameSniff.php @@ -39,7 +39,7 @@ class FileNameSniff implements Sniff { * * @var string[] */ - public $prefixes = array(); + public $prefixes = []; /** * List of files to exclude from the strict file name check. @@ -59,18 +59,18 @@ class FileNameSniff implements Sniff { * * @var string[] */ - public $exclude = array(); + public $exclude = []; /** * Object tokens to search for in a file. * * @var array */ - private $oo_tokens = array( + private $oo_tokens = [ T_CLASS, T_INTERFACE, T_TRAIT, - ); + ]; /** * Returns an array of tokens this test wants to listen for. @@ -78,7 +78,7 @@ class FileNameSniff implements Sniff { * @return array */ public function register() { - return array( T_OPEN_TAG ); + return [ T_OPEN_TAG ]; } /** @@ -189,10 +189,10 @@ public function process( File $phpcsFile, $stackPtr ) { $error, 0, $error_code, - array( + [ $expected . '.' . $extension, $basename, - ) + ] ); } @@ -213,7 +213,7 @@ protected function is_file_excluded( File $phpcsFile, $path_to_file ) { $exclude = $this->clean_custom_array_property( $this->exclude, true, true ); if ( ! empty( $exclude ) ) { - $exclude = array_map( array( $this, 'normalize_directory_separators' ), $exclude ); + $exclude = array_map( [ $this, 'normalize_directory_separators' ], $exclude ); $path_to_file = $this->normalize_directory_separators( $path_to_file ); if ( ! isset( $phpcsFile->config->basepath ) ) { diff --git a/Yoast/Sniffs/Files/TestDoublesSniff.php b/Yoast/Sniffs/Files/TestDoublesSniff.php index e909f39b..33359866 100644 --- a/Yoast/Sniffs/Files/TestDoublesSniff.php +++ b/Yoast/Sniffs/Files/TestDoublesSniff.php @@ -33,9 +33,9 @@ class TestDoublesSniff implements Sniff { * * @var array */ - public $doubles_path = array( + public $doubles_path = [ '/tests/doubles', - ); + ]; /** * Validated absolute target paths for test double/mock classes or an empty array @@ -51,11 +51,11 @@ class TestDoublesSniff implements Sniff { * @return array */ public function register() { - return array( + return [ T_CLASS, T_INTERFACE, T_TRAIT, - ); + ]; } /** @@ -120,7 +120,7 @@ public function process( File $phpcsFile, $stackPtr ) { $base_path = rtrim( $base_path, '/' ) . '/'; // Make sure the base_path ends in a single slash. if ( ! isset( $this->target_paths ) || defined( 'PHP_CODESNIFFER_IN_TESTS' ) ) { - $this->target_paths = array(); + $this->target_paths = []; foreach ( $this->doubles_path as $doubles_path ) { $target_path = $base_path; @@ -134,9 +134,9 @@ public function process( File $phpcsFile, $stackPtr ) { if ( empty( $this->target_paths ) ) { // No valid target paths found. - $data = array( + $data = [ $phpcsFile->config->basepath, - ); + ]; if ( count( $this->doubles_path ) === 1 ) { $data[] = 'directory'; @@ -169,10 +169,10 @@ public function process( File $phpcsFile, $stackPtr ) { } if ( $is_error === true ) { - $data = array( + $data = [ $tokens[ $stackPtr ]['content'], $object_name, - ); + ]; $phpcsFile->addError( 'Double/Mock test helper classes should be placed in a dedicated test doubles sub-directory. Found %s: %s', @@ -189,12 +189,12 @@ public function process( File $phpcsFile, $stackPtr ) { } if ( $more_objects_in_file !== false ) { - $data = array( + $data = [ $tokens[ $stackPtr ]['content'], $object_name, $tokens[ $more_objects_in_file ]['content'], $phpcsFile->getDeclarationName( $more_objects_in_file ), - ); + ]; $phpcsFile->addError( 'Double/Mock test helper classes should be in their own file. Found %1$s: %2$s and %3$s: %4$s', diff --git a/Yoast/Sniffs/Namespaces/NamespaceDeclarationSniff.php b/Yoast/Sniffs/Namespaces/NamespaceDeclarationSniff.php index d69747b2..4fcba833 100644 --- a/Yoast/Sniffs/Namespaces/NamespaceDeclarationSniff.php +++ b/Yoast/Sniffs/Namespaces/NamespaceDeclarationSniff.php @@ -30,9 +30,9 @@ class NamespaceDeclarationSniff implements Sniff { * @return array */ public function register() { - return array( + return [ T_OPEN_TAG, - ); + ]; } /** @@ -48,7 +48,7 @@ public function process( File $phpcsFile, $stackPtr ) { $tokens = $phpcsFile->getTokens(); - $statements = array(); + $statements = []; while ( ( $stackPtr = $phpcsFile->findNext( T_NAMESPACE, ( $stackPtr + 1 ) ) ) !== false ) { @@ -91,10 +91,10 @@ public function process( File $phpcsFile, $stackPtr ) { $count = count( $statements ); if ( $count > 1 ) { - $data = array( + $data = [ $count, $tokens[ $statements[0] ]['line'], - ); + ]; for ( $i = 1; $i < $count; $i++ ) { $phpcsFile->addError( diff --git a/Yoast/Sniffs/Yoast/AlternativeFunctionsSniff.php b/Yoast/Sniffs/Yoast/AlternativeFunctionsSniff.php index 2e595151..639aafab 100644 --- a/Yoast/Sniffs/Yoast/AlternativeFunctionsSniff.php +++ b/Yoast/Sniffs/Yoast/AlternativeFunctionsSniff.php @@ -20,17 +20,17 @@ class AlternativeFunctionsSniff extends AbstractFunctionRestrictionsSniff { * @return array */ public function getGroups() { - return array( - 'json_encode' => array( + return [ + 'json_encode' => [ 'type' => 'error', 'message' => 'Detected a call to %s(). Use %s() instead.', - 'functions' => array( + 'functions' => [ 'json_encode', 'wp_json_encode', - ), + ], 'replacement' => 'WPSEO_Utils::format_json_encode', - ), - ); + ], + ]; } /** @@ -54,10 +54,10 @@ public function process_matched_token( $stackPtr, $group_name, $matched_content $message = $this->groups[ $group_name ]['message']; $is_error = ( $this->groups[ $group_name ]['type'] === 'error' ); $error_code = $this->string_to_errorcode( $group_name . '_' . $matched_content ); - $data = array( + $data = [ $matched_content, $replacement, - ); + ]; /* * Deal with specific situations. diff --git a/Yoast/Tests/Commenting/CodeCoverageIgnoreDeprecatedUnitTest.php b/Yoast/Tests/Commenting/CodeCoverageIgnoreDeprecatedUnitTest.php index b935d4a8..e28989dc 100644 --- a/Yoast/Tests/Commenting/CodeCoverageIgnoreDeprecatedUnitTest.php +++ b/Yoast/Tests/Commenting/CodeCoverageIgnoreDeprecatedUnitTest.php @@ -21,11 +21,11 @@ class CodeCoverageIgnoreDeprecatedUnitTest extends AbstractSniffUnitTest { * @return array => */ public function getErrorList() { - return array( + return [ 41 => 1, 50 => 1, 55 => 1, - ); + ]; } /** @@ -34,6 +34,6 @@ public function getErrorList() { * @return array => */ public function getWarningList() { - return array(); + return []; } } diff --git a/Yoast/Tests/Commenting/CoversTagUnitTest.php b/Yoast/Tests/Commenting/CoversTagUnitTest.php index 9734933f..b4a2c1b1 100644 --- a/Yoast/Tests/Commenting/CoversTagUnitTest.php +++ b/Yoast/Tests/Commenting/CoversTagUnitTest.php @@ -21,7 +21,7 @@ class CoversTagUnitTest extends AbstractSniffUnitTest { * @return array => */ public function getErrorList() { - return array( + return [ 34 => 1, 35 => 1, 36 => 1, @@ -50,7 +50,7 @@ public function getErrorList() { 140 => 1, 150 => 1, 151 => 1, - ); + ]; } /** @@ -59,6 +59,6 @@ public function getErrorList() { * @return array => */ public function getWarningList() { - return array(); + return []; } } diff --git a/Yoast/Tests/Commenting/FileCommentUnitTest.php b/Yoast/Tests/Commenting/FileCommentUnitTest.php index 7fc68063..eaaaebad 100644 --- a/Yoast/Tests/Commenting/FileCommentUnitTest.php +++ b/Yoast/Tests/Commenting/FileCommentUnitTest.php @@ -27,12 +27,12 @@ public function getErrorList( $testFile = '' ) { case 'FileCommentUnitTest.2.inc': case 'FileCommentUnitTest.8.inc': case 'FileCommentUnitTest.10.inc': - return array( + return [ 1 => 1, - ); + ]; default: - return array(); + return []; } } @@ -47,12 +47,12 @@ public function getWarningList( $testFile = '' ) { switch ( $testFile ) { case 'FileCommentUnitTest.4.inc': case 'FileCommentUnitTest.6.inc': - return array( + return [ 2 => 1, - ); + ]; default: - return array(); + return []; } } } diff --git a/Yoast/Tests/Commenting/TestsHaveCoversTagUnitTest.php b/Yoast/Tests/Commenting/TestsHaveCoversTagUnitTest.php index 93a39312..201f9b65 100644 --- a/Yoast/Tests/Commenting/TestsHaveCoversTagUnitTest.php +++ b/Yoast/Tests/Commenting/TestsHaveCoversTagUnitTest.php @@ -21,10 +21,10 @@ class TestsHaveCoversTagUnitTest extends AbstractSniffUnitTest { * @return array => */ public function getErrorList() { - return array( + return [ 59 => 1, 88 => 1, - ); + ]; } /** @@ -33,6 +33,6 @@ public function getErrorList() { * @return array => */ public function getWarningList() { - return array(); + return []; } } diff --git a/Yoast/Tests/ControlStructures/IfElseDeclarationUnitTest.php b/Yoast/Tests/ControlStructures/IfElseDeclarationUnitTest.php index 327794b6..3cf2ba68 100644 --- a/Yoast/Tests/ControlStructures/IfElseDeclarationUnitTest.php +++ b/Yoast/Tests/ControlStructures/IfElseDeclarationUnitTest.php @@ -22,7 +22,7 @@ class IfElseDeclarationUnitTest extends AbstractSniffUnitTest { * @return array => */ public function getErrorList() { - return array( + return [ 22 => 1, 28 => 1, 30 => 1, @@ -32,7 +32,7 @@ public function getErrorList() { 51 => 1, 76 => 1, 84 => 2, - ); + ]; } /** @@ -41,6 +41,6 @@ public function getErrorList() { * @return array => */ public function getWarningList() { - return array(); + return []; } } diff --git a/Yoast/Tests/Files/FileNameUnitTest.php b/Yoast/Tests/Files/FileNameUnitTest.php index 52c37108..b76ad069 100644 --- a/Yoast/Tests/Files/FileNameUnitTest.php +++ b/Yoast/Tests/Files/FileNameUnitTest.php @@ -20,7 +20,7 @@ class FileNameUnitTest extends AbstractSniffUnitTest { * * @var array */ - private $expected_results = array( + private $expected_results = [ /* * In /FileNameUnitTests. @@ -74,7 +74,7 @@ class FileNameUnitTest extends AbstractSniffUnitTest { // Fall-back file in case glob() fails. 'FileNameUnitTest.inc' => 1, - ); + ]; /** * Set CLI values before the file is tested. @@ -107,7 +107,7 @@ protected function getTestFiles( $testFileBase ) { return $test_files; } - return array( $testFileBase . '.inc' ); + return [ $testFileBase . '.inc' ]; } /** @@ -120,12 +120,12 @@ protected function getTestFiles( $testFileBase ) { public function getErrorList( $testFile = '' ) { if ( isset( $this->expected_results[ $testFile ] ) ) { - return array( + return [ 1 => $this->expected_results[ $testFile ], - ); + ]; } - return array(); + return []; } /** @@ -137,11 +137,11 @@ public function getErrorList( $testFile = '' ) { */ public function getWarningList( $testFile = '' ) { if ( $testFile === 'no-basepath.inc' ) { - return array( + return [ 1 => 1, - ); + ]; } - return array(); + return []; } } diff --git a/Yoast/Tests/Files/TestDoublesUnitTest.php b/Yoast/Tests/Files/TestDoublesUnitTest.php index 66116131..25fdd3ff 100644 --- a/Yoast/Tests/Files/TestDoublesUnitTest.php +++ b/Yoast/Tests/Files/TestDoublesUnitTest.php @@ -46,7 +46,7 @@ protected function getTestFiles( $testFileBase ) { return $test_files; } - return array( $testFileBase . '.inc' ); + return [ $testFileBase . '.inc' ]; } /** @@ -61,59 +61,59 @@ public function getErrorList( $testFile = '' ) { switch ( $testFile ) { // In tests/. case 'mock-not-in-correct-dir.inc': - return array( + return [ 3 => 1, - ); + ]; case 'multiple-objects-in-file.inc': - return array( + return [ 5 => 2, - ); + ]; case 'multiple-objects-in-file-reverse.inc': - return array( + return [ 7 => 2, - ); + ]; case 'non-existant-doubles-dir.inc': - return array( + return [ 4 => 1, - ); + ]; case 'not-in-correct-custom-dir.inc': - return array( + return [ 4 => 1, - ); + ]; case 'not-in-correct-dir-double.inc': - return array( + return [ 3 => 1, - ); + ]; case 'not-in-correct-dir-mock.inc': - return array( + return [ 3 => 1, - ); + ]; // In tests/doubles. case 'multiple-mocks-in-file.inc': - return array( + return [ 3 => 1, 5 => 1, - ); + ]; // In tests/doubles-not-correct. case 'not-in-correct-subdir.inc': - return array( + return [ 3 => 1, - ); + ]; case 'not-double-or-mock.inc': // In tests. case 'correct-dir-double.inc': // In tests/doubles. case 'correct-dir-mock.inc': // In tests/doubles. case 'correct-custom-dir.inc': // In tests/mocks. default: - return array(); + return []; } } @@ -127,17 +127,17 @@ public function getErrorList( $testFile = '' ) { public function getWarningList( $testFile = '' ) { switch ( $testFile ) { case 'no-basepath.inc': - return array( + return [ 1 => 1, - ); + ]; case 'no-doubles-path-property.inc': - return array( + return [ 1 => 1, - ); + ]; default: - return array(); + return []; } } } diff --git a/Yoast/Tests/Namespaces/NamespaceDeclarationUnitTest.php b/Yoast/Tests/Namespaces/NamespaceDeclarationUnitTest.php index a6572f14..59fa79e8 100644 --- a/Yoast/Tests/Namespaces/NamespaceDeclarationUnitTest.php +++ b/Yoast/Tests/Namespaces/NamespaceDeclarationUnitTest.php @@ -25,16 +25,16 @@ class NamespaceDeclarationUnitTest extends AbstractSniffUnitTest { public function getErrorList( $testFile = '' ) { switch ( $testFile ) { case 'NamespaceDeclarationUnitTest.2.inc': - return array( + return [ 3 => 1, 5 => 3, 7 => 2, 9 => 3, 11 => 2, - ); + ]; default: - return array(); + return []; } } @@ -44,6 +44,6 @@ public function getErrorList( $testFile = '' ) { * @return array => */ public function getWarningList() { - return array(); + return []; } } diff --git a/Yoast/Tests/WhiteSpace/FunctionSpacingUnitTest.php b/Yoast/Tests/WhiteSpace/FunctionSpacingUnitTest.php index d4061549..228018bf 100644 --- a/Yoast/Tests/WhiteSpace/FunctionSpacingUnitTest.php +++ b/Yoast/Tests/WhiteSpace/FunctionSpacingUnitTest.php @@ -21,7 +21,7 @@ class FunctionSpacingUnitTest extends AbstractSniffUnitTest { * @return array => */ public function getErrorList() { - return array( + return [ 31 => 1, 33 => 1, 39 => 1, @@ -32,7 +32,7 @@ public function getErrorList() { 74 => 1, 87 => 2, 88 => 1, - ); + ]; } /** @@ -41,6 +41,6 @@ public function getErrorList() { * @return array => */ public function getWarningList() { - return array(); + return []; } } diff --git a/Yoast/Tests/Yoast/AlternativeFunctionsUnitTest.php b/Yoast/Tests/Yoast/AlternativeFunctionsUnitTest.php index dc4fd96c..78c4b149 100644 --- a/Yoast/Tests/Yoast/AlternativeFunctionsUnitTest.php +++ b/Yoast/Tests/Yoast/AlternativeFunctionsUnitTest.php @@ -19,7 +19,7 @@ class AlternativeFunctionsUnitTest extends AbstractSniffUnitTest { * @return array => */ public function getErrorList() { - return array( + return [ 12 => 1, 13 => 1, 14 => 1, @@ -27,7 +27,7 @@ public function getErrorList() { 17 => 1, 21 => 1, 22 => 1, - ); + ]; } /** @@ -36,6 +36,6 @@ public function getErrorList() { * @return array => */ public function getWarningList() { - return array(); + return []; } } From d20597c62618dd8e48ff8ac93b93dd08ba4a615d Mon Sep 17 00:00:00 2001 From: jrfnl <663378+jrfnl@users.noreply.github.com> Date: Wed, 31 Jul 2019 13:39:32 +0200 Subject: [PATCH 03/29] Travis/Composer: switch over to parallel linting of PHP files ## Composer This installs two additional PHP packages in `require-dev`: * [`php-parallel-lint`](https://packagist.org/packages/jakub-onderka/php-parallel-lint) which allows for linting PHP files in parallel (faster), as well as automatically recursively walking directories. * [`php-console-highlighter`](https://packagist.org/packages/jakub-onderka/php-console-highlighter) which provides PHP code highlighting in the command line console, allowing the linter to display the results in a more meaningful manner. It also adds a new `lint` script for use with Composer. ## Travis * Switch out the script part in the Travis script which did the linting the "old-fashioned" way to use the new Parallel linting option. * Adjust the `composer install` commands in the `before_install` section and the `phpunit` command in the `script` section. As the above mentioned packages are `require-dev`, we'll now always need to run a full dev `composer install`. We can make it a tiny bit faster by removing a few packages which aren't needed in the test stage, but that shouldn't matter much anyway as the packages are installed from the cache. Also: as a full dev `composer install` is run now anyway, we don't need to have the special conditions related to the unsupported PHPUnit 8 package. --- .travis.yml | 23 ++++++----------------- composer.json | 7 ++++++- 2 files changed, 12 insertions(+), 18 deletions(-) diff --git a/.travis.yml b/.travis.yml index 8f7fc4e2..8dfb8646 100644 --- a/.travis.yml +++ b/.travis.yml @@ -103,31 +103,20 @@ before_install: # Set the WPCS version to test against. - composer require wp-coding-standards/wpcs:${WPCS} --no-update --no-suggest --no-scripts - | - if [[ "$TRAVIS_BUILD_STAGE_NAME" == "Sniff" ]]; then - composer install --dev --no-suggest - # The DealerDirect Composer plugin script takes care of the installed_paths. - else + if [[ "$TRAVIS_BUILD_STAGE_NAME" != "Sniff" ]]; then # For testing the YoastCS native sniffs, the rest of the packages aren't needed. - composer remove phpcompatibility/phpcompatibility-wp --no-update - # The Travis images for PHP >= 7.2 ship with PHPUnit 8, but the unit test suite is not compatible with that. - if [[ ${TRAVIS_PHP_VERSION:0:3} > "7.1" ]]; then composer require phpunit/phpunit:^7.0 --no-update --no-suggest --no-scripts;fi - # This will now only install the version of PHPCS/WPCS to test against. - composer install --no-dev --no-suggest - # The DealerDirect PHPCS Composer plugin takes care of the installed_paths. + composer remove phpcompatibility/phpcompatibility-wp phpcompatibility/php-compatibility --no-update fi + - composer install --dev --no-suggest + # 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 if find . -path ./vendor -prune -o -name "*.php" -exec php -l {} \; | grep "^[Parse error|Fatal error]"; then exit 1; fi; fi + - if [[ "$PHPLINT" == "1" ]]; then composer lint; fi # Run the unit tests. - - | - if [[ ${TRAVIS_PHP_VERSION:0:3} > "7.1" ]]; then - vendor/bin/phpunit --filter Yoast --bootstrap="$PHPCS_DIR/tests/bootstrap.php" $PHPCS_DIR/tests/AllTests.php - else - phpunit --filter Yoast --bootstrap="$PHPCS_DIR/tests/bootstrap.php" $PHPCS_DIR/tests/AllTests.php - fi + - 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 diff --git a/composer.json b/composer.json index e1ca36b2..121395a1 100644 --- a/composer.json +++ b/composer.json @@ -31,7 +31,9 @@ "require-dev": { "phpcompatibility/php-compatibility": "^9.2.0", "roave/security-advisories": "dev-master", - "phpunit/phpunit": "^4.0 || ^5.0 || ^6.0 || ^7.0" + "phpunit/phpunit": "^4.0 || ^5.0 || ^6.0 || ^7.0", + "jakub-onderka/php-parallel-lint": "^1.0", + "jakub-onderka/php-console-highlighter": "^0.4" }, "minimum-stability": "dev", "prefer-stable": true, @@ -40,6 +42,9 @@ "Dealerdirect\\Composer\\Plugin\\Installers\\PHPCodeSniffer\\Plugin::run", "@php ./vendor/squizlabs/php_codesniffer/bin/phpcs --config-set default_standard Yoast" ], + "lint": [ + "@php ./vendor/jakub-onderka/php-parallel-lint/parallel-lint . -e php --exclude vendor" + ], "check-cs": [ "@php ./vendor/squizlabs/php_codesniffer/bin/phpcs --runtime-set testVersion 5.4-" ], From 2ca504e53d74f7596106310005cc2c58e5e6b2cf Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Oct 2019 22:26:09 +0200 Subject: [PATCH 04/29] Travis: minor tweak The travis linter does not like lists of languages. This should always be a singular value. --- .travis.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 8dfb8646..2ef301a2 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,5 +1,4 @@ -language: - - php +language: php dist: trusty From 353d4d9b0af207e5f41a10175af7169b2ccd5891 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Wed, 23 Oct 2019 11:54:55 +0200 Subject: [PATCH 05/29] FileName: rename public properties Sniff properties are specific to individual sniffs, but can be set for all sniffs in one go (undocumented PHPCS feature). The public `$prefixes` property is used by the WPCS `PrefixAllGlobals` sniff to pass on which text strings should be considered "prefixes" for namespaces, class names, hook names, variable names etc for the purposes of that sniff. Both the new, upcoming `ValidNamespace` sniff (141) and the Yoast specific `ValidHookName` sniff (143) will need the `$prefixes` property as well and the content for the property for those sniffs should overlap with the `$prefixes` property for the `PrefixAllGlobals` sniff. To prevent having to set the same value three times in each custom ruleset, it would be useful to be able to use the "_set the property for all sniffs in one go_" feature. However, the YoastCS `FileName` sniff also uses a `$prefixes` property which is a distinct, different property which indicates which part at the start of a class name should be removed to get the file name. This property may partially overlap with the `$prefixes` property for the `PrefixAllGlobals` sniff, but not necessarily. The overlap will become smaller/non-existent once all plugins will start using namespaces. To prevent confusion between the two types of `$prefixes` properties, I'm changing the property name in the `FileName` sniff to `$oo_prefixes`. The `FileName` sniff also has an `$exclude` property which is used to indicate which file names should be _excluded_ from the strict file content specific part of the `FileName` sniff checks. As a number of WPCS sniffs also have an `$exclude` property with a different meaning, this property again can be confusing, so we may as well rename that property now as well. **Note: This is a backward-compatibility break.** * This means that the next version will need to be YoastCS 2.0. * Existing custom rulesets using these properties will need to be adjusted to reflect the change. --- Yoast/Sniffs/Files/FileNameSniff.php | 8 ++++---- Yoast/Tests/Files/FileNameUnitTests/ExcludedFile.inc | 4 ++-- .../FileNameUnitTests/classes/class-wpseo-some-class.inc | 4 ++-- .../FileNameUnitTests/classes/excluded-CLASS-file.inc | 4 ++-- .../Tests/Files/FileNameUnitTests/classes/some-class.inc | 4 ++-- .../Files/FileNameUnitTests/classes/wpseo-some-class.inc | 4 ++-- .../FileNameUnitTests/classes/yoast-plugin-some-class.inc | 4 ++-- Yoast/Tests/Files/FileNameUnitTests/excluded-file.inc | 4 ++-- Yoast/Tests/Files/FileNameUnitTests/excluded_file.inc | 4 ++-- .../functions/excluded-functions-file.inc | 4 ++-- .../FileNameUnitTests/interfaces/different-interface.inc | 4 ++-- .../interfaces/excluded-interface-file.inc | 4 ++-- .../interfaces/no-duplicate-interface.inc | 4 ++-- .../interfaces/outline-something-interface.inc | 4 ++-- .../FileNameUnitTests/interfaces/outline-something.inc | 4 ++-- .../interfaces/yoast-outline-something.inc | 4 ++-- Yoast/Tests/Files/FileNameUnitTests/no-basepath.inc | 4 ++-- .../Files/FileNameUnitTests/traits/different-trait.inc | 4 ++-- .../FileNameUnitTests/traits/excluded-trait-file.inc | 4 ++-- .../Files/FileNameUnitTests/traits/no-duplicate-trait.inc | 4 ++-- .../FileNameUnitTests/traits/outline-something-trait.inc | 4 ++-- .../Files/FileNameUnitTests/traits/outline-something.inc | 4 ++-- .../FileNameUnitTests/traits/yoast-outline-something.inc | 4 ++-- 23 files changed, 48 insertions(+), 48 deletions(-) diff --git a/Yoast/Sniffs/Files/FileNameSniff.php b/Yoast/Sniffs/Files/FileNameSniff.php index f05a4115..d25e386a 100644 --- a/Yoast/Sniffs/Files/FileNameSniff.php +++ b/Yoast/Sniffs/Files/FileNameSniff.php @@ -39,7 +39,7 @@ class FileNameSniff implements Sniff { * * @var string[] */ - public $prefixes = []; + public $oo_prefixes = []; /** * List of files to exclude from the strict file name check. @@ -59,7 +59,7 @@ class FileNameSniff implements Sniff { * * @var string[] */ - public $exclude = []; + public $excluded_files_strict_check = []; /** * Object tokens to search for in a file. @@ -128,7 +128,7 @@ public function process( File $phpcsFile, $stackPtr ) { $tokens = $phpcsFile->getTokens(); $name = $phpcsFile->getDeclarationName( $oo_structure ); - $prefixes = $this->clean_custom_array_property( $this->prefixes ); + $prefixes = $this->clean_custom_array_property( $this->oo_prefixes ); if ( ! empty( $prefixes ) ) { // Use reverse natural sorting to get the longest of overlapping prefixes first. rsort( $prefixes, ( SORT_NATURAL | SORT_FLAG_CASE ) ); @@ -210,7 +210,7 @@ public function process( File $phpcsFile, $stackPtr ) { * @return bool */ protected function is_file_excluded( File $phpcsFile, $path_to_file ) { - $exclude = $this->clean_custom_array_property( $this->exclude, true, true ); + $exclude = $this->clean_custom_array_property( $this->excluded_files_strict_check, true, true ); if ( ! empty( $exclude ) ) { $exclude = array_map( [ $this, 'normalize_directory_separators' ], $exclude ); diff --git a/Yoast/Tests/Files/FileNameUnitTests/ExcludedFile.inc b/Yoast/Tests/Files/FileNameUnitTests/ExcludedFile.inc index ad132138..21ac4aaa 100644 --- a/Yoast/Tests/Files/FileNameUnitTests/ExcludedFile.inc +++ b/Yoast/Tests/Files/FileNameUnitTests/ExcludedFile.inc @@ -1,6 +1,6 @@ -phpcs:set Yoast.Files.FileName exclude[] ExcludedFile.inc +phpcs:set Yoast.Files.FileName excluded_files_strict_check[] ExcludedFile.inc -phpcs:set Yoast.Files.FileName prefixes[] wpseo,yoast +phpcs:set Yoast.Files.FileName oo_prefixes[] wpseo,yoast -phpcs:set Yoast.Files.FileName exclude[] classes/excluded-CLASS-file.inc +phpcs:set Yoast.Files.FileName excluded_files_strict_check[] classes/excluded-CLASS-file.inc -phpcs:set Yoast.Files.FileName prefixes[] wpseo,yoast +phpcs:set Yoast.Files.FileName oo_prefixes[] wpseo,yoast -phpcs:set Yoast.Files.FileName prefixes[] wpseo,yoast +phpcs:set Yoast.Files.FileName oo_prefixes[] wpseo,yoast -phpcs:set Yoast.Files.FileName prefixes[] wpseo,yoast,yoast-plugin +phpcs:set Yoast.Files.FileName oo_prefixes[] wpseo,yoast,yoast-plugin -phpcs:set Yoast.Files.FileName exclude[] excluded-file.inc +phpcs:set Yoast.Files.FileName excluded_files_strict_check[] excluded-file.inc -phpcs:set Yoast.Files.FileName exclude[] excluded_file.inc +phpcs:set Yoast.Files.FileName excluded_files_strict_check[] excluded_file.inc -phpcs:set Yoast.Files.FileName exclude[] functions/excluded-functions-file.inc +phpcs:set Yoast.Files.FileName excluded_files_strict_check[] functions/excluded-functions-file.inc -phpcs:set Yoast.Files.FileName prefixes[] wpseo,yoast +phpcs:set Yoast.Files.FileName oo_prefixes[] wpseo,yoast -phpcs:set Yoast.Files.FileName exclude[] interfaces/excluded-interface-file.inc +phpcs:set Yoast.Files.FileName excluded_files_strict_check[] interfaces/excluded-interface-file.inc -phpcs:set Yoast.Files.FileName prefixes[] wpseo,yoast +phpcs:set Yoast.Files.FileName oo_prefixes[] wpseo,yoast -phpcs:set Yoast.Files.FileName prefixes[] wpseo,yoast +phpcs:set Yoast.Files.FileName oo_prefixes[] wpseo,yoast -phpcs:set Yoast.Files.FileName prefixes[] wpseo,yoast +phpcs:set Yoast.Files.FileName oo_prefixes[] wpseo,yoast -phpcs:set Yoast.Files.FileName prefixes[] wpseo,yoast +phpcs:set Yoast.Files.FileName oo_prefixes[] wpseo,yoast -phpcs:set Yoast.Files.FileName exclude[] no-basepath.inc +phpcs:set Yoast.Files.FileName excluded_files_strict_check[] no-basepath.inc -phpcs:set Yoast.Files.FileName prefixes[] wpseo,yoast +phpcs:set Yoast.Files.FileName oo_prefixes[] wpseo,yoast -phpcs:set Yoast.Files.FileName exclude[] traits/excluded-trait-file.inc +phpcs:set Yoast.Files.FileName excluded_files_strict_check[] traits/excluded-trait-file.inc -phpcs:set Yoast.Files.FileName prefixes[] wpseo,yoast +phpcs:set Yoast.Files.FileName oo_prefixes[] wpseo,yoast -phpcs:set Yoast.Files.FileName prefixes[] wpseo,yoast +phpcs:set Yoast.Files.FileName oo_prefixes[] wpseo,yoast -phpcs:set Yoast.Files.FileName prefixes[] wpseo,yoast +phpcs:set Yoast.Files.FileName oo_prefixes[] wpseo,yoast -phpcs:set Yoast.Files.FileName prefixes[] wpseo,yoast +phpcs:set Yoast.Files.FileName oo_prefixes[] wpseo,yoast Date: Mon, 28 Oct 2019 16:22:47 +0100 Subject: [PATCH 06/29] Ruleset: change minimum versions to PHP 5.6 and WP 5.2 YoastCS 2.0 is intended for the Yoast plugin after the drop of support for WP < 5.2, which means that the minimum supported PHP version for the plugins will now be PHP 5.6 and the minimum supported WP version will be WP 5.2. --- Yoast/ruleset.xml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Yoast/ruleset.xml b/Yoast/ruleset.xml index 652991f1..2c0feaeb 100644 --- a/Yoast/ruleset.xml +++ b/Yoast/ruleset.xml @@ -31,7 +31,7 @@ Ref: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/wiki/Customizable-sniff-properties#minimum-wp-version-to-check-for-usage-of-deprecated-functions-classes-and-function-parameters --> - + @@ -113,7 +113,7 @@ SNIFF FOR PHP CROSS-VERSION COMPATIBILITY ############################################################################# --> - + *\.php$ From 8b162e7b26b645bc11080ca5dd905a7eaabf062f Mon Sep 17 00:00:00 2001 From: jrfnl Date: Mon, 28 Oct 2019 18:52:20 +0100 Subject: [PATCH 07/29] Ruleset: demand short arrays --- Yoast/ruleset.xml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Yoast/ruleset.xml b/Yoast/ruleset.xml index 652991f1..49e7231a 100644 --- a/Yoast/ruleset.xml +++ b/Yoast/ruleset.xml @@ -62,6 +62,9 @@ + + + @@ -108,6 +111,10 @@ + + + + +Admin\Forms; + ]]> + + + admin/forms/file.php --> +Unrelated; + ]]> + + + diff --git a/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php b/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php new file mode 100644 index 00000000..ae400ffd --- /dev/null +++ b/Yoast/Sniffs/NamingConventions/NamespaceNameSniff.php @@ -0,0 +1,358 @@ +filter_allow_only_namespace_prefixes( $prefixes ); + } + + /** + * Processes this test, when one of its tokens is encountered. + * + * @param \PHP_CodeSniffer\Files\File $phpcsFile The file being scanned. + * @param int $stackPtr The position of the current token + * in the stack passed in $tokens. + * + * @return int StackPtr to the end of the file, this sniff needs to only + * check each file once. + */ + public function process( File $phpcsFile, $stackPtr ) { + + $tokens = $phpcsFile->getTokens(); + + if ( empty( $tokens[ $stackPtr ]['conditions'] ) === false ) { + // Not a namespace declaration. + return; + } + + $next_non_empty = $phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true ); + if ( $tokens[ $next_non_empty ]['code'] === T_NS_SEPARATOR ) { + // Not a namespace declaration. + return; + } + + // Get the complete namespace name. + $namespace_name = $tokens[ $next_non_empty ]['content']; + for ( $i = ( $next_non_empty + 1 ); ; $i++ ) { + if ( isset( Tokens::$emptyTokens[ $tokens[ $i ]['code'] ] ) ) { + continue; + } + + if ( $tokens[ $i ]['code'] !== T_STRING && $tokens[ $i ]['code'] !== T_NS_SEPARATOR ) { + // Reached end of the namespace declaration. + break; + } + + $namespace_name .= $tokens[ $i ]['content']; + } + + $this->validate_prefixes(); + + // Strip off the plugin prefix. + $namespace_name_no_prefix = $namespace_name; + $found_prefix = ''; + if ( ! empty( $this->validated_prefixes ) ) { + $name = $namespace_name . '\\'; // Validated prefixes always have a \ at the end. + foreach ( $this->validated_prefixes as $prefix ) { + if ( strpos( $name . '\\', $prefix ) === 0 ) { + $namespace_name_no_prefix = rtrim( substr( $name, strlen( $prefix ) ), '\\' ); + $found_prefix = rtrim( $prefix, '\\' ); + break; + } + } + unset( $prefix, $name ); + } + + /* + * Check the namespace level depth. + */ + if ( $namespace_name_no_prefix !== '' ) { + $namespace_for_level_check = $namespace_name_no_prefix; + // Allow for `Tests\` and `Tests\Doubles\` after the prefix. + if ( strpos( $namespace_for_level_check, 'Tests\\' ) === 0 ) { + $namespace_for_level_check = substr( $namespace_for_level_check, 6 ); + if ( strpos( $namespace_for_level_check, 'Doubles\\' ) === 0 ) { + $namespace_for_level_check = substr( $namespace_for_level_check, 8 ); + } + } + + $parts = explode( '\\', $namespace_for_level_check ); + $part_count = count( $parts ); + + if ( $part_count > $this->max_levels ) { + $error = 'A namespace name is not allowed to be more than %d levels deep (excluding the prefix). Level depth found: %d in %s'; + $data = [ + $this->max_levels, + $part_count, + $namespace_name, + ]; + + $phpcsFile->addError( $error, $stackPtr, 'MaxExceeded', $data ); + } + elseif ( $part_count > $this->recommended_max_levels ) { + $error = 'A namespace name should be no more than %d levels deep (excluding the prefix). Level depth found: %d in %s'; + $data = [ + $this->recommended_max_levels, + $part_count, + $namespace_name, + ]; + + $phpcsFile->addWarning( $error, $stackPtr, 'TooLong', $data ); + } + } + + /* + * Prepare to check the path to level translation. + */ + + if ( ! isset( $phpcsFile->config->basepath ) ) { + // If no basepath is set, we don't know the project root, so bow out. + return; + } + + $base_path = $this->normalize_directory_separators( $phpcsFile->config->basepath ); + + // Stripping potential quotes to ensure `stdin_path` passed by IDEs does not include quotes. + $file = preg_replace( '`^([\'"])(.*)\1$`Ds', '$2', $phpcsFile->getFileName() ); + + if ( $file === 'STDIN' ) { + return; + } + + $directory = $this->normalize_directory_separators( dirname( $file ) ); + $relative_directory = Common::stripBasepath( $directory, $base_path ); + if ( $relative_directory === '.' ) { + $relative_directory = ''; + } + else { + if ( $relative_directory[0] !== '/' ) { + /* + * Basepath stripping appears to work differently depending on OS. + * On Windows there still is a slash at the start, on Unix/Mac there isn't. + * Normalize to allow comparison. + */ + $relative_directory = '/' . $relative_directory; + } + + // Add trailing slash to prevent matching '/sub' to '/sub-directory'. + $relative_directory .= '/'; + } + + $this->validate_src_directory(); + + if ( empty( $this->validated_src_directory ) === false ) { + foreach ( $this->validated_src_directory as $subdirectory ) { + if ( strpos( $relative_directory, $subdirectory ) !== 0 ) { + continue; + } + + $relative_directory = substr( $relative_directory, strlen( $subdirectory ) ); + break; + } + } + + // Now any potential src directory has been stripped, remove the slashes again. + $relative_directory = trim( $relative_directory, '/' ); + + $namespace_name_for_translation = str_replace( + [ '_', '\\' ], // Find. + [ '-', '/' ], // Replace with. + $namespace_name_no_prefix + ); + + if ( strcasecmp( $relative_directory, $namespace_name_for_translation ) === 0 ) { + return; + } + + $expected = ''; + if ( $found_prefix !== '' ) { + $expected = $found_prefix; + } + else { + $expected = '[Plugin\Prefix]'; + } + + if ( $relative_directory !== '' ) { + $levels = explode( '/', $relative_directory ); + $levels = array_filter( $levels ); // Remove empties. + foreach ( $levels as $level ) { + $words = explode( '-', $level ); + $words = array_map( 'ucfirst', $words ); + $expected .= '\\' . implode( '_', $words ); + } + } + + $phpcsFile->addError( + 'The namespace (sub)level(s) should reflect the directory path to the file. Expected: "%s"; Found: "%s"', + $stackPtr, + 'Invalid', + [ + $expected, + $namespace_name, + ] + ); + } + + /** + * Validate a $src_directory property when set in a custom ruleset. + * + * @return void + */ + protected function validate_src_directory() { + if ( $this->previous_src_directory === $this->src_directory ) { + return; + } + + // Set the cache *before* validation so as to not break the above compare. + $this->previous_src_directory = $this->src_directory; + + $src_directory = (array) $this->src_directory; + $src_directory = array_filter( array_map( 'trim', $src_directory ) ); + + if ( empty( $src_directory ) ) { + $this->validated_src_directory = []; + return; + } + + $validated = []; + foreach ( $src_directory as $directory ) { + if ( strpos( $directory, '..' ) !== false ) { + // Do not allow walking up the directory hierarchy. + continue; + } + + $directory = $this->normalize_directory_separators( $directory ); + + if ( $directory === '.' ) { + // The basepath/root directory is the default, so ignore. + continue; + } + + if ( strpos( $directory, './' ) === 0 ) { + $directory = substr( $directory, 2 ); + } + + if ( $directory === '' ) { + continue; + } + + $validated[] = '/' . $directory . '/'; + } + + // Use reverse natural sorting to get the longest directory first. + rsort( $validated, ( SORT_NATURAL | SORT_FLAG_CASE ) ); + + // Set the validated prefixes cache. + $this->validated_src_directory = $validated; + } + + /** + * Normalize all directory separators to be a forward slash and remove prefixed and suffixed slashes. + * + * @param string $path Path to normalize. + * + * @return string + */ + private function normalize_directory_separators( $path ) { + return trim( strtr( $path, '\\', '/' ), '/' ); + } +} diff --git a/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.inc b/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.inc new file mode 100644 index 00000000..b3d9bbc7 --- /dev/null +++ b/Yoast/Tests/NamingConventions/NamespaceNameUnitTest.inc @@ -0,0 +1 @@ +basepath = __DIR__ . DIRECTORY_SEPARATOR . 'NamespaceNameUnitTests'; + } + + /** + * Get a list of all test files to check. + * + * @param string $testFileBase The base path that the unit tests files will have. + * + * @return string[] + */ + protected function getTestFiles( $testFileBase ) { + $sep = DIRECTORY_SEPARATOR; + $test_files = glob( dirname( $testFileBase ) . $sep . 'NamespaceNameUnitTests{' . $sep . ',' . $sep . '*' . $sep . '}*.inc', GLOB_BRACE ); + + if ( ! empty( $test_files ) ) { + return $test_files; + } + + return [ $testFileBase . '.inc' ]; + } + + /** + * Returns the lines where errors should occur. + * + * @param string $testFile The name of the file being tested. + * + * @return array => + */ + public function getErrorList( $testFile = '' ) { + + switch ( $testFile ) { + // Level check tests. + case 'no-basepath.inc': + return [ + 12 => 1, + 21 => 1, + 24 => 1, + 33 => 1, + 44 => 1, + 53 => 1, + 54 => 1, + 66 => 1, + 70 => 1, + 74 => 1, + ]; + + case 'no-basepath-scoped.inc': + return [ + 14 => 1, + 24 => 1, + ]; + + // Basic path translation tests. + case 'path-translation-root.inc': + return [ + 11 => 1, + 14 => 1, + ]; + + case 'path-translation-sub1.inc': + return [ + 11 => 1, + 12 => 1, + 13 => 1, + ]; + + case 'path-translation-sub2.inc': + return [ + 12 => 1, + 13 => 1, + 14 => 1, + 15 => 1, + ]; + + // Path translation with $src_directory set tests. + case 'path-translation-src.inc': + return [ + 12 => 1, + ]; + + case 'path-translation-src-sub-a.inc': + return [ + 13 => 1, + ]; + + case 'path-translation-src-sub-b.inc': + return [ + 14 => 1, + ]; + + // Path translation with multiple items in $src_directory tests. + case 'path-translation-secondary.inc': + return [ + 13 => 1, + ]; + + case 'path-translation-secondary-sub-a.inc': + return [ + 12 => 1, + ]; + + // Path translation with multi-level item in $src_directory tests. + case 'path-translation-ignore-src.inc': + return [ + 12 => 1, + ]; + + case 'path-translation-ignore-src-sub-path.inc': + return [ + 12 => 1, + 13 => 1, + 14 => 1, + ]; + + // Path translation with no matching $src_directory. + case 'path-translation-mismatch.inc': + return [ + 13 => 1, + ]; + + case 'path-translation-mismatch-illegal.inc': + return [ + 12 => 1, + ]; + + default: + return []; + } + } + + /** + * Returns the lines where warnings should occur. + * + * @param string $testFile The name of the file being tested. + * + * @return array => + */ + public function getWarningList( $testFile = '' ) { + switch ( $testFile ) { + // Level check tests. + case 'no-basepath.inc': + return [ + 8 => 1, + 20 => 1, + 23 => 1, + 32 => 1, + 43 => 1, + 65 => 1, + 69 => 1, + 72 => 1, + 73 => 1, + ]; + + case 'no-basepath-scoped.inc': + return [ + 13 => 1, + ]; + + case 'path-translation-ignore-src-sub-path.inc': + return [ + 14 => 1, + ]; + + default: + return []; + } + } +} diff --git a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/ignore/src/path-translation-ignore-src.inc b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/ignore/src/path-translation-ignore-src.inc new file mode 100644 index 00000000..6c7a78cc --- /dev/null +++ b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/ignore/src/path-translation-ignore-src.inc @@ -0,0 +1,16 @@ + max). + */ +// phpcs:set Yoast.NamingConventions.NamespaceName max_levels 2 +// phpcs:set Yoast.NamingConventions.NamespaceName recommended_max_levels 5 + +namespace Yoast\WP\Plugin\Foo\Bar; // OK. +namespace Yoast\WP\Plugin\Foo\Bar\Baz; // Error. +namespace Yoast\WP\Plugin\Foo\Bar\Baz\Fro; // Error. + +// Reset to default settings. +// phpcs:set Yoast.NamingConventions.NamespaceName max_levels 3 +// phpcs:set Yoast.NamingConventions.NamespaceName recommended_max_levels 2 + +/* + * Test allowance for `Tests\` and `Tests\Doubles\` just after the prefix. + */ + +namespace Yoast\WP\Plugin\Tests\Foo\Bar; // OK. +namespace Yoast\WP\Plugin\Tests\Foo\Bar\Baz; // Warning. +namespace Yoast\WP\Plugin\Tests\Foo\Bar\Baz\Fro; // Error. + +namespace Yoast\WP\Plugin\Tests\Doubles\Foo\Bar; // OK. +namespace Yoast\WP\Plugin\Tests\Doubles\Foo\Bar\Baz; // Warning. +namespace Yoast\WP\Plugin\Tests\Doubles\Foo\Bar\Baz\Fro; // Error. + +namespace Yoast\WP\Plugin\Foo\Tests\Bar; // Warning, `Tests\` counted as not directly after the prefix. +namespace Yoast\WP\Plugin\Tests\Foo\Doubles\Bar; // Warning, `Doubles\` counted as not directly after the prefix + `Tests\`. +namespace Yoast\WP\Plugin\Doubles\Foo\Bar\Fro; // Error, `Doubles\` counted as not found in combination with `Tests\`. + +// Reset to default settings. +// phpcs:set Yoast.NamingConventions.NamespaceName prefixes[] + +/* + * Test against false positives for namespace operator and incorrect namespace declarations. + */ + +if ( $condition ) { + namespace Foo\Bar\Baz\Fro; // Ignore. Parse error. +} + +echo namespace\function_call(); // Ignore. Operator, not keyword. diff --git a/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/path-translation-root.inc b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/path-translation-root.inc new file mode 100644 index 00000000..cd40436c --- /dev/null +++ b/Yoast/Tests/NamingConventions/NamespaceNameUnitTests/path-translation-root.inc @@ -0,0 +1,17 @@ + Date: Fri, 25 Oct 2019 18:37:52 +0200 Subject: [PATCH 17/29] PHPCS: adjust project ruleset to allow for the new NamespaceName sniff --- .phpcs.xml.dist | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.phpcs.xml.dist b/.phpcs.xml.dist index 9c697007..01971909 100644 --- a/.phpcs.xml.dist +++ b/.phpcs.xml.dist @@ -40,4 +40,12 @@ + + + + + + + + From 8165fe21f51fa5bb55b183bfd4757b66a3a7a1a4 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Fri, 31 May 2019 17:03:55 +0200 Subject: [PATCH 18/29] Various minor documentation fixes --- Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php | 2 +- Yoast/Sniffs/Files/FileNameSniff.php | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php b/Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php index e48cd7c6..c8ec5527 100644 --- a/Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php +++ b/Yoast/Sniffs/Commenting/TestsHaveCoversTagSniff.php @@ -60,7 +60,7 @@ public function process( File $phpcsFile, $stackPtr ) { * in the stack passed in $tokens. * * @return void|int If covers annotations were found (or this is not a test class), - * will returns the stack pointer to the end of the class. + * will return the stack pointer to the end of the class. */ protected function process_class( File $phpcsFile, $stackPtr ) { $tokens = $phpcsFile->getTokens(); diff --git a/Yoast/Sniffs/Files/FileNameSniff.php b/Yoast/Sniffs/Files/FileNameSniff.php index d25e386a..5e499e2d 100644 --- a/Yoast/Sniffs/Files/FileNameSniff.php +++ b/Yoast/Sniffs/Files/FileNameSniff.php @@ -7,8 +7,6 @@ use PHP_CodeSniffer\Util\Common; /** - * YoastCS\Yoast\Sniffs\Files\FileNameSniff. - * * Ensures files comply with the Yoast file name rules. * * Rules: From bc81985c253185ef7f5e768e5e71ee25a391c57e Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 1 Dec 2019 17:49:56 +0100 Subject: [PATCH 19/29] Ruleset: disallow Yoda conditions As proposed in 166 and possible now the minimum PHPCS version is 3.5.0, this adds a sniff to disallow Yoda conditions to YoastCS. Yoda conditions are enforced by WPCS, but this rule has been turned off in YoastCS from the very beginning, however, non-Yoda conditions were not enforced up to now, leading to inconsistency. Prior to PHPCS 3.5.2 this sniff was still quite buggy, but it looks like most bugs have been addressed in the mean time. Fixes 166 --- Yoast/ruleset.xml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Yoast/ruleset.xml b/Yoast/ruleset.xml index 34533c98..2cf4e987 100644 --- a/Yoast/ruleset.xml +++ b/Yoast/ruleset.xml @@ -114,6 +114,9 @@ + + + + + + From c4af0342fc96dfc1aeebc8b45cb36e23594bf303 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 1 Dec 2019 18:05:26 +0100 Subject: [PATCH 21/29] Ruleset: disallow "// end" comments In the olden days, before there were good IDEs, it was often customary to have `// End class`/`// End function` etc comments on the line of a class/function/control structure closing brace to make its easier to find the end of such a structure. As modern IDEs are perfectly capable of handling this and handle this well, those type of comments are no longer necessary and can be considered unnecessary "noise". The sniff which is proposed to be added via this PR enforces that closing braces of classes/interfaces/traits/functions are not followed by a comment or statement, in line with PSR-12. --- Yoast/ruleset.xml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Yoast/ruleset.xml b/Yoast/ruleset.xml index 34533c98..d41289a1 100644 --- a/Yoast/ruleset.xml +++ b/Yoast/ruleset.xml @@ -144,6 +144,9 @@ + + + From 76dde7f4f6f7bbdddc583a3bbdf4e4ded66e885d Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 1 Dec 2019 17:54:19 +0100 Subject: [PATCH 22/29] Ruleset: fix links to WPCS repo The WPCS repo has moved to the WordPress organization, so the canonical address for the repo has changed. --- CHANGELOG.md | 2 +- README.md | 2 +- Yoast/ruleset.xml | 14 +++++++------- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e7b4254..d8902447 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -285,7 +285,7 @@ Initial public release as a stand-alone package. [PHP_CodeSniffer]: https://github.com/squizlabs/PHP_CodeSniffer/releases -[WordPressCS]: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/blob/develop/CHANGELOG.md +[WordPressCS]: https://github.com/WordPress/WordPress-Coding-Standards/blob/develop/CHANGELOG.md [PHPCompatibilityWP]: https://github.com/PHPCompatibility/PHPCompatibilityWP#changelog [PHPCompatibility]: https://github.com/PHPCompatibility/PHPCompatibility/blob/master/CHANGELOG.md [PHP Mess Detector]: https://github.com/phpmd/phpmd/blob/master/CHANGELOG diff --git a/README.md b/README.md index 525f1a84..b52c57ab 100644 --- a/README.md +++ b/README.md @@ -36,7 +36,7 @@ Severity levels: ### The YoastCS Standard The `Yoast` standard for PHP_CodeSniffer is comprised of the following: -* The `WordPress` ruleset from the [WordPress Coding Standards](https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards) implementing the official [WordPress PHP Coding Standards](https://make.wordpress.org/core/handbook/coding-standards/php/), with some [select exclusions](https://github.com/Yoast/yoastcs/blob/develop/Yoast/ruleset.xml#L29-L75). +* The `WordPress` ruleset from the [WordPress Coding Standards](https://github.com/WordPress/WordPress-Coding-Standards) implementing the official [WordPress PHP Coding Standards](https://make.wordpress.org/core/handbook/coding-standards/php/), with some [select exclusions](https://github.com/Yoast/yoastcs/blob/develop/Yoast/ruleset.xml#L29-L75). * The [`PHPCompatibilityWP`](https://github.com/PHPCompatibility/PHPCompatibilityWP) ruleset which checks code for PHP cross-version compatibility while preventing false positives for functionality polyfilled within WordPress. * Select additional sniffs taken from [`PHP_CodeSniffer`](https://github.com/squizlabs/PHP_CodeSniffer). * A number of custom Yoast specific sniffs. diff --git a/Yoast/ruleset.xml b/Yoast/ruleset.xml index 34533c98..bed326a8 100644 --- a/Yoast/ruleset.xml +++ b/Yoast/ruleset.xml @@ -28,7 +28,7 @@ --> @@ -145,18 +145,18 @@ - + - + - + From 0c2d65598f055bd9e0f772ab4aeaaf9b54159f45 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Mon, 2 Dec 2019 16:06:10 +0100 Subject: [PATCH 23/29] PHPCS: update project native ruleset for YoastCS 2.0.0 As demanding short arrays is now included in the YoastCS ruleset since 159, the project/repo specific configuration no longer needs to demand it. --- .phpcs.xml.dist | 8 -------- 1 file changed, 8 deletions(-) diff --git a/.phpcs.xml.dist b/.phpcs.xml.dist index 01971909..17542c91 100644 --- a/.phpcs.xml.dist +++ b/.phpcs.xml.dist @@ -27,19 +27,11 @@ - - - - - - - From 0eabd1c044990a3998442da45cb95327d17505cc Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 1 Dec 2019 20:05:13 +0100 Subject: [PATCH 24/29] Ruleset: enforce consistent indentation of chained method calls When object method calls are chained over multiple lines, indentation should always be one more or less than the previous and always at least one in from the start of the chain. This addition enforces that. Example of correct chain indentation: ```php $obj->method() ->foo() ->bar() ->select(); ``` Example of incorrect chain indentation ```php $obj->method() ->foo() ->bar() ->select(); ``` --- Yoast/ruleset.xml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Yoast/ruleset.xml b/Yoast/ruleset.xml index e85fa18b..d2397819 100644 --- a/Yoast/ruleset.xml +++ b/Yoast/ruleset.xml @@ -150,6 +150,13 @@ + + + + + + + From 0d7da3238a97a563bb40c457a4f63a9ecb45fcce Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 1 Dec 2019 20:07:31 +0100 Subject: [PATCH 25/29] Ruleset: disallow leading backslashes for import use statements An import `use` statement must always be fully qualified, so a leading backslash is redundant and discouraged by PHP itself. This sniff addition enforces this and is expected to go into WPCS at some point in the future. Correct: ```php namespace A\B; use B\C; ``` Incorrect: ```php namespace A\B; use \B\C; ``` --- Yoast/ruleset.xml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Yoast/ruleset.xml b/Yoast/ruleset.xml index e85fa18b..413eede1 100644 --- a/Yoast/ruleset.xml +++ b/Yoast/ruleset.xml @@ -171,6 +171,10 @@ Related: https://github.com/WordPress/WordPress-Coding-Standards/issues/1762 --> + + + + From 5a13cd73ccc072853fb6c021357793e63758eb0d Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 1 Dec 2019 20:08:36 +0100 Subject: [PATCH 26/29] Ruleset: enforce PHP open tag on a line by itself This is partially enforced already, this just safeguards it for the edge cases which weren't enforced yet. --- Yoast/ruleset.xml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Yoast/ruleset.xml b/Yoast/ruleset.xml index 413eede1..f82bfadf 100644 --- a/Yoast/ruleset.xml +++ b/Yoast/ruleset.xml @@ -175,6 +175,10 @@ + + + + From b6a7e3bd3b3299c8965a47179bb5916f7af50e11 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Thu, 24 Oct 2019 00:45:47 +0200 Subject: [PATCH 27/29] :sparkles: New `ValidHookName` sniff This new sniff extends the upstream WPCS `ValidHookName` sniff and overloads the `transform()` method to allow for the new Yoast specific hook prefix requirements as per issue 143. Hooks in the Yoast plugins should still comply with the "_hook names should be lowercase with words separated by underscores_" rule, but the `Yoast\WP\PluginName` prefix should be exempt. The overload of the `transform()` method prevents the upstream sniff from throwing errors for the prefix, while still allowing it to throw errors for the actual hook name. Also, and in contrast to the WPCS native sniff, when the YoastCS version of this sniff has received valid prefix(es), the hook name checks will only be executed for hooks which have a valid prefix. Non-prefixed hooks or hooks with another prefix will be ignored to prevent errors being thrown about hook names which are outside of our control. The sniff uses the `CustomPrefixesTrait` to allow setting the `public` `$prefixes` property and to validate the input received for this property. In addition to this, the new/extended sniff adds a couple of new checks: - A check that the "namespace"-style prefix is used. The sniff allows for passing both "namespace"-like, as well as "old-style" prefixes to allow for the transition period. However, if an "old-style" prefix is used, rather than the "namespace"-like prefix, a `warning` will be thrown. - A check to verify that hook names contain no more than four words / three underscores, after the plugin specific prefix. - The sniff will throw a `warning` when the name consists of more words than `soft_maximum_depth` and an `error` when the name is longer than `maximum_depth`. - Both `soft_maximum_depth`, as well as `maximum_depth` are configurable and can be changed from a custom ruleset. By default, `soft_maximum_depth` and `maximum_depth` are set to the same value: `4`, i.e. four words. Note: For dynamic hook names where the hook name length can not reliably be determined, the sniff will throw a `warning` at severity `3` suggesting the hook name be inspected manually. As the default severity for PHPCS is `5`, this `warning` at severity `3` will normally not be shown. It is there to allow for intermittently checking of the dynamic hook names. To trigger it, `--severity=3` should be passed on the command line. Also note: Just like the lowercase/underscore hook name checks, the hook name length check will only be executed when the hook name starts with (one of) the plugin specific prefix. Includes unit tests. Includes documentation. Fixes 143 --- .../ValidHookNameStandard.xml | 63 ++++ .../NamingConventions/ValidHookNameSniff.php | 317 ++++++++++++++++++ .../ValidHookNameUnitTest.inc | 124 +++++++ .../ValidHookNameUnitTest.php | 83 +++++ Yoast/ruleset.xml | 3 + 5 files changed, 590 insertions(+) create mode 100644 Yoast/Docs/NamingConventions/ValidHookNameStandard.xml create mode 100644 Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php create mode 100644 Yoast/Tests/NamingConventions/ValidHookNameUnitTest.inc create mode 100644 Yoast/Tests/NamingConventions/ValidHookNameUnitTest.php diff --git a/Yoast/Docs/NamingConventions/ValidHookNameStandard.xml b/Yoast/Docs/NamingConventions/ValidHookNameStandard.xml new file mode 100644 index 00000000..4761d4fa --- /dev/null +++ b/Yoast/Docs/NamingConventions/ValidHookNameStandard.xml @@ -0,0 +1,63 @@ + + + + + + + + 'Yoast\WP\Plugin\hook_name', $var ); + ]]> + + + 'Yoast\WP\Plugin\Hook_NAME', $var ); + ]]> + + + + + 'Yoast\WP\Plugin\hook_name', + $var +); + ]]> + + + 'Yoast\WP\Plugin\some/hook-name', + $var +); + ]]> + + + + + + + + 'Yoast\WP\Plugin\hook_name' +); + ]]> + + + 'Yoast\WP\Plugin\long_hook_name_too_long' +); + ]]> + + + diff --git a/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php b/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php new file mode 100644 index 00000000..13c5a7a4 --- /dev/null +++ b/Yoast/Sniffs/NamingConventions/ValidHookNameSniff.php @@ -0,0 +1,317 @@ +remove_prefix = true; + $this->found_prefix = ''; + $this->first_string = ''; + $this->validate_prefixes(); + + /* + * If any prefixes were passed, check if this is a hook belonging to the plugin being checked. + */ + if ( empty( $this->validated_prefixes ) === false ) { + $param = $parameters[1]; + $first_non_empty = $this->phpcsFile->findNext( Tokens::$emptyTokens, $param['start'], ( $param['end'] + 1 ), true ); + $found_prefix = ''; + + if ( isset( Tokens::$stringTokens[ $this->tokens[ $first_non_empty ]['code'] ] ) ) { + $content = trim( $this->strip_quotes( $this->tokens[ $first_non_empty ]['content'] ) ); + foreach ( $this->validated_prefixes as $prefix ) { + if ( strpos( $content, $prefix ) === 0 ) { + $found_prefix = $prefix; + break; + } + } + } + + if ( $found_prefix === '' ) { + /* + * Not a hook name with a prefix indicating it belongs to the specific plugin + * being checked. Ignore as it's probably a WP Core or external plugin hook name + * which we cannot change. + */ + return; + } + } + + // Do the WPCS native hook name check. + parent::process_parameters( $stackPtr, $group_name, $matched_content, $parameters ); + + if ( $this->found_prefix === '' ) { + return; + } + + // Do the YoastCS specific hook name length and prefix check. + $this->verify_yoast_hook_name( $stackPtr, $parameters ); + } + + /** + * Transform an arbitrary string to lowercase and replace punctuation and spaces with underscores. + * + * This overloads the parent to prevent errors being triggered on the Yoast specific + * plugin prefix for hook names and remembers whether a prefix was found to allow + * checking whether it was the correct one. + * + * @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. + * 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 ); + } + + // Not the first text string. + if ( $this->remove_prefix === false + && $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; + } + + return $this->found_prefix . parent::transform( $string, $regex, $transform_type ); + } + + /** + * Additional YoastCS specific hook name checks. + * + * @param int $stackPtr The position of the current token in the stack. + * @param array $parameters Array with information about the parameters. + * + * @return void + */ + public function verify_yoast_hook_name( $stackPtr, $parameters ) { + + $param = $parameters[1]; + $first_non_empty = $this->phpcsFile->findNext( Tokens::$emptyTokens, $param['start'], ( $param['end'] + 1 ), true ); + + /* + * Check that the namespace-like prefix is used for hooks. + */ + if ( strpos( $this->found_prefix, '\\' ) === false ) { + /* + * Find which namespace-based prefix should have been used. + * Loop till the end as the shortest prefix will be last. + */ + $namespace_prefix = ''; + foreach ( $this->validated_prefixes as $prefix ) { + if ( strpos( $prefix, '\\' ) !== false ) { + $namespace_prefix = $prefix; + } + } + + $this->phpcsFile->addWarning( + 'Wrong prefix type used. Hook names should use the "%s" namespace-like prefix. Found prefix: %s', + $first_non_empty, + 'WrongPrefix', + [ + $namespace_prefix, + $this->found_prefix, + ] + ); + } + + /* + * Check if the hook name is a single quoted string. + */ + $allow = [ \T_CONSTANT_ENCAPSED_STRING ]; + $allow += Tokens::$emptyTokens; + + $has_non_string = $this->phpcsFile->findNext( $allow, $param['start'], ( $param['end'] + 1 ), true ); + if ( $has_non_string !== false ) { + /* + * Double quoted string or a hook name concatenated together, checking the word count for the + * hook name can not be done in a reliable manner. + * + * Throwing a warning to allow for examining these if desired. + * Severity 3 makes sure that this warning will normally be invisible and will only + * be thrown when PHPCS is explicitly requested to check with a lower severity. + */ + $this->phpcsFile->addWarning( + 'Hook name could not reliably be examined for maximum word count. Please verify this hook name manually. Found: %s', + $first_non_empty, + 'NonString', + [ $param['raw'] ], + 3 + ); + + return; + } + + /* + * Check the hook name depth. + */ + $hook_ptr = $first_non_empty; // If no other tokens were found, the first non empty will be the hook name. + $hook_name = $this->strip_quotes( $this->tokens[ $hook_ptr ]['content'] ); + $hook_name = substr( $hook_name, strlen( $this->found_prefix ) ); + + $parts = explode( '_', $hook_name ); + $part_count = count( $parts ); + + if ( $part_count <= $this->recommended_max_words && $part_count <= $this->max_words ) { + return; + } + + if ( $part_count > $this->max_words ) { + $error = 'A hook name is not allowed to consist of more than %d words after the plugin prefix. Words found: %d in %s'; + $data = [ + $this->max_words, + $part_count, + $this->tokens[ $hook_ptr ]['content'], + ]; + + $this->phpcsFile->addError( $error, $hook_ptr, 'MaxExceeded', $data ); + } + elseif ( $part_count > $this->recommended_max_words ) { + $error = 'A hook name should not consist of more than %d words after the plugin prefix. Words found: %d in %s'; + $data = [ + $this->recommended_max_words, + $part_count, + $this->tokens[ $hook_ptr ]['content'], + ]; + + $this->phpcsFile->addWarning( $error, $hook_ptr, 'TooLong', $data ); + } + } +} diff --git a/Yoast/Tests/NamingConventions/ValidHookNameUnitTest.inc b/Yoast/Tests/NamingConventions/ValidHookNameUnitTest.inc new file mode 100644 index 00000000..951efee9 --- /dev/null +++ b/Yoast/Tests/NamingConventions/ValidHookNameUnitTest.inc @@ -0,0 +1,124 @@ +ID}_Action" ); // Error - use lowercase. +do_action( "admin_Head_{$obj->Values[3]->name}-Action_{$obj->Values[3]->name}_Action" ); // Error - use lowercase + warning about dash. + +/* + * Verify the Yoast specific changes to allow for the plugin prefix, while still checking + * that the rest of the hook name is lowercase and underscore separated. + * + * @phpcs:disable Yoast.NamingConventions.ValidHookName.WrongPrefix + */ + +// phpcs:set Yoast.NamingConventions.ValidHookName prefixes[] Yoast\WP\Plugin,yoast_plugin + +apply_filters_ref_array( 'yoast_plugin_some_hook_name', $var ); // OK. +apply_filters( 'Yoast\WP\Plugin\some_hook_name', $var ); // OK. + +do_action_ref_array( 'Yoast\WP\Plugin\some-hook\name', $var ); // Warning - disallowed word separators (after prefix). +apply_filters( 'Yoast\WP\Plugin\some_HookName', $var ); // Error - uppercase chars (after prefix). +apply_filters_ref_array( 'yoast_plugin_Some-Hook-Name', $var ); // Error + warning, uppercase chars + dashes (after prefix). + +// OK, ignored. Hooks do not start with correct prefix, while valid prefixes have been passed. +do_action( 'Yoast\WP\AnotherPlugin\some_hook_name', $var ); +apply_filters( 'some_hook_name_yoast_plugin_Prefix_not-at_start', $var ); + +// Second use of the prefix (correctly) not taken into account as not found at the start of the hook name. +do_action( 'Yoast\WP\Plugin\hook_' . $type . 'Yoast\WP\Plugin\hook' ); // Error + warning x 2, prefix repeated, compound name. + +// Test handing of compound hook names with the prefix as stand-alone string. +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. + +// phpcs:enable + +/* + * Test Yoast specific sniff additions for checking hook name length and such. + * + * These checks are only in effect if a prefix is found. + * The WPCS PrefixAllGlobals sniff checks that a prefix is used, that's outside the scope of this sniff. + */ + +/* + * Simple strings, no prefix. Includes testing handling of comments. + */ +do_action( + // phpcs:ignore Stnd.Cat.Sniff -- For reasons. + 'some_hook_name_too_long' /* comment */ +); // OK. Plugin prefix not found, so ignored. + +/* + * Non simple strings. + */ +// 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_plugin_some_' . 'hook_' . 'name' ); // Warning at severity 3 + warning wrong prefix. + +/* + * Test passing "unclean" prefixes property. + */ +// phpcs:set Yoast.NamingConventions.ValidHookName prefixes[] \Yoast\WP\Plugin\,_yoast_plugin_ + +apply_filters( 'Yoast\WP\Plugin\some_hook_name', $var ); // OK. +apply_filters_ref_array( 'yoast_plugin_some_hook_name', $var ); // Warning - wrong prefix. + +apply_filters( 'Yoast\WP\Plugin\some_hook_name_too_long', $var ); // Error - too long. +do_action_ref_array( "yoast_plugin_some_hook_name_too_long", $var ); // Error - too long + warning - wrong prefix. + +/* + * Testing with clean prefixes. + * + * Passing both old-style and new-style prefixes during the transition period. + */ +// phpcs:set Yoast.NamingConventions.ValidHookName prefixes[] Yoast\WP\Plugin,Yoast\WP\Plugin\Test,yoast_plugin + +do_action( 'Yoast\WP\Plugin\Test\some_hook_name', $var ); // OK. +do_action_ref_array( "yoast_plugin_some_hook_name", $var ); // Warning - wrong prefix. + +apply_filters( 'Yoast\WP\Plugin\some_hook_name_too_long', $var ); // Error - too long. +do_action_ref_array( "yoast_plugin_some_hook_name_too_long", $var ); // Error - too long + warning - wrong prefix. + +/* + * Custom word maximums. + */ +// phpcs:set Yoast.NamingConventions.ValidHookName max_words 5 +// phpcs:set Yoast.NamingConventions.ValidHookName recommended_max_words 2 + +apply_filters( 'Yoast\WP\Plugin\some_hook', $var ); // OK. + +do_action( 'Yoast\WP\Plugin\some_hook_name', $var ); // Warning - over recommended length. +do_action_ref_array( 'Yoast\WP\Plugin\some_hook_name_which_is_too_long', $var ); // Error. + +do_action( 'yoast_plugin_some_hook_name', $var ); // Warning x 2 - over recommended length + wrong prefix. +do_action_ref_array( 'yoast_plugin_some_hook_name_which_is_too_long', $var ); // Error - length + warning - wrong prefix.. + +/* + * Incorrect custom settings (soft > max). + */ +// phpcs:set Yoast.NamingConventions.ValidHookName max_words 2 +// phpcs:set Yoast.NamingConventions.ValidHookName recommended_max_words 5 + +do_action( 'Yoast\WP\Plugin\some_hook_name', $var ); // Error. + +// Reset to default settings. +// phpcs:set Yoast.NamingConventions.ValidHookName prefixes[] +// phpcs:set Yoast.NamingConventions.ValidHookName max_words 4 +// phpcs:set Yoast.NamingConventions.ValidHookName recommended_max_words 4 diff --git a/Yoast/Tests/NamingConventions/ValidHookNameUnitTest.php b/Yoast/Tests/NamingConventions/ValidHookNameUnitTest.php new file mode 100644 index 00000000..d978c37e --- /dev/null +++ b/Yoast/Tests/NamingConventions/ValidHookNameUnitTest.php @@ -0,0 +1,83 @@ +warningSeverity = 3; + } + + /** + * Returns the lines where errors should occur. + * + * @return array => + */ + public function getErrorList() { + + return [ + 14 => 1, + 15 => 1, + 17 => 1, + 18 => 1, + 19 => 1, + 34 => 1, + 35 => 1, + 42 => 1, + 83 => 1, + 84 => 1, + 96 => 1, + 97 => 1, + 108 => 1, + 111 => 1, + 119 => 1, + ]; + } + + /** + * Returns the lines where warnings should occur. + * + * @return array => + */ + public function getWarningList() { + return [ + 16 => 1, + 19 => 1, + 33 => 1, + 35 => 1, + 42 => 2, + 45 => 1, // Severity: 3. + 48 => 1, // Severity: 3. + 72 => 1, // Severity: 3. + 73 => 2, // Severity: 3 + 5. + 81 => 1, + 84 => 1, + 94 => 1, + 97 => 1, + 107 => 1, + 110 => 2, + 111 => 1, + ]; + } +} + diff --git a/Yoast/ruleset.xml b/Yoast/ruleset.xml index ab77a953..9aaa2554 100644 --- a/Yoast/ruleset.xml +++ b/Yoast/ruleset.xml @@ -45,6 +45,9 @@ + + +