Document the plugin, and check that the documentation still describes it
Five files under docs/, the names the family agreed: README, SETUP,
ARCHITECTURE, CONTRACT, RELEASE. Plus ci/check-docs.php, ported from
woo-security-manager 52dd99d, wired into the static job.
CONTRACT.md is generated from the source rather than written from memory, and
that was the right instruction: my first sweep read only single-quoted hook
names and missed two rows, one of them update_plugins_{$hostname} — the hook
behind the live-shop collision, written with interpolation and therefore
invisible to a pattern built from the concept of the line. Nine hooks
documented: six wc_edm_* filters this plugin offers, three wpmgr_* it both
offers and answers through its copy of the frozen shared-menu.php.
Two hooks the checker structurally cannot see are listed by hand with a note
saying so: wc_edm_prune_audit_log (add_action on a class constant) and
update_plugins_{$hostname} (interpolated). A generated list beats a remembered
one and is still a list of what the generator can see.
The checker needed two fixes before it was safe to run here.
🔴 The published version reads HOOK_PREFIX from ci/plugin.conf for the
documented side, and refuses without it — then tests the source side against a
hardcoded 'nlsec_'. Ported verbatim that is not a green over nothing, which
would be easy to spot. Measured in this tree with CONTRACT.md documenting no
hook at all:
3 hook(s) in the shipped source and not in docs/CONTRACT.md:
wpmgr_dashboard_cards, wpmgr_menu_glyph, wpmgr_menu_title
Red, specific, and blind to all six wc_edm_* hooks — because 'wpmgr_' is the
same string in every tree and 'nlsec_' is the one that is not. Fix those three
and it goes green over six undocumented contract hooks, with a red run behind
you as evidence that it works. A partially blind check that fails is more
convincing than one that passes.
Note which step disarmed the guard. The floor at 20 shipped PHP files refused
to run in this 14-file tree, and that refusal was the only thing standing in
the way; the porting instructions were to add HOOK_PREFIX and adjust the floor.
Doing as told is what removed the protection — two wrongs cancelling into a
pass, in the order CLAUDE.md predicts, because the floor is the error you can
see and the prefix is not.
Both numbers now come from the conf (HOOK_PREFIX, MIN_SHIPPED_PHP_FILES) and
both refuse rather than defaulting. The inherited literal 20 is gone: a second
hardcoded number per plugin is a second thing to re-measure on every port.
Ten selftest assertions, 58 total. Four failure modes, the two refusals, the
counts on the pass, and the row that matters most:
🔴 A hook name in a COMMENT must not read as a hook call. CONTRACT.md documents
a filter by showing the apply_filters() a consumer makes, and the same example
sits in a docblock — so a checker that reads comments reports this plugin
consuming a filter it answers, in the file explaining why it must not. That
assertion is a control asserting a PASS, which is the kind that can quietly
become unable to fail, so it names the specific success line rather than
reading an exit code, and it is proven both ways: with token_get_all's
T_COMMENT branch made inert, exactly one of 58 assertions fails and it is that
one.
docs/TESTING-AND-RELEASES.md is folded into RELEASE.md and deleted rather than
left beside it — two copies of a rule is how the copies start to disagree.
Folding surfaced that it still documented the pre-rename Update URI
(kw/woocommerce-email-discount-manager) and still recommended, as future work,
the split into a releases-only repository that has already happened. Both
corrected in place, with the correction dated.
docs/ is not in ci/dist-files.sh — measured, 0 docs files in the zip.
static=0 selftest=0 site=0; site still 69 assertions on WordPress 7.1.2 with
WooCommerce 11.1.2. ci/ and docs/ ship nothing, so no version bump.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
kristianwind
co-authored by
Claude Opus 5
Claude Opus 5
parent
ffa7f5da3a
commit
dccb87d15e
@@ -0,0 +1,452 @@
|
||||
<?php
|
||||
/**
|
||||
* Assert that docs/ exists, is not a stub, and still describes THIS tree.
|
||||
*
|
||||
* 🔴 The third check is the only one that earns its place. "The file exists"
|
||||
* and "the version matches" are cheap and would both pass over documentation
|
||||
* that describes a plugin we stopped shipping a year ago. The set difference
|
||||
* between docs/CONTRACT.md and the source is the one that cannot.
|
||||
*
|
||||
* Two directions, because they fail differently:
|
||||
*
|
||||
* in the code, not in CONTRACT.md — a hook a sibling plugin cannot discover
|
||||
* without reading our source. That is the hole Nolimit Forms fell into
|
||||
* asking for a reputation filter that did not exist yet.
|
||||
*
|
||||
* in CONTRACT.md, not in the code — a comment describing a tree that is not
|
||||
* this one. Renaming a hook and leaving the row is silent, and the row
|
||||
* reads as deliberate.
|
||||
*
|
||||
* 🔴 It strips comments before looking at the source, and that is not a
|
||||
* nicety: docs/CONTRACT.md documents `nlsec_request_reputation` by showing
|
||||
* the `apply_filters()` call a consumer makes, and the same example is in
|
||||
* class-reputation.php's docblock. A checker that reads comments finds this
|
||||
* plugin applying a filter it answers — the self-consumption fault — in a file
|
||||
* that is documenting why it must not. The most careful comment in the tree,
|
||||
* reported as the bug it is warning about.
|
||||
*
|
||||
* Roles matter and are easy to get backwards: `add_filter` is how you ANSWER
|
||||
* a filter, not how you consume one.
|
||||
*
|
||||
* Usage: php ci/check-docs.php <plugin-root>
|
||||
*/
|
||||
|
||||
/**
|
||||
* MIN_SHIPPED_PHP_FILES from ci/plugin.conf. Refuses rather than defaulting:
|
||||
* an unset floor parses to 0 and every comparison against it passes, which is
|
||||
* the same bug as a scan over zero files one level up.
|
||||
*/
|
||||
function shipped_php_floor() {
|
||||
$conf = dirname( __DIR__ ) . '/ci/plugin.conf';
|
||||
|
||||
if ( is_file( $conf ) && preg_match( '/^MIN_SHIPPED_PHP_FILES=([0-9]+)/m', (string) file_get_contents( $conf ), $m ) && (int) $m[1] > 0 ) {
|
||||
return (int) $m[1];
|
||||
}
|
||||
|
||||
fwrite( STDERR, "::error::No usable MIN_SHIPPED_PHP_FILES in ci/plugin.conf — refusing to run with no floor.\n" );
|
||||
exit( 1 );
|
||||
}
|
||||
|
||||
/**
|
||||
* This plugin's own hook prefix, read from ci/plugin.conf.
|
||||
*
|
||||
* Refuses rather than guessing, and is deliberately NOT derived from
|
||||
* TEXT_DOMAIN: here the domain is wc-email-discount and the hooks are wc_edm_,
|
||||
* and in the tree this file came from the domain is nolimit-security and the
|
||||
* hooks are nlsec_. A checker that guessed would be wrong in both.
|
||||
*/
|
||||
function hook_prefix() {
|
||||
static $prefix = null;
|
||||
|
||||
if ( null !== $prefix ) {
|
||||
return $prefix;
|
||||
}
|
||||
|
||||
$conf = dirname( __DIR__ ) . '/ci/plugin.conf';
|
||||
$prefix = '';
|
||||
|
||||
if ( is_file( $conf ) && preg_match( '/^HOOK_PREFIX=([A-Za-z0-9_]+)/m', (string) file_get_contents( $conf ), $m ) ) {
|
||||
$prefix = strtolower( $m[1] );
|
||||
}
|
||||
|
||||
if ( '' === $prefix ) {
|
||||
fwrite( STDERR, "::error::No HOOK_PREFIX in ci/plugin.conf — refusing to guess which hooks are ours.\n" );
|
||||
exit( 1 );
|
||||
}
|
||||
|
||||
return $prefix;
|
||||
}
|
||||
|
||||
/**
|
||||
* Is this hook part of a contract, or is it internal wiring?
|
||||
*
|
||||
* 🔴 A contract is a name SOMEBODY ELSE has to know. That is the whole rule,
|
||||
* and getting it wrong in either direction ruins the check.
|
||||
*
|
||||
* The first version required every hook in the tree, which meant 50 core and
|
||||
* WooCommerce names — `admin_menu`, `init`, `wp_login`, `woocommerce_login_form`
|
||||
* — that we merely listen to. Listening to `init` is not a promise to anyone;
|
||||
* WordPress documents `init`. Requiring them would have made CONTRACT.md a
|
||||
* transcript of our own `add_action` calls, and the first person to read it
|
||||
* would stop, which is worse than not having the check.
|
||||
*
|
||||
* So the test is the PREFIX, because the prefix is exactly "is this a name the
|
||||
* family owns":
|
||||
*
|
||||
* do_action / apply_filters on nlsec_* or wpmgr_*
|
||||
* we are the source, or we are asking the family. Document it.
|
||||
*
|
||||
* add_filter / add_action on wpmgr_*
|
||||
* we are answering a name another plugin defined. It crosses a
|
||||
* repository boundary, which is the class of value nobody's checks cover.
|
||||
* Document it.
|
||||
*
|
||||
* add_filter / add_action on nlsec_*
|
||||
* our own wiring. If it is also offered, the first rule already caught
|
||||
* it; if it is not, nobody outside needs to know.
|
||||
*
|
||||
* anything else
|
||||
* core, WooCommerce, a third party. Not ours to document.
|
||||
*
|
||||
* Narrowing it this way is not loosening it. It found three real gaps on the
|
||||
* first run that the noisy version had buried under fifty core hooks:
|
||||
* `wpmgr_ai_usage` (a family-wide hook this plugin EMITS and had never told
|
||||
* anybody about), `nlsec_public_rest_routes` (a genuine extension point a form
|
||||
* plugin needs when a shop locks REST down) and `nlsec_pending_wp_config`.
|
||||
*
|
||||
* @param string $call do_action | apply_filters | add_filter | add_action
|
||||
* @param string $hook The hook name.
|
||||
* @return bool
|
||||
*/
|
||||
function is_contract( $call, $hook ) {
|
||||
// 🔴 From ci/plugin.conf, never a literal. The version of this file
|
||||
// published as woo-security-manager 52dd99d asked plugin.conf for the
|
||||
// prefix on the DOCUMENTED side (and refuses without it, correctly) and
|
||||
// still tested the SOURCE side against a hardcoded 'nlsec_'. Ported here
|
||||
// verbatim that is not a green-over-nothing — it is worse. Measured on this
|
||||
// tree with docs/CONTRACT.md documenting no hook at all:
|
||||
//
|
||||
// 3 hook(s) in the shipped source and not in docs/CONTRACT.md:
|
||||
// wpmgr_dashboard_cards, wpmgr_menu_glyph, wpmgr_menu_title
|
||||
//
|
||||
// Red, specific, and blind to all six wc_edm_* hooks — because 'wpmgr_' is
|
||||
// the same string in every tree and 'nlsec_' is the one that is not. Fix
|
||||
// those three and the check goes green over six undocumented contract
|
||||
// hooks, with a red run behind you as evidence that it works. A partially
|
||||
// blind check that fails is more convincing than one that passes.
|
||||
//
|
||||
// Note which step disarmed the guard: the floor at 20 shipped PHP files
|
||||
// refused to run in this 14-file tree, which is the only thing that stopped
|
||||
// it, and the porting instructions were to add HOOK_PREFIX and fix the
|
||||
// floor. Doing as told is what removed the protection. Two wrongs
|
||||
// cancelling into a pass, in the documented order.
|
||||
$ours = 0 === strpos( $hook, hook_prefix() );
|
||||
$family = 0 === strpos( $hook, 'wpmgr_' );
|
||||
|
||||
if ( ! $ours && ! $family ) {
|
||||
return false;
|
||||
}
|
||||
|
||||
if ( 'do_action' === $call || 'apply_filters' === $call ) {
|
||||
return true;
|
||||
}
|
||||
|
||||
// add_filter / add_action: only when the name is the family's, i.e. we are
|
||||
// answering somebody else.
|
||||
return $family;
|
||||
}
|
||||
|
||||
$root = isset( $argv[1] ) ? rtrim( $argv[1], '/' ) : '.';
|
||||
$docs = $root . '/docs';
|
||||
|
||||
$fails = 0;
|
||||
|
||||
function fail( $message ) {
|
||||
global $fails;
|
||||
$fails++;
|
||||
printf( "::error::%s\n", $message );
|
||||
}
|
||||
|
||||
/*
|
||||
* 1. The set exists and is not a stub.
|
||||
*
|
||||
* A floor, not a mere existence test: an empty file passes `is_file()`, and a
|
||||
* heading with nothing under it passes any check written from the concept of
|
||||
* "is it documented". The numbers are per-file because the documents have
|
||||
* genuinely different jobs — README is one screen on purpose.
|
||||
*/
|
||||
$required = array(
|
||||
'README.md' => 25,
|
||||
'SETUP.md' => 40,
|
||||
'ARCHITECTURE.md' => 60,
|
||||
'CONTRACT.md' => 50,
|
||||
'RELEASE.md' => 40,
|
||||
);
|
||||
|
||||
$lines_total = 0;
|
||||
|
||||
foreach ( $required as $file => $floor ) {
|
||||
$path = $docs . '/' . $file;
|
||||
|
||||
if ( ! is_file( $path ) ) {
|
||||
fail( sprintf( 'docs/%s is missing.', $file ) );
|
||||
continue;
|
||||
}
|
||||
|
||||
$lines = count( file( $path, FILE_IGNORE_NEW_LINES ) );
|
||||
$lines_total += $lines;
|
||||
|
||||
if ( $lines < $floor ) {
|
||||
fail( sprintf( 'docs/%s has %d lines, under the floor of %d — a stub is not documentation.', $file, $lines, $floor ) );
|
||||
}
|
||||
}
|
||||
|
||||
if ( $fails > 0 ) {
|
||||
printf( "\n%d problem(s) with the document set.\n", $fails );
|
||||
exit( 1 );
|
||||
}
|
||||
|
||||
printf( " %d documents, %d lines.\n", count( $required ), $lines_total );
|
||||
|
||||
/*
|
||||
* 2. The version in the docs is this plugin's version.
|
||||
*
|
||||
* Named value, not "a version is mentioned": a check that merely finds a
|
||||
* version-shaped string passes on last year's.
|
||||
*/
|
||||
$mains = glob( $root . '/*.php' );
|
||||
$version = '';
|
||||
|
||||
foreach ( (array) $mains as $main ) {
|
||||
if ( preg_match( '/^\s*\*\s*Version:\s*([0-9][^\s]*)/m', (string) file_get_contents( $main ), $m ) ) {
|
||||
$version = $m[1];
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
if ( '' === $version ) {
|
||||
fail( 'No Version: header found in the plugin main file — nothing to compare the docs against.' );
|
||||
} else {
|
||||
$readme = (string) file_get_contents( $docs . '/README.md' );
|
||||
|
||||
if ( false === strpos( $readme, $version ) ) {
|
||||
fail( sprintf( 'docs/README.md does not mention version %s.', $version ) );
|
||||
} else {
|
||||
printf( " docs/README.md names version %s, which is the header's.\n", $version );
|
||||
}
|
||||
}
|
||||
|
||||
/*
|
||||
* 3. CONTRACT.md against the source, both directions.
|
||||
*/
|
||||
$listing = $root . '/ci/dist-files.sh';
|
||||
|
||||
if ( ! is_file( $listing ) ) {
|
||||
fail( 'ci/dist-files.sh is missing — cannot tell which files ship.' );
|
||||
exit( 1 );
|
||||
}
|
||||
|
||||
exec( escapeshellcmd( $listing ) . ' 2>/dev/null', $shipped, $rc );
|
||||
|
||||
if ( 0 !== $rc ) {
|
||||
fail( 'ci/dist-files.sh failed — refusing to guess which files ship.' );
|
||||
exit( 1 );
|
||||
}
|
||||
|
||||
$php = array_values( array_filter( $shipped, static function ( $f ) {
|
||||
return '.php' === substr( $f, -4 );
|
||||
} ) );
|
||||
|
||||
// The floor comes from ci/plugin.conf, not from a literal. It arrived here as
|
||||
// 20 — measured in the tree this file was written in — and refused to run in
|
||||
// this 14-file one. A second hardcoded number per plugin is a second thing to
|
||||
// re-measure on every port, and the conf already carries this one.
|
||||
//
|
||||
// The floor is not the fault: a copied kit that reads zero files prints a
|
||||
// plausible number and passes. Refuse instead of reporting a green over
|
||||
// nothing — and refuse rather than warn, because a warning next to a plausible
|
||||
// number is read as a plausible number.
|
||||
$min_php = shipped_php_floor();
|
||||
|
||||
if ( count( $php ) < $min_php ) {
|
||||
fail( sprintf( 'Only %d shipped PHP files found, under the floor of %d. That is too few to be this tree — check the prefix and the paths before lowering this.', count( $php ), $min_php ) );
|
||||
exit( 1 );
|
||||
}
|
||||
|
||||
$in_code = array();
|
||||
$anywhere = array();
|
||||
|
||||
foreach ( $php as $file ) {
|
||||
$path = $root . '/' . $file;
|
||||
|
||||
if ( ! is_file( $path ) ) {
|
||||
continue;
|
||||
}
|
||||
|
||||
$source = (string) file_get_contents( $path );
|
||||
|
||||
// 🔴 Strip comments. See the header: our own docblocks contain the exact
|
||||
// calls this check looks for, and reading them turns the file that
|
||||
// explains a fault into the file that reports it.
|
||||
$code = '';
|
||||
|
||||
foreach ( token_get_all( $source ) as $token ) {
|
||||
if ( is_array( $token ) ) {
|
||||
if ( T_COMMENT === $token[0] || T_DOC_COMMENT === $token[0] ) {
|
||||
continue;
|
||||
}
|
||||
|
||||
$code .= $token[1];
|
||||
continue;
|
||||
}
|
||||
|
||||
$code .= $token;
|
||||
}
|
||||
|
||||
if ( preg_match_all( '/(do_action|apply_filters|add_filter|add_action)\(\s*\x27([a-z0-9_]+)\x27/', $code, $m, PREG_SET_ORDER ) ) {
|
||||
foreach ( $m as $hit ) {
|
||||
if ( is_contract( $hit[1], $hit[2] ) ) {
|
||||
$in_code[ $hit[2] ] = true;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/*
|
||||
* 🔴 A second, wider set, and the two directions need different ones.
|
||||
*
|
||||
* "Must be documented" is the narrow question — contract hooks only.
|
||||
* "Has this row gone stale" is a different one, and asking it against the
|
||||
* narrow set is wrong: it reported `nlsec_request_reputation` as no longer
|
||||
* existing, which is this plugin's most important contract. The only
|
||||
* occurrence in shipped code is `add_filter()` — we ANSWER it, and the
|
||||
* consumer applies it — so the contract classifier correctly declines to
|
||||
* demand it and the staleness check then read that as absence.
|
||||
*
|
||||
* Same for the five cron hooks, which the document names under a heading
|
||||
* that says they are explicitly NOT a contract.
|
||||
*
|
||||
* So: a documented name is stale only when it appears NOWHERE in the
|
||||
* shipped source. Every string literal, not just the ones in hook calls.
|
||||
*/
|
||||
if ( preg_match_all( '/\x27([a-z0-9_]+)\x27/', $code, $m ) ) {
|
||||
foreach ( $m[1] as $literal ) {
|
||||
$anywhere[ $literal ] = true;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// WordPress core hooks, and our own scheduled jobs, are named in the document
|
||||
// under headings of their own rather than in the tables. Listing them here is
|
||||
// what stops the check reporting them as undocumented forever — which is how a
|
||||
// floor gets lowered until the check reads nothing.
|
||||
$contract = (string) file_get_contents( $docs . '/CONTRACT.md' );
|
||||
|
||||
$documented = array();
|
||||
|
||||
if ( preg_match_all( '/`([a-z0-9_]+)`/', $contract, $m ) ) {
|
||||
foreach ( $m[1] as $name ) {
|
||||
$documented[ $name ] = true;
|
||||
}
|
||||
}
|
||||
|
||||
$core = array( 'plugins_api', 'rest_api_init' );
|
||||
|
||||
$undocumented = array();
|
||||
|
||||
foreach ( array_keys( $in_code ) as $hook ) {
|
||||
if ( ! isset( $documented[ $hook ] ) ) {
|
||||
$undocumented[] = $hook;
|
||||
}
|
||||
}
|
||||
|
||||
sort( $undocumented );
|
||||
|
||||
// The other direction is narrower on purpose: CONTRACT.md legitimately names
|
||||
// things that are not hooks (fields, codes, functions), so only names carrying
|
||||
// this plugin's own prefix are held to existing in the source.
|
||||
// The prefix comes from ci/plugin.conf, which every plugin in the family has,
|
||||
// rather than from a pattern over the hook names — a copied kit that guesses
|
||||
// the prefix from its own contents is how a check ends up reading zero files
|
||||
// in the next tree and printing a plausible number.
|
||||
$prefix = '';
|
||||
|
||||
if ( is_file( $root . '/ci/plugin.conf' ) && preg_match( '/^HOOK_PREFIX=([A-Za-z0-9_]+)/m', (string) file_get_contents( $root . '/ci/plugin.conf' ), $m ) ) {
|
||||
$prefix = strtolower( $m[1] );
|
||||
}
|
||||
|
||||
if ( '' === $prefix ) {
|
||||
fail( 'No HOOK_PREFIX in ci/plugin.conf — refusing to guess which documented names are ours.' );
|
||||
exit( 1 );
|
||||
}
|
||||
|
||||
$stale = array();
|
||||
|
||||
foreach ( array_keys( $documented ) as $name ) {
|
||||
// Long enough to be a hook rather than the bare prefix itself, which is
|
||||
// also a table-name literal in the source.
|
||||
if ( 0 !== strpos( $name, $prefix ) || strlen( $name ) <= strlen( $prefix ) + 3 ) {
|
||||
continue;
|
||||
}
|
||||
|
||||
if ( isset( $anywhere[ $name ] ) ) {
|
||||
continue;
|
||||
}
|
||||
|
||||
/*
|
||||
* 🔴 A dynamic hook is built by concatenation, so the source carries the
|
||||
* literal PREFIX and the document has to carry the full name a reader
|
||||
* needs — `update_option_{$option}`, `site_transient_{$transient}`,
|
||||
* `plugin_action_links_{$file}`. Compared literally, that row is correct
|
||||
* in both files and fails forever, and the fix somebody reaches for is
|
||||
* deleting the row rather than the rule. Found by Nolimit Cache, whose
|
||||
* run was 56/56 with exactly one such false positive.
|
||||
*/
|
||||
$dynamic = false;
|
||||
|
||||
foreach ( array_keys( $anywhere ) as $literal ) {
|
||||
if ( strlen( $literal ) > strlen( $prefix ) && 0 === strpos( $name, $literal ) ) {
|
||||
$dynamic = true;
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
if ( ! $dynamic ) {
|
||||
$stale[] = $name;
|
||||
}
|
||||
}
|
||||
|
||||
sort( $stale );
|
||||
|
||||
if ( $undocumented ) {
|
||||
fail( sprintf(
|
||||
'%d hook(s) in the shipped source and not in docs/CONTRACT.md: %s',
|
||||
count( $undocumented ),
|
||||
implode( ', ', $undocumented )
|
||||
) );
|
||||
}
|
||||
|
||||
if ( $stale ) {
|
||||
fail( sprintf(
|
||||
'%d name(s) in docs/CONTRACT.md that no longer exist in the source: %s',
|
||||
count( $stale ),
|
||||
implode( ', ', $stale )
|
||||
) );
|
||||
}
|
||||
|
||||
if ( ! $undocumented && ! $stale ) {
|
||||
printf(
|
||||
" %d hooks in %d shipped PHP files, all %d in docs/CONTRACT.md (prefix %s, %d core).\n",
|
||||
count( $in_code ),
|
||||
count( $php ),
|
||||
count( $in_code ),
|
||||
'' !== $prefix ? $prefix . '*' : '?',
|
||||
count( $core )
|
||||
);
|
||||
}
|
||||
|
||||
if ( $fails > 0 ) {
|
||||
printf( "\n%d documentation problem(s).\n", $fails );
|
||||
exit( 1 );
|
||||
}
|
||||
|
||||
exit( 0 );
|
||||
Reference in New Issue
Block a user