Add PHPCS linting for PHP blocks in feature files

.readme-partials/USING.md

@@ -78,6 +78,8 @@ To make use of the WP-CLI testing framework, you need to complete the following
     ```
 
     All other [PHPCS configuration options](https://github.com/PHPCSStandards/PHP_CodeSniffer/wiki/Annotated-Ruleset) are, of course, available.
+    The PHP snippets embedded in your feature files are checked along with the rest of the package. See [Checking the code style of the PHP blocks in feature files](#checking-the-code-style-of-the-php-blocks-in-feature-files) below.
+
 6. Optionally add a `phpstan-feature-files.neon.dist` file to the package root to also run PHPStan over the PHP snippets embedded in your feature files. See [Analysing the PHP blocks in feature files](#analysing-the-php-blocks-in-feature-files) below.
 
 7. Update your composer dependencies and regenerate your autoloader and binary folders:
@@ -165,6 +167,56 @@ Two kinds of blocks are left out of the analysis, and are listed at the end of t
 Blocks that declare the same class or function as another block are analysed separately from each
 other, so that PHPStan does not resolve a name to the wrong block's declaration.
 
+### Checking the code style of the PHP blocks in feature files
+
+`composer phpcs` also checks the PHP snippets that feature files embed in docstrings, and
+`composer phpcbf` fixes them in place. No configuration is needed, and like the analysis above the
+blocks are padded so that findings are reported against the feature file itself:
+
+```text
+FILE: features/command.feature
+----------------------------------------------------------------------
+FOUND 1 ERROR AFFECTING 1 LINE
+----------------------------------------------------------------------
+ 438 | ERROR | [x] Expected 1 space after IF keyword; 0 found
+----------------------------------------------------------------------
+```
+
+Only a docstring belonging to a step that creates a `.php` file is checked:
+
+```gherkin
+Given a wp-content/mu-plugins/test-harness.php file:
+  """
+  <?php
+  WP_CLI::add_command( 'test-harness', 'Test_Harness' );
+  """
+```
+
+Unlike the analysis above, a docstring that merely opens with `<?php` does not count. Those are
+routinely an expectation about the contents of a file rather than a file, and reformatting one would
+make it stop matching what it is checked against.
+
+The defaults leave out the sniffs that look at a block as if it were a file of its own, along with
+those that ask of a fixture what is only worth asking of production code. They live in
+`phpcs/feature-files.sh` and are shared by the check and the fixer, so that the two cannot disagree
+over which sniff applies. A package replaces them wholesale by adding a `phpcs-feature-files.xml`
+(or `phpcs-feature-files.xml.dist`) ruleset to its root:
+
+```xml
+<?xml version="1.0"?>
+<ruleset name="WP-CLI-PROJECT-NAME-feature-files">
+    <arg name="warning-severity" value="0"/>
+
+    <rule ref="WP_CLI_CS">
+        <exclude name="Generic.Files.InlineHTML"/>
+        <exclude name="Squiz.Commenting.FileComment"/>
+    </rule>
+</ruleset>
+```
+
+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.
+
 ### Controlling what to test
 
 To send one or more arguments to one of the test tools, prepend the argument(s) with a double dash. As an example, here's how to run the functional tests for a specific feature file only:

bin/run-phpcbf-cleanup

@@ -1,7 +1,82 @@
 #!/bin/sh
 
-# Run the code style check only if a configuration file exists.
+EXIT_CODE=0
+
+# 1. Run standard PHPCBF if configuration file exists.
 if [ -f ".phpcs.xml" ] || [ -f "phpcs.xml" ] || [ -f ".phpcs.xml.dist" ] || [ -f "phpcs.xml.dist" ]
 then
-	vendor/bin/phpcbf "$@"
+	vendor/bin/phpcbf "$@" || EXIT_CODE=$?
 fi
+
+# 2. Run PHPCBF over the PHP blocks in .feature files and sync back fixes.
+# Composer installs this script as a symlink in the vendor binary directory, so
+# it has to be resolved before the root of this package can be derived from it.
+SOURCE="$0"
+while [ -h "$SOURCE" ]
+do
+	SOURCE_DIR="$(cd -P "$(dirname "$SOURCE")" && pwd)"
+	SOURCE="$(readlink "$SOURCE")"
+	# A relative symlink is resolved against the directory holding the symlink.
+	case "$SOURCE" in
+		/*) ;;
+		*) SOURCE="$SOURCE_DIR/$SOURCE" ;;
+	esac
+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.
+FEATURE_STANDARD=""
+for CANDIDATE in "phpcs-feature-files.xml" "phpcs-feature-files.xml.dist"
+do
+	if [ -f "$CANDIDATE" ]
+	then
+		FEATURE_STANDARD="$(pwd)/$CANDIDATE"
+		break
+	fi
+done
+
+FEATURE_ARGS=""
+if [ -z "$FEATURE_STANDARD" ] && [ -f "$DIR/phpcs/feature-files.sh" ]
+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"
+fi
+
+# An argument naming what to fix applies to the files of the package itself, so
+# the blocks are left alone once a run has been narrowed down to a path.
+FIX_BLOCKS=1
+for ARG in "$@"
+do
+	case "$ARG" in
+		-*) ;;
+		*) FIX_BLOCKS=0 ;;
+	esac
+done
+
+if [ "$FIX_BLOCKS" -eq 1 ] && [ -d "features" ] && [ -n "$FEATURE_STANDARD" ] \
+	&& [ -f "$DIR/utils/extract-feature-php.php" ]
+then
+	TEMP_DIR=$(mktemp -d 2>/dev/null || mktemp -d -t 'feature_phpcbf')
+	trap 'rm -rf "$TEMP_DIR"' EXIT HUP INT TERM
+
+	# Fixes are only synced back when the extraction they are based on succeeded.
+	if php "$DIR/utils/extract-feature-php.php" extract features "$TEMP_DIR"
+	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 \
+				"$TEMP_DIR" >/dev/null || EXIT_CODE=$?
+
+			php "$DIR/utils/extract-feature-php.php" update features "$TEMP_DIR" >/dev/null || EXIT_CODE=$?
+		fi
+	else
+		EXIT_CODE=1
+	fi
+fi
+
+exit $EXIT_CODE

bin/run-phpcs-tests

@@ -1,7 +1,92 @@
 #!/bin/sh
 
-# Run the code style check only if a configuration file exists.
+EXIT_CODE=0
+
+# 1. Run standard PHP code style check if a configuration file exists.
 if [ -f ".phpcs.xml" ] || [ -f "phpcs.xml" ] || [ -f ".phpcs.xml.dist" ] || [ -f "phpcs.xml.dist" ]
 then
-	vendor/bin/phpcs "$@"
+	vendor/bin/phpcs "$@" || EXIT_CODE=$?
 fi
+
+# 2. Run PHPCS over the PHP blocks in .feature files.
+# Composer installs this script as a symlink in the vendor binary directory, so
+# it has to be resolved before the root of this package can be derived from it.
+SOURCE="$0"
+while [ -h "$SOURCE" ]
+do
+	SOURCE_DIR="$(cd -P "$(dirname "$SOURCE")" && pwd)"
+	SOURCE="$(readlink "$SOURCE")"
+	# A relative symlink is resolved against the directory holding the symlink.
+	case "$SOURCE" in
+		/*) ;;
+		*) SOURCE="$SOURCE_DIR/$SOURCE" ;;
+	esac
+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.
+FEATURE_STANDARD=""
+for CANDIDATE in "phpcs-feature-files.xml" "phpcs-feature-files.xml.dist"
+do
+	if [ -f "$CANDIDATE" ]
+	then
+		FEATURE_STANDARD="$(pwd)/$CANDIDATE"
+		break
+	fi
+done
+
+FEATURE_ARGS=""
+if [ -z "$FEATURE_STANDARD" ] && [ -f "$DIR/phpcs/feature-files.sh" ]
+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"
+fi
+
+# An argument naming what to check applies to the files of the package itself,
+# so the blocks are left alone once a run has been narrowed down to a path.
+CHECK_BLOCKS=1
+for ARG in "$@"
+do
+	case "$ARG" in
+		-*) ;;
+		*) CHECK_BLOCKS=0 ;;
+	esac
+done
+
+if [ "$CHECK_BLOCKS" -eq 1 ] && [ -d "features" ] && [ -n "$FEATURE_STANDARD" ] \
+	&& [ -f "$DIR/utils/extract-feature-php.php" ]
+then
+	TEMP_DIR=$(mktemp -d 2>/dev/null || mktemp -d -t 'feature_phpcs')
+	PHPCS_OUTPUT=$(mktemp 2>/dev/null || mktemp -t 'feature_phpcs_output')
+	trap 'rm -rf "$TEMP_DIR" "$PHPCS_OUTPUT"' EXIT HUP INT TERM
+
+	# Results are only reported when the extraction they are based on succeeded.
+	if php "$DIR/utils/extract-feature-php.php" extract features "$TEMP_DIR"
+	then
+		if [ -n "$(ls -A "$TEMP_DIR" 2>/dev/null)" ]
+		then
+			# The report is written to a file so that the status of PHPCS itself
+			# is preserved instead of the status of the command rewriting it.
+			# `--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 \
+				--basepath="$TEMP_DIR" "$TEMP_DIR" >"$PHPCS_OUTPUT" 2>&1 || EXIT_CODE=$?
+
+			# Findings are reported against the feature files they came from.
+			sed -E \
+				-e 's|^FILE: |FILE: features/|' \
+				-e 's/\.feature_L[0-9]+_E[0-9]+_(HASPHP|NOPHP)\.php/.feature/g' \
+				"$PHPCS_OUTPUT"
+		fi
+	else
+		EXIT_CODE=1
+	fi
+fi
+
+exit $EXIT_CODE

features/behat-steps.feature

@@ -587,7 +587,7 @@ Feature: Test that WP-CLI Behat steps work as expected
     And a send-email.php file:
       """
       <?php
-      wp_mail('test@example.com', 'Test', 'Body');
+      wp_mail( 'test@example.com', 'Test', 'Body' );
       """
     When I run `wp eval-file send-email.php`
     Then an email should be sent

features/testing.feature

@@ -18,7 +18,7 @@ Feature: Test that WP-CLI loads.
     And a test_cron.php file:
       """
       <?php
-      $cron_disabled = defined( "DISABLE_WP_CRON" ) ? DISABLE_WP_CRON : false;
+      $cron_disabled = defined( 'DISABLE_WP_CRON' ) ? DISABLE_WP_CRON : false;
       echo 'DISABLE_WP_CRON is: ' . ( $cron_disabled ? 'true' : 'false' );
       """
 

phpcs/feature-files.sh

@@ -0,0 +1,57 @@
+# 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"