Check the PHP blocks in feature files as the space-indented code they are (#362)

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Claude <noreply@anthropic.com>

Author: swissspidy

.readme-partials/USING.md

@@ -199,24 +199,34 @@ Unlike the analysis above, a docstring that merely opens with `<?php` does not c
 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:
+Feature files indent with spaces, so the sniffs that would indent a block with tabs are turned around
+for this run: a block is checked, and fixed, as the space-indented code it is, and a tab that does
+find its way into one is reported and fixed like any other violation. Trailing whitespace is reported
+too, as the fixer leaves some behind wherever it breaks a line.
+
+All of this is the `WP_CLI_CS_Feature_Files` ruleset, which is `WP_CLI_CS` with the sniffs that
+indent turned around, and with the sniffs that look at a block as if it were a file of its own left
+out, along with those that ask of a fixture what is only worth asking of production code. The check
+and the fixer both use it, so that the two cannot disagree over which sniff applies to a block. A
+package replaces it 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">
+    <rule ref="WP_CLI_CS_Feature_Files">
         <exclude name="Generic.Files.InlineHTML"/>
         <exclude name="Squiz.Commenting.FileComment"/>
     </rule>
 </ruleset>
 ```
 
+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.
 

README.md

@@ -210,24 +210,34 @@ Unlike the analysis above, a docstring that merely opens with `<?php` does not c
 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:
+Feature files indent with spaces, so the sniffs that would indent a block with tabs are turned around
+for this run: a block is checked, and fixed, as the space-indented code it is, and a tab that does
+find its way into one is reported and fixed like any other violation. Trailing whitespace is reported
+too, as the fixer leaves some behind wherever it breaks a line.
+
+All of this is the `WP_CLI_CS_Feature_Files` ruleset, which is `WP_CLI_CS` with the sniffs that
+indent turned around, and with the sniffs that look at a block as if it were a file of its own left
+out, along with those that ask of a fixture what is only worth asking of production code. The check
+and the fixer both use it, so that the two cannot disagree over which sniff applies to a block. A
+package replaces it 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">
+    <rule ref="WP_CLI_CS_Feature_Files">
         <exclude name="Generic.Files.InlineHTML"/>
         <exclude name="Squiz.Commenting.FileComment"/>
     </rule>
 </ruleset>
 ```
 
+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.
 

WP_CLI_CS_Feature_Files/ruleset.xml

@@ -0,0 +1,93 @@
+<?xml version="1.0"?>
+<ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" name="WP_CLI_CS_Feature_Files" xsi:noNamespaceSchemaLocation="../vendor/squizlabs/php_codesniffer/phpcs.xsd">
+
+	<description>Coding standard for the PHP blocks embedded in Behat feature files</description>
+
+	<!--
+	The defaults for the code style check of the PHP blocks that Behat feature
+	files embed in docstrings, used by `run-phpcs-tests` and by
+	`run-phpcbf-cleanup`.
+
+	Keeping them in one place is what makes the check and the fixer agree: a
+	sniff left out 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.
+
+	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.
+	-->
+
+	<!-- Warnings are advisory, and the fixer must not rewrite a feature file
+		 over something the check does not report. -->
+	<arg name="warning-severity" value="0"/>
+
+	<rule ref="WP_CLI_CS">
+
+		<!--
+		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.
+		-->
+		<exclude name="Generic.Files.InlineHTML"/>
+		<exclude name="Generic.Files.LineEndings"/>
+		<exclude name="PSR2.Files.EndFileNewline"/>
+		<exclude name="PSR12.Files.FileHeader"/>
+		<exclude name="Squiz.Commenting.FileComment"/>
+		<exclude name="Generic.PHP.RequireStrictTypes"/>
+		<exclude name="WordPress.Files.FileName"/>
+		<exclude name="Universal.WhiteSpace.PrecisionAlignment"/>
+
+		<!--
+		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.
+		-->
+		<exclude name="WordPress.NamingConventions.PrefixAllGlobals"/>
+		<exclude name="WordPress.WP.GlobalVariablesOverride"/>
+		<exclude name="WordPress.PHP.YodaConditions"/>
+		<exclude name="Universal.Files.SeparateFunctionsFromOO"/>
+		<exclude name="Generic.Files.OneObjectStructurePerFile"/>
+		<exclude name="Universal.Namespaces.OneDeclarationPerFile"/>
+		<exclude name="Universal.Namespaces.DisallowCurlyBraceSyntax"/>
+		<exclude name="Universal.Namespaces.DisallowDeclarationWithoutName"/>
+		<exclude name="PSR2.Methods.FunctionClosingBrace"/>
+
+		<!-- A snippet testing error handling is deliberately incomplete. -->
+		<exclude name="Generic.CodeAnalysis.EmptyStatement"/>
+
+		<!-- A feature file indents with spaces. -->
+		<exclude name="Generic.WhiteSpace.DisallowSpaceIndent"/>
+	</rule>
+
+	<!-- The other way round, so that a block arriving with tabs is brought in line. -->
+	<rule ref="Generic.WhiteSpace.DisallowTabIndent"/>
+
+	<rule ref="Generic.WhiteSpace.ScopeIndent">
+		<properties>
+			<property name="tabIndent" value="false"/>
+		</properties>
+	</rule>
+
+	<rule ref="WordPress.Arrays.ArrayIndentation">
+		<properties>
+			<property name="tabIndent" value="false"/>
+		</properties>
+	</rule>
+
+	<!--
+	The fixer leaves whitespace behind wherever it breaks a line, so this sniff
+	has to run. Only the two message codes that look at the file as a whole are
+	in the way: taking the padding off a block would shift every line number
+	reported against the feature file it came from.
+	-->
+	<rule ref="Squiz.WhiteSpace.SuperfluousWhitespace">
+		<exclude name="Squiz.WhiteSpace.SuperfluousWhitespace.StartFile"/>
+		<exclude name="Squiz.WhiteSpace.SuperfluousWhitespace.EndFile"/>
+	</rule>
+
+</ruleset>

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=$?

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.