Skip to content

Make phpcs actually scan the plugin, and remove an orphan class - #74

Merged
erseco merged 6 commits into
mainfrom
fix/phpcs-exclude-pattern
Jul 29, 2026
Merged

Make phpcs actually scan the plugin, and remove an orphan class#74
erseco merged 6 commits into
mainfrom
fix/phpcs-exclude-pattern

Conversation

@erseco

@erseco erseco commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Problem

.phpcs.xml.dist carried 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 most
of them: .../ate/exelearning/wp-exelearning/, and the wp-env mount
wp-content/plugins/exelearning/ itself.

The result was a silent false green. From such a checkout:

$ vendor/bin/phpcs -q . ; echo $?
0
Time: 62ms

Zero files. make lint "passed" by scanning nothing, while CI — whose checkout
path 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:

<exclude-pattern type="relative">^wp-content/uploads/exelearning/*</exclude-pattern>

Patterns without a type attribute are absolute-type, so an earlier revision of
this fix (*/wp-content/uploads/exelearning/*) still excluded the whole plugin
from a checkout living under a path like
/srv/wp-content/uploads/exelearning/work/plugin/ — rarer trigger, same false
green (caught in review). With type="relative" the checkout's own location can
never 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 one
under wp-content/uploads/exelearning/ stays excluded. No other ruleset behavior
changed — 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 files
are 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-index because .gitattributes export-ignores
dotfiles — git archive would drop .phpcs.xml.dist itself.

2. Remove public/views/elp-list.php. It declared Exelearning_Admin, a
leftover from the plugin generator. composer.json does declare PSR-4 mappings,
but the production entry point never loads Composer's autoloader — every class is
pulled in by an explicit require_once from exelearning.php — and the global
Exelearning_Admin class doesn't follow the PSR-4 mapping anyway, so no loader
could reach it. No require_once names this path, and nothing instantiates the
class. It was also the only file under public/views/, so the directory goes
with it. The stale coverage exclude
in phpunit.xml.dist that pointed at it goes too, and the catalogs under
languages/ are regenerated: "Select eXeLearning File" and "Use this file"
were unique to this file and "Settings" loses one of its three source
references.

3. Reset the script and style registries between block tests.
WP_UnitTestCase keeps one $wp_scripts and one $wp_styles for the whole
process, so registrations and inline data accumulate across the tests in
ElpUploadBlockTest. Any assertion of the form "is this script/style on the
page?" 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 saw exelearning-frontend
as enqueued. set_up() now calls reset_assets(), which drops both
$GLOBALS['wp_scripts'] and $GLOBALS['wp_styles'] so WordPress rebuilds them
per 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)
  • Nested-checkout scenario, both directions: a copy of the tree under a
    wp-content/uploads/exelearning/ parent scans 34 files with the relative-type
    pattern, 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.
  • The new CI regression guard caught the bug for real once already: run locally
    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.
  • phpunit under wp-env — OK (796 tests, 1706 assertions)
  • make check-translations — "Translations are up to date and deterministic."
  • Full CI green on the head commit, including the new guard step:
    run 30429900246

@github-actions

Copy link
Copy Markdown
Contributor

Test in WordPress Playground

Test the plugin with the code from this branch:

Preview in WordPress Playground

ℹ️ The eXeLearning editor is fetched from the shared release and unpacked into the plugin when the playground boots, so the first load may take a few extra seconds. ELP upload, shortcode, Gutenberg block and preview work normally.

erseco added 4 commits July 29, 2026 00:41
…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
erseco force-pushed the fix/phpcs-exclude-pattern branch from 5e81f21 to 12da2d3 Compare July 28, 2026 23:43
erseco and others added 2 commits July 29, 2026 07:42
… 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.
@erseco
erseco merged commit f073261 into main Jul 29, 2026
4 checks passed
@erseco
erseco deleted the fix/phpcs-exclude-pattern branch July 29, 2026 07:08
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.

1 participant