diff --git a/.readme-partials/USING.md b/.readme-partials/USING.md index 94a96d63..6b070a72 100644 --- a/.readme-partials/USING.md +++ b/.readme-partials/USING.md @@ -199,24 +199,34 @@ Unlike the analysis above, a docstring that merely opens with ` - + ``` +Starting from `WP_CLI_CS_Feature_Files`, as above, keeps the defaults and adds to them. Starting +from `WP_CLI_CS` instead gives up all of them, including the space indentation, and leaves the +package to say for itself what a padded block cannot satisfy. + The blocks are left alone when a run is narrowed down to a path, as in `composer phpcs -- src/`, since such an argument is about the files of the package itself. diff --git a/README.md b/README.md index ba07708c..7a48d30f 100644 --- a/README.md +++ b/README.md @@ -210,24 +210,34 @@ Unlike the analysis above, a docstring that merely opens with ` - + ``` +Starting from `WP_CLI_CS_Feature_Files`, as above, keeps the defaults and adds to them. Starting +from `WP_CLI_CS` instead gives up all of them, including the space indentation, and leaves the +package to say for itself what a padded block cannot satisfy. + The blocks are left alone when a run is narrowed down to a path, as in `composer phpcs -- src/`, since such an argument is about the files of the package itself. diff --git a/WP_CLI_CS_Feature_Files/ruleset.xml b/WP_CLI_CS_Feature_Files/ruleset.xml new file mode 100644 index 00000000..d753252c --- /dev/null +++ b/WP_CLI_CS_Feature_Files/ruleset.xml @@ -0,0 +1,93 @@ + + + + Coding standard for the PHP blocks embedded in Behat feature files + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/bin/run-phpcbf-cleanup b/bin/run-phpcbf-cleanup index 677a13ec..f4c7b458 100755 --- a/bin/run-phpcbf-cleanup +++ b/bin/run-phpcbf-cleanup @@ -25,8 +25,8 @@ done DIR="$(cd -P "$(dirname "$SOURCE")/.." && pwd)" # A ruleset of the same purpose in the package root replaces the defaults -# wholesale. Both scripts read the defaults from the same file, so that the -# check and the fixer cannot disagree over which sniff applies to a block. +# wholesale. Both scripts use the same ruleset otherwise, so that the check and +# the fixer cannot disagree over which sniff applies to a block. FEATURE_STANDARD="" for CANDIDATE in "phpcs-feature-files.xml" "phpcs-feature-files.xml.dist" do @@ -37,13 +37,9 @@ do fi done -FEATURE_ARGS="" -if [ -z "$FEATURE_STANDARD" ] && [ -f "$DIR/phpcs/feature-files.sh" ] +if [ -z "$FEATURE_STANDARD" ] then - . "$DIR/phpcs/feature-files.sh" - FEATURE_STANDARD="$WP_CLI_TESTS_FEATURE_STANDARD" - # Holds no path, so leaving it unquoted below splits it into arguments. - FEATURE_ARGS="$WP_CLI_TESTS_FEATURE_ARGS" + FEATURE_STANDARD="WP_CLI_CS_Feature_Files" fi # An argument naming what to fix applies to the files of the package itself, so @@ -57,7 +53,7 @@ do esac done -if [ "$FIX_BLOCKS" -eq 1 ] && [ -d "features" ] && [ -n "$FEATURE_STANDARD" ] \ +if [ "$FIX_BLOCKS" -eq 1 ] && [ -d "features" ] \ && [ -f "$DIR/utils/extract-feature-php.php" ] then TEMP_DIR=$(mktemp -d 2>/dev/null || mktemp -d -t 'feature_phpcbf') @@ -68,8 +64,7 @@ then then if [ -n "$(ls -A "$TEMP_DIR" 2>/dev/null)" ] then - # shellcheck disable=SC2086 # Intentional word splitting. - vendor/bin/phpcbf --standard="$FEATURE_STANDARD" $FEATURE_ARGS \ + vendor/bin/phpcbf --standard="$FEATURE_STANDARD" \ "$TEMP_DIR" >/dev/null || EXIT_CODE=$? php "$DIR/utils/extract-feature-php.php" update features "$TEMP_DIR" >/dev/null || EXIT_CODE=$? diff --git a/bin/run-phpcs-tests b/bin/run-phpcs-tests index f1a632a9..3c2a9f04 100755 --- a/bin/run-phpcs-tests +++ b/bin/run-phpcs-tests @@ -44,8 +44,8 @@ done DIR="$(cd -P "$(dirname "$SOURCE")/.." && pwd)" # A ruleset of the same purpose in the package root replaces the defaults -# wholesale. Both scripts read the defaults from the same file, so that the -# check and the fixer cannot disagree over which sniff applies to a block. +# wholesale. Both scripts use the same ruleset otherwise, so that the check and +# the fixer cannot disagree over which sniff applies to a block. FEATURE_STANDARD="" for CANDIDATE in "phpcs-feature-files.xml" "phpcs-feature-files.xml.dist" do @@ -56,13 +56,9 @@ do fi done -FEATURE_ARGS="" -if [ -z "$FEATURE_STANDARD" ] && [ -f "$DIR/phpcs/feature-files.sh" ] +if [ -z "$FEATURE_STANDARD" ] then - . "$DIR/phpcs/feature-files.sh" - FEATURE_STANDARD="$WP_CLI_TESTS_FEATURE_STANDARD" - # Holds no path, so leaving it unquoted below splits it into arguments. - FEATURE_ARGS="$WP_CLI_TESTS_FEATURE_ARGS" + FEATURE_STANDARD="WP_CLI_CS_Feature_Files" fi # An argument naming what to check applies to the files of the package itself, @@ -76,7 +72,7 @@ do esac done -if [ "$CHECK_BLOCKS" -eq 1 ] && [ -d "features" ] && [ -n "$FEATURE_STANDARD" ] \ +if [ "$CHECK_BLOCKS" -eq 1 ] && [ -d "features" ] \ && [ -f "$DIR/utils/extract-feature-php.php" ] then TEMP_DIR=$(mktemp -d 2>/dev/null || mktemp -d -t 'feature_phpcs') @@ -93,8 +89,7 @@ then # `--basepath` reduces the reported paths to the part that is worth # showing, which also keeps PHPCS from truncating them from the left # once they grow past the width of the report. - # shellcheck disable=SC2086 # Intentional word splitting. - vendor/bin/phpcs --standard="$FEATURE_STANDARD" $FEATURE_ARGS \ + vendor/bin/phpcs --standard="$FEATURE_STANDARD" \ --basepath="$TEMP_DIR" "$TEMP_DIR" >"$PHPCS_OUTPUT" 2>&1 || EXIT_CODE=$? # Findings are reported against the feature files they came from. diff --git a/phpcs/feature-files.sh b/phpcs/feature-files.sh deleted file mode 100644 index 0d8cba7e..00000000 --- a/phpcs/feature-files.sh +++ /dev/null @@ -1,57 +0,0 @@ -# Defaults for the code style check of the PHP blocks embedded in Behat feature -# files, shared by `run-phpcs-tests` and `run-phpcbf-cleanup`. -# -# Keeping the list in one place is what makes the check and the fixer agree: a -# sniff excluded for one but not the other would have the fixer rewrite feature -# files over something the check never reports, or have the check report -# something the fixer refuses to touch. -# -# The exclusions are passed on the command line rather than declared in a -# ruleset because a ruleset aborts the whole run over a sniff that the installed -# PHP_CodeSniffer does not know, while `--exclude` passes over it. The list -# spans several major versions of PHP_CodeSniffer and of the standards it -# builds on, and not every entry exists in all of them. -# -# A package replaces these defaults wholesale by adding a -# `phpcs-feature-files.xml` (or `phpcs-feature-files.xml.dist`) ruleset to its -# root, which is then used as the standard instead. - -WP_CLI_TESTS_FEATURE_STANDARD="WP_CLI_CS" - -# Warnings are advisory, and the fixer must not rewrite a feature file over -# something the check does not report. -WP_CLI_TESTS_FEATURE_ARGS="--warning-severity=0" - -# A block is not a file. It is padded with one empty line per preceding line of -# the feature file so that reported line numbers match it, and one that does not -# bring its own opening tag is given one. Neither is part of the snippet, and -# the shared docstring indentation is taken off before the check and put back -# afterwards, so none of the sniffs looking at a file as a whole apply. -WP_CLI_TESTS_FEATURE_EXCLUDES="Generic.Files.InlineHTML" -WP_CLI_TESTS_FEATURE_EXCLUDES="$WP_CLI_TESTS_FEATURE_EXCLUDES,Generic.PHP.CharacterBeforePHPOpenTag" -WP_CLI_TESTS_FEATURE_EXCLUDES="$WP_CLI_TESTS_FEATURE_EXCLUDES,Generic.Files.LineEndings" -WP_CLI_TESTS_FEATURE_EXCLUDES="$WP_CLI_TESTS_FEATURE_EXCLUDES,PSR2.Files.EndFileNewline" -WP_CLI_TESTS_FEATURE_EXCLUDES="$WP_CLI_TESTS_FEATURE_EXCLUDES,PSR12.Files.FileHeader" -WP_CLI_TESTS_FEATURE_EXCLUDES="$WP_CLI_TESTS_FEATURE_EXCLUDES,Squiz.Commenting.FileComment" -WP_CLI_TESTS_FEATURE_EXCLUDES="$WP_CLI_TESTS_FEATURE_EXCLUDES,Generic.PHP.RequireStrictTypes" -WP_CLI_TESTS_FEATURE_EXCLUDES="$WP_CLI_TESTS_FEATURE_EXCLUDES,WordPress.Files.FileName" -WP_CLI_TESTS_FEATURE_EXCLUDES="$WP_CLI_TESTS_FEATURE_EXCLUDES,Universal.WhiteSpace.PrecisionAlignment" -WP_CLI_TESTS_FEATURE_EXCLUDES="$WP_CLI_TESTS_FEATURE_EXCLUDES,Squiz.WhiteSpace.SuperfluousWhitespace" - -# A block is a fixture, not production code. Snippets exist to set up a -# scenario, run inside a throwaway WordPress installation, are written to be -# read at a glance, and are routinely a single class or function on their own. -WP_CLI_TESTS_FEATURE_EXCLUDES="$WP_CLI_TESTS_FEATURE_EXCLUDES,WordPress.NamingConventions.PrefixAllGlobals" -WP_CLI_TESTS_FEATURE_EXCLUDES="$WP_CLI_TESTS_FEATURE_EXCLUDES,WordPress.WP.GlobalVariablesOverride" -WP_CLI_TESTS_FEATURE_EXCLUDES="$WP_CLI_TESTS_FEATURE_EXCLUDES,WordPress.PHP.YodaConditions" -WP_CLI_TESTS_FEATURE_EXCLUDES="$WP_CLI_TESTS_FEATURE_EXCLUDES,Universal.Files.SeparateFunctionsFromOO" -WP_CLI_TESTS_FEATURE_EXCLUDES="$WP_CLI_TESTS_FEATURE_EXCLUDES,Generic.Files.OneObjectStructurePerFile" -WP_CLI_TESTS_FEATURE_EXCLUDES="$WP_CLI_TESTS_FEATURE_EXCLUDES,Universal.Namespaces.OneDeclarationPerFile" -WP_CLI_TESTS_FEATURE_EXCLUDES="$WP_CLI_TESTS_FEATURE_EXCLUDES,Universal.Namespaces.DisallowCurlyBraceSyntax" -WP_CLI_TESTS_FEATURE_EXCLUDES="$WP_CLI_TESTS_FEATURE_EXCLUDES,Universal.Namespaces.DisallowDeclarationWithoutName" -WP_CLI_TESTS_FEATURE_EXCLUDES="$WP_CLI_TESTS_FEATURE_EXCLUDES,PSR2.Methods.FunctionClosingBrace" - -# A snippet testing error handling is deliberately incomplete. -WP_CLI_TESTS_FEATURE_EXCLUDES="$WP_CLI_TESTS_FEATURE_EXCLUDES,Generic.CodeAnalysis.EmptyStatement" - -WP_CLI_TESTS_FEATURE_ARGS="$WP_CLI_TESTS_FEATURE_ARGS --exclude=$WP_CLI_TESTS_FEATURE_EXCLUDES" diff --git a/tests/tests/TestFeatureFilesRuleset.php b/tests/tests/TestFeatureFilesRuleset.php new file mode 100644 index 00000000..4b33dd37 --- /dev/null +++ b/tests/tests/TestFeatureFilesRuleset.php @@ -0,0 +1,116 @@ +temp_file = Utils\get_temp_dir() . uniqid( 'wp-cli-test-feature-ruleset-', true ) . '.php'; + } + + protected function tear_down(): void { + if ( file_exists( $this->temp_file ) ) { + unlink( $this->temp_file ); + } + + parent::tear_down(); + } + + /** + * Runs PHPCBF over a block and returns what it made of it. + * + * @param string $block Contents of the block, as extraction would write them. + * @return string Contents of the block after fixing. + */ + private function fix( $block ): string { + file_put_contents( $this->temp_file, $block ); + + $phpcbf = dirname( dirname( __DIR__ ) ) . '/vendor/squizlabs/php_codesniffer/bin/phpcbf'; + + $command = escapeshellarg( PHP_BINARY ) + . ' ' . escapeshellarg( $phpcbf ) + . ' ' . escapeshellarg( '--standard=WP_CLI_CS_Feature_Files' ) + . ' ' . escapeshellarg( '--sniffs=' . self::SNIFFS ) + . ' ' . escapeshellarg( $this->temp_file ) + . ' 2>&1'; + + $output = array(); + $exit_code = 0; + + exec( $command, $output, $exit_code ); + + // PHPCBF reports 1 when it fixed something and 0 when there was nothing + // to fix. Anything above that is the run itself having gone wrong. + $this->assertLessThanOrEqual( 1, $exit_code, implode( "\n", $output ) ); + + return (string) file_get_contents( $this->temp_file ); + } + + public function test_a_block_is_fixed_to_space_indentation(): void { + $this->assertSame( + "\n\n\nfix( "\n\n\nassertSame( $block, $this->fix( $block ) ); + } + + public function test_an_array_is_fixed_to_space_indentation(): void { + // Array indentation is a sniff of its own, with a tab setting of its own. + $this->assertSame( + "\n\n\n 1,\n);\n", + $this->fix( "\n\n\n 1,\n);\n" ) + ); + } + + public function test_trailing_whitespace_is_removed(): void { + // The fixer leaves whitespace behind wherever it breaks a line, so the + // sniff that cleans that up has to be part of the run. + $this->assertSame( + "\n\n\nfix( "\n\n\nassertSame( $block, $this->fix( $block ) ); + } +}