fix(templates): only record a seed run that actually seeded (#865) (#1026)

CertTemplateSeeder::maybe_seed() wrote the seed-version flag unconditionally,
including when the run created nothing. The whole path fails silently:
Utils::read_file_contents() returns '' for a file it cannot read, seed() then
continues past that definition without a word, and the flag short-circuits
every later run — so a first seed that read nothing left the pool permanently
empty and never retried.

That is not cosmetic. An empty pool is exactly the condition under which
AdminAssetsManager::discover_layout_templates() falls through to the deprecated
legacy html/ glob, so the fallback's stated exit condition ("removed once the
pool seeds on every install") could not be met while this hole existed.

The flag is now written only once pool_has_defaults() confirms the pool holds a
shipped default; otherwise the run leaves it alone and retries on the next
admin request, with an off-by-default Debug::log_admin breadcrumb.

The guard is deliberately narrow — "not empty", not "everything seeded". A
partial seed still populates the picker and keeps the fallback dormant, and
gating on completeness would re-run restore()'s meta writes on every admin
request for as long as one file stayed unreadable.

Also registers the html/ fallback in the CLAUDE.md shim inventory with its
exit condition: evidence-gated, not a versioned deprecation cycle, and split
across two releases so a repaired install is observed before losing the net.


Claude-Session: https://claude.ai/code/session_01D1wR59A8Z7d3QGYnm8q2KQ

Co-authored-by: Claude <noreply@anthropic.com>

Author: rpgmem

CHANGELOG.md

@@ -8,6 +8,7 @@ The format follows [Keep a Changelog] (https://keepachangelog.com/en/1.1.0/).
 ## [Unreleased]
 
 ### Fixed
+- **A failed first seed left the certificate-template pool permanently empty** (#865): `CertTemplateSeeder::maybe_seed()` recorded the seed version even when it created nothing — an unreadable seed file is skipped silently — and the flag then short-circuits every later run. The install kept an empty pool and the form editor's layout picker was served by the deprecated legacy `html/` fallback instead. The version is now recorded only once a default is actually in the pool, so a failed seed retries on the next admin request.
 - **The activity log was mostly untranslatable** (#1024): `get_action_label()` fell back to `ucwords()` for any action it did not know, silently rendering untranslated English — 49 of the 64 action keys in use, including every recruitment, privacy and migration event, across the log table, the summary and the CSV export. All 49 now have translated labels, with a guard so a new action cannot rely on the fallback again.
 - **Linking a submission to a user logged the wrong action** (#1024): the call passed the action name where the level belongs, so every link and unlink recorded the generic `submission` while `user_linked`/`user_unlinked` landed in the level slot and was discarded by the level validation. Existing rows keep a label; new ones record the real action.
 - **The update screen said the plugin was untested on your WordPress** (#1022): `GitHubUpdater` hard-coded the compatibility fields WordPress actually reads, so raising `Tested up to` in `readme.txt` (#984) left the shipped value at 7.0 and every 7.1 site was told a verified release was untested; `Requires at least` had drifted the same way (6.2 vs 6.4). The values are now read from the plugin header and `readme.txt` at update-check time, removing the copy that drifted.

CLAUDE.md

@@ -480,10 +480,23 @@ A full security audit confirmed these hold plugin-wide — keep them that way (t
 
 Inventory of the legacy compatibility shims that remain in the code by design (snapshot — re-confirm the location in code before removing; paths and lines change with every refactor, so the table cites files/methods, never line numbers). Removing them requires evidence that no production installation depends on them.
 
-_The two shims previously tracked here — the `ensure_legacy_caps_renamed()` v1 pre-6.2.0 cap-rename migration and the pre-4.6.15 orphan-cron cleanup (activator + deactivator/uninstall) — were removed in 6.18.0 (#809) as scheduled; the inventory is currently empty._
+_The two shims previously tracked here — the `ensure_legacy_caps_renamed()` v1 pre-6.2.0 cap-rename migration and the pre-4.6.15 orphan-cron cleanup (activator + deactivator/uninstall) — were removed in 6.18.0 (#809) as scheduled._
+
+| Shim | Location | Risk if removed | Why it stays |
+| --- | --- | --- | --- |
+| **Legacy `html/` layout fallback** (#865 phase-4) | `AdminAssetsManager::discover_layout_templates()` → `discover_layout_templates_legacy_glob()` (the `html/*.html` glob when the pool is empty), and `FormEditor`'s by-filename load of `FFC_PLUGIN_DIR . 'html/'` for the `filename` param those entries post. Plus the `html/default_certificate_{1,2,3}.html` files that `.distignore` still ships. | **Medium.** An install whose template pool is empty loses the form editor's layout picker entirely — no defaults to choose, and the by-filename load path that the picker's own entries depend on disappears with it. | The pool is not yet guaranteed non-empty on every install. Until 6.22.0 a first seed that read nothing recorded itself as applied and never retried, permanently emptying the pool (fixed by `CertTemplateSeeder::pool_has_defaults()`); installs that hit it before the fix are repaired by the retry, but only after they take the update. |
 
 When a new shim is added, log it here (Shim · Location · Risk if removed · Why it stays), and when a new feature makes one unsafe or inadequate, open a specific sub-issue + a breaking-change banner in the CHANGELOG.
 
+#### Exit condition for the `html/` fallback — evidence, not a deprecation cycle
+
+Retiring it is **evidence-gated** (the `cpf_rf_encrypted` shape below), *not* a versioned deprecation cycle. The cycle exists for surfaces whose consumers a code scan cannot see — a public method an external integration might call. Both sites here are internal render paths with no hook, no filter and no external caller, so nothing invisible can depend on them; what they depend on is **install state**, which is observable. Two conditions, and note they are genuinely different — conflating them is the mistake that nearly retired this shim early:
+
+1. **The pool seeds on every install** — the fallback's own written condition, and the one that was *not* met before 6.22.0. The seeder fix is what makes it hold; verify on a real install that the layout picker is served from the pool.
+2. **Settings → Migrations → `import_legacy_templates` reads 0 pending** — every file an admin dropped into `html/` has been imported. This one has read 0 in production for some time, but it measures *imports*, not seeding, and on its own says nothing about condition 1.
+
+Ship the seeder fix and the removal in **different releases** (6.22.0 → 6.23.0): an install with an empty pool must receive the repair, and be observed to have taken it, before losing the safety net. The removal is ⚠ breaking for anyone still relying on a file in `html/`, so it carries a CHANGELOG banner.
+
 #### Gathering the evidence to remove a **High**-risk shim
 
 Not every High shim is provable by data. Before proposing to build a diagnostic, check whether the evidence already exists — the resolved `cpf_rf_encrypted` case below is the exemplar:

includes/admin/class-ffc-cert-template-seeder.php

@@ -10,7 +10,10 @@
  * hidden/edited defaults) are never clobbered.
  *
  * Runs once per seed version, guarded by the `ffc_cert_templates_seeded_version`
- * option; the option is bumped when new shipped defaults are added.
+ * option; the option is bumped when new shipped defaults are added — but only
+ * once the run has actually left a default in the pool, so a seed that read
+ * nothing retries instead of recording itself as done (see
+ * {@see self::pool_has_defaults()}).
  *
  * @package FreeFormCertificate\Admin
  * @since   6.18.0
@@ -78,6 +81,10 @@ class CertTemplateSeeder {
 	/**
 	 * Run the seeder once per seed version.
 	 *
+	 * The flag is written only when the pool actually ends up holding a shipped
+	 * default — see {@see self::pool_has_defaults()} for why an unconditional
+	 * write is a trap.
+	 *
 	 * @return void
 	 */
 	public static function maybe_seed(): void {
@@ -99,9 +106,55 @@ public static function maybe_seed(): void {
 			self::seed();
 		}
 
+		if ( ! self::pool_has_defaults() ) {
+			\FreeFormCertificate\Core\Debug::log_admin(
+				'CertTemplateSeeder: seed run produced no default template; leaving the seed flag unwritten so the next admin request retries.',
+				array(
+					'seed_version' => self::SEED_VERSION,
+					'applied'      => $applied,
+					'seed_dir'     => FFC_PLUGIN_DIR . self::SEED_DIR,
+				)
+			);
+			return;
+		}
+
 		update_option( self::SEED_FLAG, self::SEED_VERSION );
 	}
 
+	/**
+	 * Whether the pool holds at least one shipped default after a seed run.
+	 *
+	 * This is the guard that keeps the seeder honest, and it exists because the
+	 * whole path fails silently: {@see \FreeFormCertificate\Core\Utils::read_file_contents()}
+	 * returns '' for a file it cannot read, {@see self::seed()} then `continue`s
+	 * past that definition without a word, and an unconditional
+	 * `update_option( SEED_FLAG, … )` would record the run as applied. On the
+	 * next request `maybe_seed()` returns early, so a first seed that read
+	 * nothing — a deploy that landed mid-rsync, a permissions problem, a
+	 * packaging slip that dropped the seed files — leaves the pool permanently
+	 * empty and never retries.
+	 *
+	 * That is not a cosmetic failure: an empty pool is exactly the condition
+	 * under which
+	 * {@see \FreeFormCertificate\Admin\AdminAssetsManager::discover_layout_templates()}
+	 * falls back to the deprecated legacy `html/` glob, so the fallback's stated
+	 * exit condition — "removed once the pool seeds on every install" — could
+	 * not be met while this hole existed (#865 phase-4).
+	 *
+	 * Deliberately narrow: it asks whether the pool is *empty*, not whether every
+	 * definition seeded. A partial seed still populates the picker and keeps the
+	 * fallback dormant, so recording it as applied is correct — and blocking the
+	 * flag on completeness would re-run restore()'s meta writes on every single
+	 * admin request for as long as one file stayed unreadable. Visibility is not
+	 * consulted either: hiding a default is the admin's choice, not a seed
+	 * failure.
+	 *
+	 * @return bool
+	 */
+	private static function pool_has_defaults(): bool {
+		return array() !== self::existing_default_map();
+	}
+
 	/**
 	 * Create any missing default templates (non-destructive). Also the target
 	 * of a future "Restore defaults" action.

tests/Unit/CertTemplateSeederTest.php

@@ -95,16 +95,27 @@ static function ( $key, $value ) use ( &$bumped ) {
 
 	public function test_maybe_seed_seeds_all_defaults_and_bumps_flag(): void {
 		Functions\when( 'get_option' )->justReturn( 0 );
-		Functions\when( 'get_posts' )->justReturn( array() ); // no existing defaults
-		Functions\when( 'get_post_meta' )->justReturn( '' );
+
+		$meta = array();
+		// Stateful pool: empty when seed() looks, populated by the time the
+		// post-run guard checks whether anything was actually created.
+		Functions\when( 'get_posts' )->alias(
+			static function () use ( &$meta ) {
+				return array_keys( $meta );
+			}
+		);
+		Functions\when( 'get_post_meta' )->alias(
+			static function ( $id ) use ( &$meta ) {
+				return $meta[ $id ][ CertTemplateCpt::META_DEFAULT_SLUG ] ?? '';
+			}
+		);
 
 		$inserted = 0;
 		Functions\when( 'wp_insert_post' )->alias(
 			static function () use ( &$inserted ) {
 				return 100 + ( ++$inserted );
 			}
 		);
-		$meta = array();
 		Functions\when( 'update_post_meta' )->alias(
 			static function ( $id, $key, $value ) use ( &$meta ) {
 				$meta[ $id ][ $key ] = $value;
@@ -219,6 +230,125 @@ static function () use ( &$inserted ) {
 		$this->assertSame( 5, $inserted, 'the five missing defaults are inserted' );
 	}
 
+	/**
+	 * The #865 phase-4 hole. `maybe_seed()` used to write the seed flag
+	 * unconditionally, so a first seed that created nothing still recorded
+	 * itself as applied — and because the flag short-circuits every later run,
+	 * the pool stayed empty forever. An empty pool is precisely what makes
+	 * `AdminAssetsManager::discover_layout_templates()` fall through to the
+	 * deprecated legacy `html/` glob, so the fallback could never meet its own
+	 * exit condition ("removed once the pool seeds on every install").
+	 *
+	 * Simulated here through a failing `wp_insert_post()`. The real-world cause
+	 * is usually the other one — `Utils::read_file_contents()` returning '' for
+	 * an unreadable seed file, which `seed()` silently `continue`s past — but
+	 * that path cannot be exercised from a unit test: it keys on
+	 * `FFC_PLUGIN_DIR`, a process-wide constant the suite defines once. Both
+	 * causes funnel into the same post-run check, which is what this pins.
+	 */
+	public function test_maybe_seed_does_not_mark_seeded_when_nothing_was_created(): void {
+		Functions\when( 'get_option' )->justReturn( 0 );
+		Functions\when( 'get_posts' )->justReturn( array() ); // pool stays empty
+		Functions\when( 'get_post_meta' )->justReturn( '' );
+		Functions\when( 'update_post_meta' )->justReturn( true );
+		// wp_insert_post( …, true ) returning WP_Error is the failure shape.
+		Functions\when( 'wp_insert_post' )->justReturn( new \WP_Error( 'db_insert_error', 'nope' ) );
+
+		$written = null;
+		Functions\when( 'update_option' )->alias(
+			static function ( $key, $value ) use ( &$written ) {
+				$written = array( $key, $value );
+				return true;
+			}
+		);
+
+		CertTemplateSeeder::maybe_seed();
+
+		$this->assertNull(
+			$written,
+			'the seed flag must stay unwritten so the next admin request retries'
+		);
+	}
+
+	/**
+	 * Same guard on the version-bump branch: an already-seeded install whose
+	 * defaults were all deleted must not have the new seed version recorded
+	 * when restore() fails to put any of them back.
+	 */
+	public function test_maybe_seed_does_not_bump_the_version_when_restore_creates_nothing(): void {
+		Functions\when( 'get_option' )->justReturn( 1 ); // seeded under an older version
+		Functions\when( 'get_posts' )->justReturn( array() ); // …but the pool is empty now
+		Functions\when( 'get_post_meta' )->justReturn( '' );
+		Functions\when( 'update_post_meta' )->justReturn( true );
+		Functions\when( 'wp_insert_post' )->justReturn( new \WP_Error( 'db_insert_error', 'nope' ) );
+
+		$written = null;
+		Functions\when( 'update_option' )->alias(
+			static function ( $key, $value ) use ( &$written ) {
+				$written = array( $key, $value );
+				return true;
+			}
+		);
+
+		CertTemplateSeeder::maybe_seed();
+
+		$this->assertNull(
+			$written,
+			'the seed flag must stay unwritten so the next admin request retries'
+		);
+	}
+
+	/**
+	 * The guard is deliberately narrow — "the pool is not empty", not "every
+	 * definition seeded". A partial seed still populates the layout picker and
+	 * keeps the legacy fallback dormant, so it counts as applied. Blocking the
+	 * flag on completeness instead would re-run the seeder on every admin
+	 * request for as long as a single seed file stayed unreadable.
+	 */
+	public function test_maybe_seed_marks_seeded_when_only_some_defaults_could_be_created(): void {
+		Functions\when( 'get_option' )->justReturn( 0 );
+
+		$meta = array();
+		Functions\when( 'get_posts' )->alias(
+			static function () use ( &$meta ) {
+				return array_keys( $meta );
+			}
+		);
+		Functions\when( 'get_post_meta' )->alias(
+			static function ( $id ) use ( &$meta ) {
+				return $meta[ $id ][ CertTemplateCpt::META_DEFAULT_SLUG ] ?? '';
+			}
+		);
+		Functions\when( 'update_post_meta' )->alias(
+			static function ( $id, $key, $value ) use ( &$meta ) {
+				$meta[ $id ][ $key ] = $value;
+				return true;
+			}
+		);
+
+		// Only the first insert succeeds; the other five fail.
+		$calls = 0;
+		Functions\when( 'wp_insert_post' )->alias(
+			static function () use ( &$calls ) {
+				++$calls;
+				return 1 === $calls ? 101 : new \WP_Error( 'db_insert_error', 'nope' );
+			}
+		);
+
+		$bumped = null;
+		Functions\when( 'update_option' )->alias(
+			static function ( $key, $value ) use ( &$bumped ) {
+				$bumped = array( $key, $value );
+				return true;
+			}
+		);
+
+		CertTemplateSeeder::maybe_seed();
+
+		$this->assertSame( 'ffc_cert_templates_seeded_version', $bumped[0] ?? null );
+		$this->assertSame( 5, $bumped[1] ?? null, 'a partial seed still counts as applied' );
+	}
+
 	public function test_seed_html_references_shipped_assets_not_html_folder(): void {
 		// #865 crit #7: default images moved to the versioned assets/ dir so
 		// html/ can eventually be retired; guard against a seed reference