Skip to content

fix: use mapfile array instead of xargs to preserve real phpcs exit code#23

Merged
WayneRocha merged 1 commit into
mainfrom
fix/phpcs-xargs-exit-code
Jul 1, 2026
Merged

fix: use mapfile array instead of xargs to preserve real phpcs exit code#23
WayneRocha merged 1 commit into
mainfrom
fix/phpcs-xargs-exit-code

Conversation

@WayneRocha

@WayneRocha WayneRocha commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Problem

After #22 merged, phpcs CI still fails incorrectly when violations are found.

xargs translates phpcs exit code 1 (violations found) into exit code 123. GitHub Actions runs steps with bash -e by default, so when xargs returns 123 the step dies immediately — reviewdog never runs, no inline comments are posted, and the job fails with a cryptic exit code instead of the actual violations.

Practical impact:

  • PRs with real phpcs violations get a job failure with exit code 123 and no inline comments
  • Developers have no idea what they need to fix

Fix

Replace printf | xargs with mapfile to read CHANGED_FILES into a bash array. This:

  • Gives us phpcs's real exit code (0 = clean, 1 = violations) instead of xargs's 123
  • Uses || PHPCS_EXIT_CODE=$? to prevent bash -e from killing the step before reviewdog runs
  • Still correctly handles filenames with spaces (each array element is one file path)
# Before
JSON_REPORT=$(printf '%s\n' "$CHANGED_FILES" | xargs -d '\n' vendor/bin/phpcs --report=json -q)
PHPCS_EXIT_CODE=$?   # always 123 when violations found, never 1

# After
mapfile -t FILES <<< "$CHANGED_FILES"
PHPCS_EXIT_CODE=0
JSON_REPORT=$(vendor/bin/phpcs --report=json -q "${FILES[@]}") || PHPCS_EXIT_CODE=$?

Test PRs

All affected repos have test PRs pointing to fix/phpcs-xargs-exit-code (updated from #22 test PRs).

Examples

the-events-calendar/the-events-calendar#5676
the-events-calendar/tribe-common#2939
the-events-calendar/event-tickets#4280
More in #22

xargs translates phpcs exit code 1 (violations found) to exit code 123.
GitHub Actions runs steps with bash -e, so when xargs returns 123 the step
dies immediately - reviewdog never runs and no inline comments are posted.

Using mapfile to read CHANGED_FILES into a bash array gives us the real
phpcs exit code and || prevents bash -e from killing the step early.
@WayneRocha

Copy link
Copy Markdown
Contributor Author

Even the bot started working returning to this formmating

image

@WayneRocha WayneRocha requested review from dpanta94 and redscar July 1, 2026 13:33
@WayneRocha WayneRocha merged commit a9d3d57 into main Jul 1, 2026
@WayneRocha WayneRocha deleted the fix/phpcs-xargs-exit-code branch July 1, 2026 13:48
WayneRocha added a commit that referenced this pull request Jul 2, 2026
…de (#24)

## Problem

After #22 and #23, the phpcs job still fails on PRs that introduce **no
new violations**.

The `Run reviewdog` step ends with:

```bash
exit $PHPCS_EXIT_CODE
```

So the job's pass/fail is driven by **phpcs's** exit code. But phpcs
scans the whole changed file, so its exit code reflects **every** issue
in that file — including pre-existing ones on lines the PR never
touched. `reviewdog`, by contrast, runs with `-filter-mode=added` and
only reports/fails on issues introduced on the PR's **added lines**.

The two disagree, and the job exits on the phpcs code.

### Reproduction — the-events-calendar#5687 "clean" test (`bf090fc`)

From the job log:

```
mapfile -t FILES <<< "src/functions/views/provider.php"
##[error]Process completed with exit code 2.
```

- phpcs scanned `provider.php`, found pre-existing issues → exited
**2**.
- Those issues are **not** on the PR's added lines, so reviewdog
filtered them out → **no inline comment** ("No bot display").
- Then `exit $PHPCS_EXIT_CODE` ran `exit 2` → **job failed anyway**.

`#22`/`#23` focused on faithfully *preserving* the phpcs exit code —
which is exactly the value that should **not** gate the job.

## Fix

- Run phpcs with `--report=checkstyle` and pipe straight to reviewdog
(`-f=checkstyle`), dropping the `jq` transform.
- **Exit with reviewdog's exit code, not phpcs's.** `-filter-mode=added
-fail-level=any` means reviewdog exits `1` only when it reports a
violation on a line this PR changed.
- Keep a guard: if phpcs produces **no** checkstyle output *and* exits
non-zero, it genuinely failed to run (bad standard, parse error, …) →
fail the job with phpcs's code.

```bash
set +e
CHECKSTYLE_REPORT=$(vendor/bin/phpcs --report=checkstyle -q "${FILES[@]}")
PHPCS_EXIT_CODE=$?
set -e

if [ -z "$CHECKSTYLE_REPORT" ] && [ "$PHPCS_EXIT_CODE" -ne 0 ]; then
  echo "phpcs failed to run (exit code $PHPCS_EXIT_CODE)"
  exit "$PHPCS_EXIT_CODE"
fi

REVIEWDOG_EXIT_CODE=0
echo "$CHECKSTYLE_REPORT" \
  | reviewdog -f=checkstyle -name="phpcs" -filter-mode="added" -fail-level=any -reporter=github-pr-review \
  || REVIEWDOG_EXIT_CODE=$?

exit "$REVIEWDOG_EXIT_CODE"
```

## Expected behavior across the-events-calendar#5687 test commits

| Commit | Introduces new violations? | reviewdog comment | Job |
|---|---|---|---|
| Test 1 / Test 2 | yes | posted | **fails** ✅ |
| Test 3 (clean) | no (pre-existing only) | none | **passes** ✅ |

## Test PR

the-events-calendar#5687 repointed to `@fix/phpcs-reviewdog-exit-code`
to re-run all three scenarios.

Created by claude 🤖
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants