Commit 3de91f10394 for woocommerce
commit 3de91f1039401333f8a4a4b5521bc3665dd97c1b
Author: Raluca Stan <ralucastn@gmail.com>
Date: Fri Aug 21 12:38:59 2026 +0200
Add inline PHPCS annotations to failed lint checks (#66989)
* Add inline PHPCS annotations to failed lint checks
Right now a failed lint check only shows a red X; contributors have to dig
through the raw CI log to find the offending file and line. This installs
cs2pr and re-runs phpcs-changed with a checkstyle report when Lint fails,
so violations show up as inline comments on Files changed instead.
* Scope cs2pr annotations to fork PRs and fix annotation paths
Annotations are aimed at community contributors, who are least likely to have
our linters set up locally; members keep the plain Lint failure as before.
Also prefixes the checkstyle paths with plugins/woocommerce. phpcs reports them
relative to the step's working directory, but GitHub resolves annotation paths
from the repo root, so without this they never attach to the diff.
* Stop the annotation step from failing the job a second time
cs2pr exits non-zero whenever the report contains errors. The job has already
failed at Lint by that point, so all this added was a second red step and a
stray "exit code 1" annotation pointing at the workflow file.
* Remove fork gate from PHPCS inline annotation steps
* Fix word-splitting of changed PHP paths in annotation step
* Emit a checkstyle report from lint-branch.sh for cs2pr annotations
The annotation step re-computed the changed-file diff itself, with its own
hardcoded origin/trunk, to produce a checkstyle report for cs2pr -- a second
place to maintain the diff logic. Move the report emission into lint-branch.sh:
when WC_LINT_CHECKSTYLE_FILE is set and PHPCS fails, the script re-renders the
findings as checkstyle at that path (phpcs-changed emits one format per run, so
the readable report people dig into stays in the Lint log, and green runs never
pay for the extra render). ci.yml just points cs2pr at the file, prefacing it
with total finding counts since GitHub renders only the first ~10 annotations
of each severity per step.
The cs2pr steps are gated on the Lint step's own outcome (id/steps.lint.outcome,
as in Gutenberg's phpcs job) rather than job-level failure(), so they react only
to a lint failure, never to an unrelated step failing.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* Stop the checkstyle report from double-prefixing file paths
phpcs-changed reports the git path when it resolves a file through git, so the
paths already carry the 'plugins/woocommerce/' prefix; prepending it again
produced 'plugins/woocommerce/plugins/woocommerce/src/...', which GitHub cannot
match to any file in the diff. The annotations were silently dropped, with no
error and a green step.
The prefix is still needed for the case the original comment described:
phpcs-changed falls back to plain phpcs when it cannot resolve a file through
git, and then reports a path relative to this directory. Strip the prefix before
adding it so both forms land on the same correct path.
* Guard the checkstyle report on phpcs's own status, not the accumulator
* Gate the annotation steps on the checkstyle report existing
* Correct the comment about the paths phpcs-changed reports
* Set the checkstyle report path via GITHUB_ENV, not job-level env
* Rename the report variable to say only phpcs may write it
* Install cs2pr with the monorepo setup instead of a failure-path step
* Clarify why the report path is set via GITHUB_ENV
* Cache phpcs output in CI so the checkstyle render is nearly free
* Guard cs2pr against a non-XML report and reword stale comments
* Restore the word-splitting fix for changed PHP paths
* Fail the branch lint when the merge base can't be resolved
* Quote $baseBranch in phpcs-changed invocations
Defense-in-depth: $baseBranch comes from the script argument, so quoting
it prevents word splitting and globbing. Not attacker-reachable in CI
today (ci-jobs passes no --base-ref, so it falls to the origin/trunk
default), but the quotes make the intent explicit.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
diff --git a/.github/actions/setup-woocommerce-monorepo/action.yml b/.github/actions/setup-woocommerce-monorepo/action.yml
index cb42f780229..94ba095dd5d 100644
--- a/.github/actions/setup-woocommerce-monorepo/action.yml
+++ b/.github/actions/setup-woocommerce-monorepo/action.yml
@@ -4,6 +4,9 @@ inputs:
php-version:
description: 'The PHP version that should be installed. Use "false" to skip PHP installation.'
default: '7.4'
+ php-tools:
+ description: 'Given a comma-separated list, installs setup-php tools (e.g. "cs2pr"). Ignored when the PHP setup step is skipped (php-version "false" or build-type "dependencies").'
+ default: ''
install:
description: 'Given a boolean or PNPM filter, runs the install command for monorepo project(s).'
default: false
@@ -45,6 +48,7 @@ runs:
with:
php-version: '${{ inputs.php-version }}'
coverage: 'none'
+ tools: '${{ inputs.php-tools }}'
- name: 'Cache: identify pnpm caching directory'
if: ${{ inputs.pull-package-deps != 'false' }}
shell: 'bash'
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 39e2cc40896..931355623b5 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -175,10 +175,55 @@ jobs:
install: '${{ matrix.projectName }}...'
pull-package-deps: '${{ matrix.projectName }}'
php-version: ${{ ( ( matrix.projectName == '@woocommerce/plugin-woocommerce' && '7.4' ) || 'false' ) }}
+ # cs2pr turns the PHPCS checkstyle report into inline annotations in the
+ # 'Lint: PHP inline annotations' step; installed here alongside PHP (and
+ # only where PHP installs) rather than in a second setup-php invocation.
+ php-tools: 'cs2pr'
+
+ # On PHPCS failure, lint-branch.sh writes the findings to this path as a
+ # checkstyle report, so the annotation step after 'Lint' can surface them as
+ # inline annotations on 'Files changed' while the Lint log keeps the readable
+ # output. Set via GITHUB_ENV so the path is defined exactly once: the runner
+ # context isn't available in job-level env, and step-level env (where runner
+ # IS available) would have to repeat the path on every step that needs it.
+ # The file's presence is what triggers the annotation, so it must mean exactly
+ # "phpcs failed": if another linter ever emits a checkstyle report, it needs
+ # its own variable and file, never this one.
+ - name: 'Set the PHPCS report path'
+ run: echo "WC_PHPCS_CHECKSTYLE_FILE=$RUNNER_TEMP/phpcs-checkstyle.xml" >> "$GITHUB_ENV"
- name: 'Lint'
+ id: 'lint'
run: 'pnpm --filter="${{ matrix.projectName }}" ${{ matrix.command }}'
+ # Gate on the Lint step's own outcome (not job-level failure()) so this step
+ # reacts only to a lint failure, never to an unrelated step failing;
+ # !cancelled() is required because after a failure GitHub skips later steps
+ # unless told otherwise. The report only exists when PHPCS itself failed, so
+ # a JS-only failure (or another project's) takes the empty branch.
+ - name: 'Lint: PHP inline annotations'
+ if: ${{ !cancelled() && steps.lint.outcome == 'failure' }}
+ # cs2pr exits non-zero whenever the report contains errors; finish green
+ # regardless: the Lint step already set the verdict, and a second red step
+ # would be noise. The totals are stated first because GitHub renders roughly
+ # the first 10 error and 10 warning annotations per step; past that, findings
+ # exist only in this log, and a capped diff view shouldn't read as the full list.
+ # The XML check guards against the report holding composer chatter or a partial
+ # write instead of checkstyle: cs2pr would fail to parse it, and the || true
+ # would turn that into a silently empty step.
+ run: |
+ if [[ ! -s "$WC_PHPCS_CHECKSTYLE_FILE" ]]; then
+ echo 'No PHPCS checkstyle report to annotate.'
+ elif ! head -n1 "$WC_PHPCS_CHECKSTYLE_FILE" | grep -q '<?xml'; then
+ echo 'The PHPCS report exists but does not look like checkstyle XML; skipping annotations. First lines:'
+ head -n5 "$WC_PHPCS_CHECKSTYLE_FILE"
+ else
+ errors=$(grep -c 'severity="error"' "$WC_PHPCS_CHECKSTYLE_FILE" || true)
+ warnings=$(grep -c 'severity="warning"' "$WC_PHPCS_CHECKSTYLE_FILE" || true)
+ echo "PHPCS reported ${errors} error(s) and ${warnings} warning(s). GitHub may render only the first ~10 of each inline; the full readable report is in the Lint step's log."
+ cs2pr "$WC_PHPCS_CHECKSTYLE_FILE" || true
+ fi
+
woocommerce-plugin-build-artifact:
name: "Build WooCommerce plugin artifact${{ needs.project-jobs.outputs.shared-plugin-build-has-required-consumers != 'true' && ' (optional)' || '' }}"
runs-on: ${{ github.event.pull_request.user.login == 'woocommercebot' && fromJSON('{"group":"WooCommerce Release Checks"}') || 'ubuntu-latest' }}
diff --git a/plugins/woocommerce/.gitignore b/plugins/woocommerce/.gitignore
index 47c9eac6584..1fabac65a6e 100644
--- a/plugins/woocommerce/.gitignore
+++ b/plugins/woocommerce/.gitignore
@@ -34,3 +34,6 @@ i18n/languages/woocommerce.pot
.wp-env.override.json
.wp-env.test.override.json
.wp-env.e2e.override.json
+
+# phpcs-changed cache (written when lint-branch.sh runs with --cache; see WC_PHPCS_CHECKSTYLE_FILE)
+.phpcs-changed-cache
diff --git a/plugins/woocommerce/bin/lint-branch.sh b/plugins/woocommerce/bin/lint-branch.sh
index 13351e04da7..5ed09d7b9d9 100644
--- a/plugins/woocommerce/bin/lint-branch.sh
+++ b/plugins/woocommerce/bin/lint-branch.sh
@@ -7,13 +7,29 @@
#
# Example:
# ./lint-branch.sh base-branch
+#
+# When WC_PHPCS_CHECKSTYLE_FILE is set and PHPCS finds problems, the findings are also
+# written to that path as a checkstyle report, for a workflow step to feed to cs2pr,
+# which turns them into inline annotations on 'Files changed'. See the
+# 'Lint: PHP inline annotations' step in .github/workflows/ci.yml.
baseBranch=${1:-"origin/trunk"}
-changedFiles=$(git diff $(git merge-base HEAD $baseBranch) --relative --name-only --diff-filter=d -- '*.php')
+# Validated outside the process substitution below, where a failure (unfetched base
+# branch, shallow clone) would be swallowed and read as "no changed files": exit 0
+# with PHPCS never run.
+mergeBase=$(git merge-base HEAD "$baseBranch") || exit 1
+
+# -z plus the read loop keeps one path per argument: a filename with a space or a
+# glob character would otherwise split into several bogus arguments. (mapfile is
+# not an option; macOS ships bash 3.2, which doesn't have it.)
+changedFiles=()
+while IFS= read -r -d '' file; do
+ changedFiles+=("$file")
+done < <(git diff -z "$mergeBase" --relative --name-only --diff-filter=d -- '*.php')
# Only complete this if changed files are detected.
-if [[ -z $changedFiles ]]; then
+if [[ ${#changedFiles[@]} -eq 0 ]]; then
echo "No changed files detected."
exit 0
fi
@@ -22,7 +38,35 @@ fi
# so a failure in one check is never masked by a later one passing.
status=0
-composer exec phpcs-changed -- -s --git --git-base $baseBranch $changedFiles || status=1
+# Cache phpcs output only in CI, where the checkstyle render below re-runs the same
+# check: with the cache primed by the first run, the re-run is mostly cache reads.
+# Local runs stay cache-free, so no .phpcs-changed-cache file is left behind.
+cacheArgs=()
+[[ -n $WC_PHPCS_CHECKSTYLE_FILE ]] && cacheArgs=('--cache')
+
+# phpcs gets its own status besides the shared accumulator: the checkstyle report
+# below must be tied to phpcs itself failing, not to any other check that sets status.
+phpcsStatus=0
+composer exec phpcs-changed -- -s --git --git-base "$baseBranch" "${cacheArgs[@]}" "${changedFiles[@]}" || phpcsStatus=1
+status=$phpcsStatus
+
+# The readable report above is the log people dig into; this re-runs the same check
+# only to render the same findings as checkstyle for cs2pr (phpcs-changed can only
+# emit one format per run). Guarded on failure so green runs never pay for it.
+#
+# GitHub resolves annotation paths from the repository root, so every path must carry
+# this directory's prefix exactly once or the annotation silently never attaches to
+# the diff. phpcs-changed names findings two ways (verified in v2.12.0, incl. locally
+# with a planted violation): for modified files, getNewMessages() renames them to the
+# `git diff --no-prefix` header path, which is repository-rooted; for new files there
+# is no diff, so the cwd-relative CLI path survives. Stripping the prefix before
+# adding it normalizes both forms to a single prefix.
+if [[ -n $WC_PHPCS_CHECKSTYLE_FILE && $phpcsStatus -eq 1 ]]; then
+ prefix=$(git rev-parse --show-prefix)
+ composer exec phpcs-changed -- --git --git-base "$baseBranch" --report=checkstyle "${cacheArgs[@]}" "${changedFiles[@]}" |
+ sed -e "s|<file name=\"${prefix}|<file name=\"|g" \
+ -e "s|<file name=\"|<file name=\"${prefix}|g" > "$WC_PHPCS_CHECKSTYLE_FILE"
+fi
# Also verify that no new PHP functions are added.
php ./bin/check-new-functions.php HEAD "$baseBranch" || status=1
diff --git a/plugins/woocommerce/changelog/add-phpcs-inline-lint-annotations b/plugins/woocommerce/changelog/add-phpcs-inline-lint-annotations
new file mode 100644
index 00000000000..8393cc1ee0a
--- /dev/null
+++ b/plugins/woocommerce/changelog/add-phpcs-inline-lint-annotations
@@ -0,0 +1,4 @@
+Significance: patch
+Type: dev
+
+Surface PHPCS findings as inline annotations on a pull request's Files changed tab.