Skip to content

make lint reports "No changes found." when phpcs-changed is not installed #23

Description

@donnchawp

Symptom

make lint prints No changes found. on a branch that definitely has changed PHP files. It looks like a clean run. Nothing was linted.

Why

The cs script in composer.json is:

temp=$(git diff --diff-filter=d --name-only trunk HEAD | grep '.php'); [[ -n $temp ]] && phpcs-changed -s --always-exit-zero --git --git-base trunk $temp || echo 'No changes found.'

phpcs-changed is not in require-dev and is not installed by make install. In the Jetpack monorepo it comes from the repo root, so working there it is always on PATH and this never shows up. Outside that context the binary is missing, the command exits 127, the || branch fires, and you get the same message as a genuinely clean run.

So the failure mode is indistinguishable from success. The only clue is a phpcs-changed: command not found line above it, which is easy to miss and disappears entirely if stderr is not being watched.

cs-staged has the same shape and the same problem.

Second issue in the same line

--always-exit-zero means that even with phpcs-changed installed, lint failures cannot fail the command. Combined with the above, the lint gate currently cannot fail for any reason.

Suggested fix

Three separate small things:

  1. Add squizlabs/php_codesniffer and the WordPress coding standards to require-dev so make install genuinely installs everything needed to lint.
  2. Separate the two conditions so a missing binary is loud:
temp=$(git diff --diff-filter=d --name-only trunk HEAD | grep '\.php$')
if [[ -z $temp ]]; then echo 'No changes found.'; exit 0; fi
command -v phpcs-changed >/dev/null || { echo 'phpcs-changed is not installed.' >&2; exit 1; }
phpcs-changed -s --git --git-base trunk $temp
  1. Drop --always-exit-zero, or keep it only where a non-blocking report is actually wanted and make that explicit.

The || echo idiom is the root of it. Any script shaped that way turns a missing dependency into a success message, so it is worth fixing the same way anywhere else it appears.

There is already a working model for this

wp-super-cache solves exactly this and is worth copying rather than reinventing:

  • sirbrillig/phpcs-changed and automattic/jetpack-codesniffer are both in require-dev, so composer install provides them.
  • composer lint runs ./tools/run-phpcs-changed.sh, which uses set -euo pipefail, invokes the vendored vendor/bin/phpcs-changed by path rather than off PATH, and propagates the exit code instead of swallowing it.
  • It handles staged, unstaged and untracked files separately, and returns 0 only when there was genuinely nothing to check.

jetpack and jetpack-sirt also call phpcs-changed -s --git with no || echo and no --always-exit-zero, and both have the codesniffer in require-dev.

So this repo is the outlier among the four, not the pattern. Porting the wp-super-cache wrapper across would fix it in one go.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions