Merge branch 'recopy-check-docs' into shipping-only

ci/check-docs.php

@@ -31,52 +31,6 @@
  * 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?
  *
@@ -114,34 +68,27 @@ function hook_prefix() {
  * 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 $call One of 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_' );
+	// 🔴 From the conf, never a literal. Hardcoding 'nlsec_' here is exactly
+	// the copied-kit trap this family has paid for twice: the check keeps
+	// working in the tree it was written in and silently classifies every hook
+	// in the next one as "not ours", so CONTRACT.md may be empty and the run
+	// is green. I wrote it as a literal first and caught it re-reading.
+	$prefix = hook_prefix();
+
+	$ours   = 0 === strpos( $hook, $prefix );
+	$family = false;
+
+	foreach ( sibling_prefixes() as $other ) {
+		if ( 0 === strpos( $hook, $other ) ) {
+			$family = true;
+			break;
+		}
+	}
 
 	if ( ! $ours && ! $family ) {
 		return false;
@@ -156,14 +103,147 @@ function is_contract( $call, $hook ) {
 	return $family;
 }
 
+/**
+ * The shipped-file floor, from MIN_SHIPPED_PHP_FILES in ci/plugin.conf.
+ *
+ * Refuses rather than defaulting: an unset floor parses to 0 and every
+ * comparison against it passes, which is a scan over zero files one level up.
+ *
+ * @return int
+ */
+function shipped_php_floor() {
+	global $conf_root;
+
+	$conf = $conf_root . '/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, from ci/plugin.conf.
+ *
+ * Refuses rather than guessing. TEXT_DOMAIN is not it — here that is
+ * nolimit-security while the hooks are nlsec_ — and a checker that guessed
+ * would guess wrong in this tree and silently right in another, which is the
+ * worse of the two.
+ *
+ * @return string
+ */
+function hook_prefix() {
+	static $prefix = null;
+
+	if ( null !== $prefix ) {
+		return $prefix;
+	}
+
+	global $conf_root;
+
+	$conf = $conf_root . '/ci/plugin.conf';
+
+	if ( is_file( $conf ) && preg_match( '/^HOOK_PREFIX=([A-Za-z0-9_]+)/m', (string) file_get_contents( $conf ), $m ) ) {
+		$prefix = strtolower( $m[1] );
+
+		return $prefix;
+	}
+
+	fwrite( STDERR, "::error::No HOOK_PREFIX in ci/plugin.conf — refusing to guess which hooks are this plugin's.\n" );
+	exit( 1 );
+}
+
+/**
+ * Prefixes owned by the family, ours excluded.
+ *
+ * 🔴 Without this the check has a hole on the CONSUMER side, found by Nolimit
+ * Forms by deleting two rows and watching it stay green. The first version
+ * tested `ours || wpmgr_`, which is right in the tree that wrote it — `nlsec_`
+ * IS its prefix — and wrong everywhere else: in Forms' tree `nlsec_` is
+ * neither theirs nor `wpmgr_`, so the whole Forms/Security boundary fell into
+ * "core, not ours to document" and a green was printed over its absence.
+ *
+ * That is the class `wpmgr_` was widened to catch in the first place: a name a
+ * SIBLING owns that we answer. `wpmgr_` was one instance of it, not the rule.
+ *
+ * ⚠️ This list goes stale the day a ninth plugin appears — the same fault as
+ * the hardcoded prefix, one level out, and it is worth saying rather than
+ * leaving to be found. It belongs in FAMILY.md once that exists, read from one
+ * place by all eight. Until then it is one line per repo, visible, in the file
+ * that already holds everything plugin-specific.
+ *
+ * Named "sibling" rather than "family" because PHPCS's compatibility sniff
+ * reads a function starting `fam` as the removed PHP 5.1 `fam_*` extension and
+ * fails the build on it — a false positive, but one that costs the next person
+ * the same ten minutes.
+ *
+ * @return array
+ */
+function sibling_prefixes() {
+	static $list = null;
+
+	if ( null !== $list ) {
+		return $list;
+	}
+
+	global $conf_root;
+
+	$conf = $conf_root . '/ci/plugin.conf';
+	$raw  = is_file( $conf ) ? (string) file_get_contents( $conf ) : '';
+
+	/*
+	 * 🔴 Refuse when the line is absent, rather than defaulting to wpmgr_.
+	 *
+	 * The default used to BE array( 'wpmgr_' ) — which is exactly the narrow
+	 * behaviour this key exists to remove. So a tree that copied the kit and
+	 * forgot the line got the consumer-side hole straight back, silently, with
+	 * a plausible number and a green, while HOOK_PREFIX three lines away
+	 * refused. Nolimit Forms named it: **a default that is the pre-fix
+	 * behaviour is a fix that switches itself off.**
+	 *
+	 * `FAMILY_PREFIXES=""` is how a plugin says it answers no sibling hooks.
+	 * Same argument as HOOK_PREFIX_MIN=0: "none" and "nobody filled this in"
+	 * are otherwise identical silences, and only one of the two is a decision.
+	 *
+	 * Quotes are stripped because ci/plugin.conf is SOURCED by every ci/*.sh,
+	 * so a bare space-separated value makes the shell try to run the second
+	 * word. The site test found that, reporting
+	 * `ci/plugin.conf: line 89: nlfrm_: command not found` — a message naming
+	 * neither this file nor the setting.
+	 */
+	if ( ! preg_match( '/^FAMILY_PREFIXES=(.*)$/m', $raw, $m ) ) {
+		fwrite( STDERR, "::error::No FAMILY_PREFIXES in ci/plugin.conf. Without it a hook a SIBLING owns falls into \"core, not ours to document\" and this check goes green over the absence of the whole boundary. List the other plugins' prefixes, or set FAMILY_PREFIXES=\"\" to say this plugin answers none.\n" );
+		exit( 1 );
+	}
+
+	$list = preg_split( '/[\s,]+/', trim( $m[1], " \t\"'" ), -1, PREG_SPLIT_NO_EMPTY );
+
+	// Ours is handled separately; leaving it here too is harmless but makes the
+	// two arms of is_contract() overlap, which reads as a bug to the next person.
+	$list = array_values( array_diff( $list, array( hook_prefix() ) ) );
+
+	return $list;
+}
+
 $root = isset( $argv[1] ) ? rtrim( $argv[1], '/' ) : '.';
 $docs = $root . '/docs';
 
+global $conf_root;
+$conf_root = $root;
+
 $fails = 0;
 
+/**
+ * Record a problem and print it where CI will pick it up.
+ *
+ * @param string $message What is wrong, naming the file or hook.
+ * @return void
+ */
 function fail( $message ) {
 	global $fails;
-	$fails++;
+	++$fails;
 	printf( "::error::%s\n", $message );
 }
 
@@ -193,7 +273,7 @@ foreach ( $required as $file => $floor ) {
 		continue;
 	}
 
-	$lines = count( file( $path, FILE_IGNORE_NEW_LINES ) );
+	$lines        = count( file( $path, FILE_IGNORE_NEW_LINES ) );
 	$lines_total += $lines;
 
 	if ( $lines < $floor ) {
@@ -214,7 +294,7 @@ printf( "  %d documents, %d lines.\n", count( $required ), $lines_total );
  * 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' );
+$mains   = glob( $root . '/*.php' );
 $version = '';
 
 foreach ( (array) $mains as $main ) {
@@ -253,22 +333,21 @@ if ( 0 !== $rc ) {
 	exit( 1 );
 }
 
-$php = array_values( array_filter( $shipped, static function ( $f ) {
-	return '.php' === substr( $f, -4 );
-} ) );
+$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 ) {
+	// 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.
 	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 );
 }
@@ -341,9 +420,31 @@ foreach ( $php as $file ) {
 // floor gets lowered until the check reads nothing.
 $contract = (string) file_get_contents( $docs . '/CONTRACT.md' );
 
+/*
+ * 🔴 Table ROWS, not every backticked token in the file.
+ *
+ * Found by Nolimit Assistant from the other side and confirmed here by plant:
+ * delete a hook's table row, leave the name in one sentence of prose, and the
+ * first version printed "all 16 in docs/CONTRACT.md" and exited 0. A mention
+ * is not a row — the row carries the signature, what a caller gets when we are
+ * absent, and what a consumer must not do, and none of that is what a reader
+ * needs replaced by its own name in a sentence.
+ *
+ * It matters in the other direction too, which is how Assistant hit it: their
+ * first version read every backticked token and reported nine names as
+ * vanished hooks — two functions and seven WP_Error codes, every one of them
+ * correctly written in prose. Scoping to rows fixes both at once, and it makes
+ * the prose free: the tables are the contract, everything else is commentary,
+ * and commentary should not be able to satisfy or break a check.
+ *
+ * Verified against this document before narrowing: all 16 contract hooks are
+ * in rows, and the 30 backticked names that are not are field names, core
+ * hooks and the internal cron hooks listed under a heading that says they are
+ * explicitly not a contract.
+ */
 $documented = array();
 
-if ( preg_match_all( '/`([a-z0-9_]+)`/', $contract, $m ) ) {
+if ( preg_match_all( '/^\|[^|]*`([a-z0-9_]+)`/m', $contract, $m ) ) {
 	foreach ( $m[1] as $name ) {
 		$documented[ $name ] = true;
 	}
@@ -368,14 +469,63 @@ sort( $undocumented );
 // 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 = '';
+$prefix = hook_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] );
+/*
+ * 🔴 A wrong prefix is GREEN, and that is the whole reason this block exists.
+ *
+ * Measured on this tree by setting HOOK_PREFIX=zzzz_ : the run printed
+ * "6 hooks in 46 shipped PHP files, all 6 in docs/CONTRACT.md" and exited 0.
+ * Sixteen became six — only the wpmgr_ names survived — and nothing looked
+ * wrong, because six is a plausible number and every one of the six really was
+ * documented. Two wrongs cancelling into a pass, in a checker I had just told
+ * seven sibling sessions to copy into trees with seven different prefixes.
+ *
+ * Same shape as the copied kit that read zero always-loaded files: the port is
+ * not done when plugin.conf is, and the error you can see is never the one
+ * that matters. So: refuse. A plugin may genuinely define no hooks of its own,
+ * but that is a thing to state rather than to arrive at by a typo.
+ */
+$own = 0;
+
+foreach ( array_keys( $in_code ) as $hook ) {
+	if ( 0 === strpos( $hook, $prefix ) ) {
+		++$own;
+	}
 }
 
-if ( '' === $prefix ) {
-	fail( 'No HOOK_PREFIX in ci/plugin.conf — refusing to guess which documented names are ours.' );
+$min_own = 1;
+
+if ( is_file( $root . '/ci/plugin.conf' ) && preg_match( '/^HOOK_PREFIX_MIN=([0-9]+)/m', (string) file_get_contents( $root . '/ci/plugin.conf' ), $m ) ) {
+	$min_own = (int) $m[1];
+}
+
+if ( $own < $min_own ) {
+	// Nolimit Cache's improvement on the first version of this message, and it
+	// is the better one: say what IS there. "No hook carries nlsec_" leaves the
+	// reader to work out whether the scan found nothing at all or found the
+	// wrong things, and those need different fixes.
+	$present = array();
+
+	foreach ( array_keys( $in_code ) as $hook ) {
+		if ( preg_match( '/^([a-z]{2,8}_)/', $hook, $pm ) ) {
+			$present[ $pm[1] ] = isset( $present[ $pm[1] ] ) ? $present[ $pm[1] ] + 1 : 1;
+		}
+	}
+
+	$summary = array();
+
+	foreach ( $present as $seen => $count ) {
+		$summary[] = sprintf( '%s* (%d)', $seen, $count );
+	}
+
+	$message = sprintf(
+		'HOOK_PREFIX is %s* and no hook in the shipped source starts with it — the prefixes actually present are: %s. Either HOOK_PREFIX is wrong for this plugin, or this plugin defines none of its own; if it really defines none, say so in docs/CONTRACT.md and set HOOK_PREFIX_MIN=0 in ci/plugin.conf.',
+		$prefix,
+		$summary ? implode( ', ', $summary ) : 'none'
+	);
+
+	fail( $message );
 	exit( 1 );
 }
 
@@ -418,19 +568,23 @@ foreach ( array_keys( $documented ) as $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 )
-	) );
+	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 )
-	) );
+	fail(
+		sprintf(
+			'%d name(s) in docs/CONTRACT.md that no longer exist in the source: %s',
+			count( $stale ),
+			implode( ', ', $stale )
+		)
+	);
 }
 
 if ( ! $undocumented && ! $stale ) {

ci/dist-files.sh

@@ -25,7 +25,18 @@ done < .distignore
 # with nothing said. (-z also covers paths with spaces.) Found in a sibling
 # plugin that has exactly such a filename; this repo has none today, which is
 # precisely why it would have sat here until someone added one.
-git ls-files -z | while IFS= read -r -d '' file; do
+# In a git-less copy (ci_php's php container, which is handed exactly the
+# tracked files and has no git) the tree IS the tracked set, so walk it.
+# selftest.sh asserts the two forms print the same list.
+tracked() {
+  if [ -e .git ]; then
+    git ls-files -z
+  else
+    find . -type f -print0 | sed -z 's|^\./||' | LC_ALL=C sort -z
+  fi
+}
+
+tracked | while IFS= read -r -d '' file; do
   keep=1
   for p in "${patterns[@]}"; do
     # A bare name excludes the file or directory wherever it appears, which is

ci/plugin.conf

@@ -110,3 +110,6 @@ OLD_PLUGIN_NAME="WooCommerce Email Discount Manager"
 # run reports green having tested two thirds of what it claims.
 MIN_SITE_ASSERTIONS=60
 HOOK_PREFIX=wc_edm_
+# Hook prefixes owned by the sibling plugins. Quoted because this file is
+# sourced. Read from each sibling's HOOK_PREFIX, not guessed.
+FAMILY_PREFIXES="wpmgr_ nlfrm_ nlseo_ nlvw_ kws_ nlass_ nlcac_ nlsec_"

ci/selftest.sh

@@ -400,6 +400,19 @@ else
 fi
 cp "$work/helpers.orig" includes/class-helpers.php
 
+# 1b. the same fault for a hook a SIBLING owns and we answer. Before
+# FAMILY_PREFIXES this fell into "core, not ours to document" and stayed green.
+printf '\n// Added by ci/selftest.sh.\nadd_filter( %s, %s );\n' \
+  "'nlsec_selftest_sibling'" "'__return_true'" >> includes/class-helpers.php
+if [ "$(grep -c 'nlsec_selftest_sibling' includes/class-helpers.php)" -lt 1 ]; then
+  bad "could not stage the sibling-hook fixture"
+else
+  printf '%s' "$(docs_verdict)" | grep -q 'not in docs/CONTRACT.md: .*nlsec_selftest_sibling' \
+    && ok "flagged a sibling-owned hook we answer and CONTRACT.md does not mention" \
+    || bad "NOT FLAGGED: an undocumented sibling hook — the whole boundary to another plugin is invisible"
+fi
+cp "$work/helpers.orig" includes/class-helpers.php
+
 # 2. 🔴 the control that matters: the same name, in a COMMENT only. The checker
 # must stay GREEN, and this asserts the specific success line rather than an
 # exit code — a refusal, a missing file or a fatal all exit non-zero too, and
@@ -451,11 +464,29 @@ else
 fi
 git checkout -q -- docs/README.md 2>/dev/null || cp "$work/README.md.orig" docs/README.md
 
+# 5b. the file list must be the same without git. ci_php runs check-docs.php in
+# a php container that is handed the tracked files and has no git; dist-files.sh
+# used to exit 127 there, so the docs check failed on the Gitea runner alone.
+nogit=$(mktemp -d)
+git ls-files -z | tar --null -T - -cf - | tar -xf - -C "$nogit"
+if [ -e "$nogit/.git" ]; then
+  bad "could not stage the git-less copy"
+else
+  with_git=$(./ci/dist-files.sh)
+  without_git=$(cd "$nogit" && ./ci/dist-files.sh 2>&1 || echo "exit $?")
+  if [ -n "$with_git" ] && [ "$with_git" = "$without_git" ]; then
+    ok "dist-files.sh lists the same $(printf '%s\n' "$with_git" | wc -l | tr -d ' ') files with and without git"
+  else
+    bad "dist-files.sh differs without git — the docs check goes red on the Gitea runner only"
+  fi
+fi
+rm -rf "$nogit"
+
 # 6. both conf values must refuse rather than default. An unset floor parses to
 # 0 and every comparison against it passes, and a guessed prefix classifies
 # every hook in the tree as somebody else's.
 cp ci/plugin.conf "$work/conf.orig"
-for key in HOOK_PREFIX MIN_SHIPPED_PHP_FILES; do
+for key in HOOK_PREFIX MIN_SHIPPED_PHP_FILES FAMILY_PREFIXES; do
   grep -v "^$key=" "$work/conf.orig" > ci/plugin.conf
   if [ "$(grep -c "^$key=" ci/plugin.conf)" -ne 0 ]; then
     bad "could not stage the missing-$key fixture"