Make phpcs actually scan the plugin, and remove an orphan class - #74
Merged
Conversation
Contributor
Test in WordPress PlaygroundTest the plugin with the code from this branch:
|
…alse-green The bare `<exclude-pattern>/exelearning/*</exclude-pattern>` matched the FULL path, so any checkout under a directory containing an `exelearning/` segment (e.g. .../ate/exelearning/wp-exelearning/, or the wp-env mount .../plugins/exelearning/) excluded the ENTIRE plugin — local `make phpcs` "passed" in 61ms by scanning nothing, while CI (checkout .../wp-exelearning/) scanned normally and failed. Anchor it to the intended target only, the WordPress uploads extraction dir. Verified from the real checkout path: `phpcs -q .` now scans the plugin (3.5s, was 61ms), is clean, a probe violation under includes/ is caught, and a probe under wp-content/uploads/exelearning/ stays excluded. No other ruleset behavior changed.
public/views/elp-list.php declared Exelearning_Admin, a leftover from the WordPress plugin generator. Nothing reaches it: the plugin has no autoloader (every class is pulled in by an explicit require_once from exelearning.php), no require_once names this path, and it was the only file under public/views/. Its strings, nonce and hooks are all dead with it. The coverage exclude in phpunit.xml.dist that carried it -- "requires non-existent file dependency" -- goes away with the file it was excluding. The catalogs under languages/ are regenerated because the file's strings and source references go with it: "Select eXeLearning File" and "Use this file" were unique to it, and "Settings" loses one of its three references. CI regenerates them and fails on a stale tree, so they belong in this commit.
WP_UnitTestCase keeps a single $wp_scripts for the whole process, so registrations and inline data accumulate across the tests in this file. Any assertion of the form "is this script on the page?" therefore passes as soon as some earlier test enqueued it, in file order, whether or not the code under test did anything -- the check stops being a check. set_up() now drops $GLOBALS['wp_scripts'] so each test starts from an empty registry and WordPress rebuilds it on first use.
The helper dropped $wp_scripts but not $wp_styles, and this file asserts against both -- wp_style_is() for exelearning-frontend, the block editor sheet and dashicons. The style registry therefore kept accumulating across tests exactly as the script registry used to. Demonstrated rather than assumed: a probe test that enqueues nothing, placed after test_enqueue_frontend_styles_enqueues_style, still saw exelearning-frontend as enqueued. With $wp_styles dropped it does not. Renamed to reset_assets() since it now covers both registries. Dropping $wp_styles is safe because WordPress rebuilds it lazily on next use and re-registers the defaults, dashicons included -- confirmed by the suite, not by reading the source.
erseco
force-pushed
the
fix/phpcs-exclude-pattern
branch
from
July 28, 2026 23:43
5e81f21 to
12da2d3
Compare
… it in CI Exclude patterns without a type attribute are matched against the absolute file path, so */wp-content/uploads/exelearning/* still excluded the entire plugin from a checkout living under a path like /srv/wp-content/uploads/exelearning/work/plugin/ - the same class of false green the pattern rewrite was meant to close, just from a rarer trigger. type="relative" matches against the path inside the scanned directory, which the checkout's own location can never satisfy. Verified both ways: a probe with violations under the repo's wp-content/uploads/exelearning/ stays excluded, and a copy of the tree nested under a wp-content/uploads/exelearning/ parent is scanned in full (34 files) where the absolute pattern scanned zero with exit 0. CI now re-runs phpcs on such a nested copy and fails if zero files are checked, so the regression cannot come back silently. The copy is made with git checkout-index because .gitattributes export-ignores dotfiles, including .phpcs.xml.dist itself.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
.phpcs.xml.distcarried a bare<exclude-pattern>/exelearning/*</exclude-pattern>.PHP_CodeSniffer matches exclude patterns against the full path, not a path
relative to the ruleset, so that pattern excluded the entire plugin from any
checkout living under a directory with an
exelearning/segment — which is mostof them:
.../ate/exelearning/wp-exelearning/, and the wp-env mountwp-content/plugins/exelearning/itself.The result was a silent false green. From such a checkout:
Zero files.
make lint"passed" by scanning nothing, while CI — whose checkoutpath is
.../wp-exelearning/, without the offending segment — scanned normally.Local linting has not been telling us anything for as long as the pattern has
been there.
What this changes
1. Make the exclude pattern relative-type. It now names only what it was
meant to name, the WordPress uploads extraction directory, and matches against
the path inside the scanned tree rather than the absolute file path:
Patterns without a
typeattribute are absolute-type, so an earlier revision ofthis fix (
*/wp-content/uploads/exelearning/*) still excluded the whole pluginfrom a checkout living under a path like
/srv/wp-content/uploads/exelearning/work/plugin/— rarer trigger, same falsegreen (caught in review). With
type="relative"the checkout's own location cannever satisfy the pattern.
From the same checkout, phpcs now scans the plugin (34 files, ~3 s) and is clean.
Verified with probes: a deliberate violation under
includes/is caught, and oneunder
wp-content/uploads/exelearning/stays excluded. No other ruleset behaviorchanged — the two
WordPress.Files.FileName.*sniffs stay excluded,/bin/*stays excluded.
CI now guards the regression: a new step re-runs phpcs on a copy of the tree
nested under a
wp-content/uploads/exelearning/parent and fails unless filesare actually checked. The absolute-type pattern scans zero files there with
exit 0, which is exactly the silent failure mode this PR exists to close. The
copy is made with
git checkout-indexbecause.gitattributesexport-ignoresdotfiles —
git archivewould drop.phpcs.xml.distitself.2. Remove
public/views/elp-list.php. It declaredExelearning_Admin, aleftover from the plugin generator.
composer.jsondoes declare PSR-4 mappings,but the production entry point never loads Composer's autoloader — every class is
pulled in by an explicit
require_oncefromexelearning.php— and the globalExelearning_Adminclass doesn't follow the PSR-4 mapping anyway, so no loadercould reach it. No
require_oncenames this path, and nothing instantiates theclass. It was also the only file under
public/views/, so the directory goeswith it. The stale coverage exclude
in
phpunit.xml.distthat pointed at it goes too, and the catalogs underlanguages/are regenerated:"Select eXeLearning File"and"Use this file"were unique to this file and
"Settings"loses one of its three sourcereferences.
3. Reset the script and style registries between block tests.
WP_UnitTestCasekeeps one$wp_scriptsand one$wp_stylesfor the wholeprocess, so registrations and inline data accumulate across the tests in
ElpUploadBlockTest. Any assertion of the form "is this script/style on thepage?" passes as soon as some earlier test enqueued it, in file order, whether
or not the code under test did anything. For styles this was demonstrated, not
assumed: a probe test that enqueues nothing, placed after
test_enqueue_frontend_styles_enqueues_style, still sawexelearning-frontendas enqueued.
set_up()now callsreset_assets(), which drops both$GLOBALS['wp_scripts']and$GLOBALS['wp_styles']so WordPress rebuilds themper test (defaults, dashicons included, re-register lazily on next use). No test
in this file relies on the leak today; this closes the door before one does.
Verification
vendor/bin/phpcs --standard=.phpcs.xml.dist -q .— exit 0, 34/34 files, clean(was: 0 files, 62 ms, exit 0)
wp-content/uploads/exelearning/parent scans 34 files with the relative-typepattern, where the absolute-type revision scanned 0 files with exit 0; a
probe file with deliberate violations under the repo's own
wp-content/uploads/exelearning/stays excluded.against an index that still held the absolute-type pattern it failed with
0 files checked, and passed with 34 once the fix was staged.
phpunitunder wp-env — OK (796 tests, 1706 assertions)make check-translations— "Translations are up to date and deterministic."run 30429900246