Conversation
The squash of #1019 landed on main as ae38302. Every shipped version heading carries its release commit reference; this adds 6.21.0's. Cut onto the post-release develop rather than before it: the sync is a hard reset to main, so a backfill landing first would have been discarded — the same order #1004 and 037001f followed after 6.20.1 and 6.20.0. Claude-Session: https://claude.ai/code/session_01D1wR59A8Z7d3QGYnm8q2KQ Co-authored-by: Claude <noreply@anthropic.com>
…1021) Two errors in the release section, both found by following it during the 6.21.0 release. The first: step 4 said to do the CHANGELOG short-SHA backfill "before tagging so the tagged tree carries it". That is not achievable and has never happened. The post-release sync is a hard reset of develop to main, so a backfill landing before it is silently discarded — which is why #1004 and 037001f both cut theirs onto the post-release develop instead. Checked against the artifact rather than the prose: v6.20.1's tagged tree reads `## [6.20.1] (2026-08-30)` with no suffix, and main only acquired that suffix one release later. The backfill therefore becomes its own step 6, after the sync, stating where it lands and why the order is forced. The consequence is named as structural rather than as a defect: the newest shipped heading is expected to lack its suffix until the next release carries it, so only a missing suffix on an *older* heading means the backfill was actually skipped. Step 2 no longer carries the instruction at all — it sat under "In that PR", which is the bump commit, and that placement is what made the wrong order look right. The second: step 5 said to rebase develop on main after a release, while the Sync section it points at says post-release must be a reset, since the squash already contains every develop commit. Step 5 now says reset and gives the reason, so the two agree. Docs only — no CHANGELOG entry, matching #1006 and #1017, which changed repo conventions without touching plugin behaviour. Claude-Session: https://claude.ai/code/session_01D1wR59A8Z7d3QGYnm8q2KQ Co-authored-by: Claude <noreply@anthropic.com>
…them (#1023) The update screen told every WordPress 7.1 site "Compatibility with WordPress 7.1: Not tested" for a release that had been verified against 7.1. GitHubUpdater hard-coded the three compatibility fields it puts into the update-transient object, as constants kept in sync with readme.txt and the plugin header by nothing but a docblock asking politely. Both WP values had drifted: WP_TESTED still read 7.0 after #984 raised readme.txt to 7.1, and WP_REQUIRES read 6.2 against an actual 6.4. The stale copy is the one that counts — list_plugin_updates() compares the transient's `tested` field against the running version and never reads readme.txt, because this plugin updates from GitHub rather than the WordPress.org directory. WP_REQUIRES was the quieter half: understating the floor offers the update to 6.2 and 6.3 sites that cannot run it. Rather than correcting the constants and leaving the third copy in place, the values are now read with get_file_data() from the files that own them, so the class of bug is gone rather than reset. Two files, not one, and that is the floor: both are parsed by tooling before PHP runs — WordPress reads the plugin header to decide whether to load the plugin at all, WordPress.org reads readme.txt — so neither can be derived from the other. "Tested up to" is not a plugin-header field in the first place; it exists only in readme.txt. Not memoised. A static cache would be process-global mutable state that every test has to reset, bought for two 8 KB reads that happen on an update check and not on ordinary requests. Three guards, because the runtime read moves the failure mode rather than removing it — a renamed or deleted header makes get_file_data() return an empty string silently, which is a worse symptom than a stale one: - GithubUpdaterTest asserts the built update object carries exactly what the two files declare. Its get_file_data() stub is aliased to a real parse of the real files, not a fixture, since a fixture could agree with a stale value. - GitHubUpdaterCompatTest pins the headers the read depends on: present, non-empty, shaped like a version, and agreeing where readme.txt and the header overlap. It also fails if a hard-coded constant comes back, and if .distignore ever excludes readme.txt — which would break the read only on installed sites, never in a checkout. - Both were checked against the mutations they claim to catch (renamed header, disagreeing files), not just against green. Full suite: 7445 tests, 21483 assertions. Closes #1022 Claude-Session: https://claude.ai/code/session_01D1wR59A8Z7d3QGYnm8q2KQ Co-authored-by: Claude <noreply@anthropic.com>
…link action (#1025) Two defects, one reported and one found while diagnosing it. The reported one: the activity log showed "Capabilities Granted" untranslated on a Portuguese install. get_action_label() falls back to ucwords() for any action it does not know, which renders untranslated English in every locale. The fallback is silent, so a missing label looks deliberate — nothing fails and nothing warns. It was not one string. The map carried 15 labels; the log writes 64 action keys. The other 49 all went through the fallback: every recruitment action, every privacy action, every migration, plus scheduling, reregistration, bulk operations, decrypt failures and operator-issued certificates. The same helper feeds three surfaces — the log table, the per-action summary, and the CSV export. All 49 now have labels. The found one: SubmissionHandler passed the action name where the level belongs. $action = $user_id ? 'user_linked' : 'user_unlinked'; ActivityLog::log( 'submission', $action, ... ); against log( string $action, string $level = LEVEL_INFO, ... ). So linking and unlinking a submission both recorded the generic `submission`, the real action name landed in the level slot and was discarded by the level validation, and category_for_action() fell through to its `submissions` default. Auditing could not tell a link from an unlink, and no label would have fixed that. The arguments are corrected; `submission` keeps a label marked legacy so rows already written stay readable, and nothing writes it any more. ActivityLogActionLabelsTest is the ratchet. It collects action keys from ActivityLog::log() call sites and from category_for_action()'s map, and fails when one has no label. Three things it was built around: - It is narrow about the receiver. A first, looser pass matched `::log(` on any class and swept in keys belonging to other loggers, which is how `submission` first surfaced as an "action" — a false positive that happened to lead to a real bug. - Its own sanity check caught a second trap: the page declares an unrelated `$labels` map in get_preflight_reason_label(), and slicing on the variable name alone silently mixed the two. The extraction anchors on the method. - It asserts labels are wrapped in __(), not merely present, since a bare string would otherwise read as "missing" for the wrong reason. Checked against the mutations it claims to catch — a deleted label and a label stripped of its __() call both fail it. The fallback stays: rows written by an older version can carry a key the code no longer has, and those must still render something. The guard is what stops a current action from relying on it. Full suite: 7448 tests, 21494 assertions. Closes #1024 Claude-Session: https://claude.ai/code/session_01D1wR59A8Z7d3QGYnm8q2KQ Co-authored-by: Claude <noreply@anthropic.com>
…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>
) (#1029) All 31 PHPStan suppressions in includes/ used the form @phpstan-ignore-next-line argument.type which does NOT filter by that identifier — the -next-line variant suppresses every error on the line and treats the trailing text as a comment. Each one therefore read as narrow and behaved as a blanket. Measured with PHPStan 2.1.17 against the pinned wordpress-stubs: converting all 31 to the filtering `@phpstan-ignore <id>` form changes nothing except surfacing one previously hidden error. That error was real. ReregistrationSubmissionReader::get_by_reregistration() returned $wpdb->get_results() straight through, but get_results() returns null on a failed query and the method declares `: array` — a TypeError waiting for the first failing query. Fixed with the idiom the same file already uses two methods below (is_array() guard + @var cast). Necessity was established per suppression rather than assumed: neutralising the 17 placeholder-docblock suppressions made all 17 literal-string errors appear, so none was dead. Three of them were removable a better way — the export WHERE-clause builder really does return a literal-string (every fragment is a literal carrying placeholders; values travel separately), so declaring that is honest, PHPStan verifies it, and the three suppressions downstream stop being needed. The remaining 14 are irreducible and now say what is reported and why the interpolated fragment is code-chosen. %i does not help any of them, contrary to the issue's hypothesis: every identifier already uses %i, and what breaks literal-string is clause-level interpolation (WHERE/ORDER BY/LIMIT), which %i cannot express. Adds tests/Unit/PhpstanSuppressionTest.php, which fails on a suppression that uses the broad form, names no identifier, or carries no reason. Claude-Session: https://claude.ai/code/session_01D1wR59A8Z7d3QGYnm8q2KQ Co-authored-by: Claude <noreply@anthropic.com>
…1031) Necessity was established per annotation rather than assumed. Each of the 925 phpcs:ignore annotations in includes/ was neutralised in place (rewritten to name a sniff that does not exist, so the line stays a valid directive and the comment syntax is unchanged) and PHPCS re-run; an annotation whose target line then reported none of the sniffs it names is doing nothing. Measured against the stricter of two configurations, so the result holds either way: 520 load-bearing, 59 over-broad, 334 dead, 12 inconclusive. The 333 dead ones that are real directives are removed here (the 334th is prose in a docblock that merely mentions phpcs:ignore, not a directive). Verified three ways rather than by the model that selected them: - PHPCS under the committed ruleset is clean, as before. - PHPCS with WordPress.DB.DirectDatabaseQuery and WordPress.Security.ValidatedSanitizedInput additionally enabled reports 292 violations, exactly as it did before the removal — so nothing was unmasked under the stricter configuration either. - The diff is token-identical to HEAD ignoring whitespace and comments: no code changed. Deleting a directive line between two assignments merged two alignment blocks and produced 24 MultipleStatementAlignment warnings; phpcbf was run with only that sniff enabled to re-align them, which is the whitespace in the diff. The audit also explains why so many were inert, and that part is a finding rather than a cleanup: the two sniffs above are not part of WordPress-Extra, which is what phpcs.xml.dist references, so they have never run in this repository. 520 annotations are written as if they do. Enabling them turns those 520 into real suppressions and surfaces 292 sites nobody has triaged. That decision is left open on #1028; this commit only removes what is inert under both answers. Claude-Session: https://claude.ai/code/session_01D1wR59A8Z7d3QGYnm8q2KQ Co-authored-by: Claude <noreply@anthropic.com>
…sites (#1028) (#1032) WordPress.Security.ValidatedSanitizedInput is not part of WordPress-Extra — it lives only in the full WordPress standard — so it had never run in this repository, while dozens of annotations across the codebase were written as if it did. This enables it on the existing ruleset and leaves the gate clean. All 50 sites it reports were read individually first, because this is the sniff where a real bug could have been hiding. None was: every one is the same false positive, where sanitisation happens in the next statement rather than the same expression. The forms found, each now stated at the site rather than assumed: - a shared sanitiser one call away (build_query_args(), sanitize_value()) - an (int) or floatval() cast, which sanitises - a strict '1' === comparison, where the raw value never survives - wp_kses() with the HtmlPolicy allowlist, for the two template-HTML saves that must not go through sanitize_text_field() - ColorValidator::normalize(), array_map( 'intval' ), RequestInput::is_truthy() - an HMAC-verified cookie payload - is_uploaded_file() on a tmp_name that PHP generates and the client never sends - RequestInput itself, which is the sanitiser the rest of the codebase calls Eleven sites already carried a NonceVerification suppression; those were merged into rather than duplicated, keeping the existing reason and appending the new one. Inserting a comment between two assignments split an alignment block in four files, so phpcbf was run there with only that sniff enabled. The value of turning it on is not the 50 — they were all fine. It is that the 51st unsanitised read now fails CI instead of passing unnoticed. Verified: the committed gate is clean across the whole project, not only includes/; the diff is token-identical to HEAD ignoring whitespace and comments; PHPStan unchanged at 192 (the same environment-artifact set). DirectDatabaseQuery, the other sniff the audit found inert, is deliberately not enabled here — its 129 sites need their own pass, and NoCaching in particular cannot be justified in bulk. Claude-Session: https://claude.ai/code/session_01D1wR59A8Z7d3QGYnm8q2KQ Co-authored-by: Claude <noreply@anthropic.com>
#1028) (#1033) WordPress.DB.DirectDatabaseQuery is the second of the two sniffs the suppression audit found had never run here — like ValidatedSanitizedInput it lives only in the full WordPress standard, not in WordPress-Extra. This enables it on the existing ruleset and leaves the gate clean across the whole project. The 129 sites were classified before being annotated, because a single stamped reason across all of them would repeat the habit #1028 exists to correct: 25 transaction control (START TRANSACTION / COMMIT / ROLLBACK) — touches no table, so there is nothing to cache or invalidate 47 writes to the plugin's own ffc_* tables, which WordPress has no API for 3 schema changes in activators and migrations, which dbDelta cannot emit 7 reads that ARE the cache-miss path — the hit is the cache_get() above 17 reads in migrations, privacy erasers/exporters and activators, which must read live state, so caching would be wrong rather than useless 7 uniqueness and create-probes, where a cached answer defeats the check 3 export cursors, where each page is read exactly once 21 repository reads, each given its own reason Of those 21, several are not caching questions at all once read: get_by_auth_code() and get_by_magic_token() are token lookups where a consumed or revoked token must never come from cache; expire_overdue() reads the rows it is about to mutate; search_user_by_cpf() is keyed on a per-request hash. Five more are filtered admin lists whose cache key would be as varied as the query. The remaining dozen are genuine candidates, named as such at the site and tracked in a follow-up rather than papered over with a reason that implies all is well. Two placement traps, both caught by the gates rather than by inspection: - Inserting the directive between a docblock and its statement retargets any @PHPStan-Ignore in that docblock at the comment. PHPStan went 192 -> 194. - Moving it above the docblock instead aims the phpcs directive at the docblock, since it also covers only the next line — 14 sites went unsuppressed. The trailing form on the statement line is the only placement that leaves both pointing at the code. Verified: the committed gate is clean over the whole project; the diff is token-identical to HEAD ignoring whitespace and comments; PHPStan back to 192, the same set. Claude-Session: https://claude.ai/code/session_01D1wR59A8Z7d3QGYnm8q2KQ Co-authored-by: Claude <noreply@anthropic.com>
…1030) (#1036) First slice of the vacuous-test baseline. Tests only — no product code changed. All 25 drove a guard and then asserted nothing: $_GET['_wpnonce'] = 'bad-nonce'; Functions\when('wp_verify_nonce')->justReturn(false); ReregistrationSubmissionActions::handle_approve(); $this->assertTrue(true); Delete the nonce check from handle_approve() and that passes — it approves the submission, redirects, and the test calls it a success. The same shape covered every guard in the file, plus the capability check on ReregistrationAdmin. Each now asserts the guarded effect did not happen. The handlers call the writer and then wp_safe_redirect(), and read get_current_user_id() only to pass to the writer, so neither call happening is the proof the guard held. ReregistrationAdmin::handle_actions() is put in a state where it demonstrably WOULD act — right page, a message queued — so add_settings_error() never being called means the capability check stopped it. handle_save/handle_delete assert the nonce is never verified and wp_die() never reached. The email handler says outright what its comment had only reasoned about: the campaign query must not run. Verified the way #1030 asks for rather than by re-running the guard: the protected check was deleted locally and the tests confirmed to fail. Removing the capability check fails with "Method add_settings_error() should be called"; removing the handle_save action check fails naming wp_verify_nonce; removing the approve nonce check fails on the writer being reached. The expectations are repeated inline rather than factored into a helper. AssertionCoverageTest reads each test method's own body, so a helper call is invisible to it — the first attempt did exactly that and the baseline only shrank by 6 instead of 25. Hiding an assertion from the guard that exists to find missing assertions trades one false negative for another. Baseline regenerated: 95 -> 70 entries. Claude-Session: https://claude.ai/code/session_01D1wR59A8Z7d3QGYnm8q2KQ Co-authored-by: Claude <noreply@anthropic.com>
… (#1037) Slice 2 of the vacuous-test baseline. Tests only — no product code changed. Same shape as slice 1: each drove a nonce, capability or parameter guard and then asserted nothing, so deleting the guard left them green. The handlers here share a layout — the statement immediately after the parameter guard is the destructive-capability check — which makes the assertion precise: a capability that is never consulted is proof the guard returned. Ten SettingsActionHandler tests now say that with shouldNotReceive( 'current_user_can_admin_or' ). Where a guard could only be reached by a request that would otherwise act, the request is set up to act: handle_cache_actions() is asked for warm_cache so the capability is the only thing that can stop it, and the two handle_submission_actions() tests queue a real trash action so the page and capability checks are what the assertion is about, with the submission handler asserted never to receive trash_submission(). Two were subtler than the rest, and both had the same fault. The geolocation enqueue tests stubbed wp_enqueue_script() to throw and then asserted nothing: the guarantee was real but lived 400 lines away in the stub, not in the test. They now collect handles and assert the list is empty, like the sibling test that checks the enqueue does happen. test_save_settings_calls_save_locations was worse — its comment claimed save_settings no longer owns main_geo_areas and then verified only that nothing threw; it now asserts an option was written and that it carries no main_geo_areas key. Verified by deletion, not by re-running the guard: removing the parameter check from handle_migration_execution() and the manage-certificates check from handle_submission_actions() each make their test fail. Baseline regenerated: 70 -> 51 entries. Claude-Session: https://claude.ai/code/session_01D1wR59A8Z7d3QGYnm8q2KQ Co-authored-by: Claude <noreply@anthropic.com>
…) (#1038) Slice 3 of the vacuous-test baseline. Tests only — no product code changed. Three shapes here, and two of them were not guard tests at all: 9 enqueue guards — wrong hook, wrong screen, wrong post type, wrong tab, non-singular, no post, no shortcode. Each called the method and asserted nothing, so the guard could be deleted and the test stayed green. They now collect what wp_enqueue_script/style receive and assert the list is empty. 4 CPT guards — autosave, revision, non-publish, wrong post type. Past those guards sync_calendar_data() and cleanup_calendar_data() look the calendar up by post id before writing or deleting, so the repository never being consulted is the proof the guard returned. Asserted with the same overload mock the file's own create/update/no-record tests already use. 3 registration tests — test_register_calendar_cpt_registers_post_type, test_add_submenu_pages_registers_menu, test_add_custom_metaboxes_registers _boxes. These are the ones worth calling out: each named a thing it did not check, so a method that registered nothing at all would have passed. They now collect the registered slugs and assert the registration happened. The loader test is the inverse of its own sibling: wiring the admin trio is what registers the appointments export source, so on the frontend it must not be in the SourceRegistry — which the admin test already asserts positively. Verified by deletion: removing the wp_is_post_revision() guard from sync_calendar_data() makes its test fail. Baseline regenerated: 51 -> 35 entries. Claude-Session: https://claude.ai/code/session_01D1wR59A8Z7d3QGYnm8q2KQ Co-authored-by: Claude <noreply@anthropic.com>
) Slice 4 of the vacuous-test baseline. Tests only — no product code changed. Three findings beyond the usual "ends in assertTrue( true )": A test that passed for the wrong reason. The three handle_actions_returns_early_without_permission tests set the capability to false and called the handler with nothing in $_POST — so the handler would have done nothing whether or not the guard existed. They now queue the page's real save action and assert the nonce is never verified, which is what every acting branch does immediately after matching the action. And my own first pass repeated that mistake: it queued 'save_audience' on all three pages, but the calendar handles 'save_schedule' and the environment 'save_environment'. Two of the three would have gone on passing for the wrong reason with better prose attached. Fixed to each page's real action. A test that never reached what it was named after. test_handle_global_holiday_actions_returns_early_without_permission: the capability check in that method sits INSIDE the add_global_holiday branch, after the nonce, so with no ffc_action in $_POST the method returned for an unrelated reason and the permission guard was never executed. It now queues the action and a valid nonce so the capability really is the last thing standing, and asserts nothing was persisted through update_option(). Two tests whose names stated facts they did not check. test_register_rest_routes_creates_controller ("we just verify it doesn't throw") and the three shows_feedback_message tests: a register_rest_routes() that registered nothing, or a handler that surfaced no notice, would have passed. They now collect and assert. Two mechanics worth recording. AudienceAdminPageTest could not alias-mock Capabilities — it is already autoloaded by the time that test runs — so the assertion goes through current_user_can(), which the helper delegates to. And handle_csv_import() reads its three capability tiers BEFORE any action branch, so a capability check proves nothing there; the nonce is the marker. AudienceSampleCsvSourceTest is the weakest of the twelve and says so: the file loads no Brain\Monkey, so no WordPress function is defined in it, and a gate of any kind would fatal. It asserts the return contract; the environment supplies the teeth. Verified by deletion: removing the ffc_manage_audiences guard from AudienceAdminAudience::handle_actions() makes its test fail. Baseline regenerated: 35 -> 23 entries. Claude-Session: https://claude.ai/code/session_01D1wR59A8Z7d3QGYnm8q2KQ Co-authored-by: Claude <noreply@anthropic.com>
Slices 5 and 6 together — the frontend group and the long tail. They are one PR because the tail is by definition unrelated files with one entry each, and splitting it would buy a CI cycle rather than a reviewer any clarity. Tests only; no product code changed. The routine half: nine enqueue/no-op guards now collect what they claim was not enqueued and assert the list is empty; three "registers X" tests assert the registration happened; two request handlers assert the query var was read exactly once, since past the guard they read it twice more. The part worth reading is where writing a real assertion proved me wrong. Csv::writer( $h )->close() does NOT close $h. My first assertion said it did. close() fcloses only a handle the writer owns, and Csv::writer() is handed an open one it does not — the caller keeps what the caller opened. The test now asserts that contract, which is the opposite of what I first wrote. Csv::reader_from_string()->all() returns positional rows, not header-keyed ones. I asserted header-keyed and it failed. The documented return is list<list<string>>. Both had sat behind assertTrue( true ) for their whole lives, which is exactly how a wrong belief survives: nothing ever contradicted it. Two entries did not get an assertion, for different reasons: VerificationHandlerTest::test_magic_token_rate_limited_returns_error is DELETED. Its own comment said the real coverage is in RateLimiterTest (51 tests, and the rate_limited path is there). The body constructed nothing and called nothing — a comment with an assertion attached, named after behaviour it did not exercise. Keeping it would preserve a false coverage signal. PluginActivationSmokeTest::test_foreign_key_migration_composes_when_version _stale STAYS in the baseline, now with the reason written at the site. Two observables were tried and neither separates the branches: the FK version stamp is not advanced in this environment, and $this->ddl records dbDelta() while the migration issues ALTER TABLE through $wpdb->query(). What it does establish is what its name says. Proving the ALTER statements belongs to the fresh-install job (#994), which has the database this does not. Verified by deletion: removing the WP_Post guard from Frontend::frontend_assets() makes its test fail. Baseline: 23 -> 1 entry, and that one is explained. Claude-Session: https://claude.ai/code/session_01D1wR59A8Z7d3QGYnm8q2KQ Co-authored-by: Claude <noreply@anthropic.com>
(#1034) (#1042) Comments only — the diff is token-identical to HEAD. #1034 listed twelve uncached repository reads as caching candidates and named UserService::get_user_statistics() "the strongest caching candidate found", on the grounds that it runs three COUNT queries on every user-dashboard render. Tracing the callers, that is wrong. get_user_statistics() is reached from exactly two places, both cold: PrivacyExporters via export_personal_data() (a GDPR export request) and UserCleanup via user_has_ffc_data() (the deleted_user short-circuit). No dashboard render reaches it. Caching it would buy nothing, and on the deletion path a stale "has data" answer would be actively wrong. The premise came from counting call sites rather than reading them, in an issue whose own text warned that "three queries per dashboard render is an argument, not a measurement". The warning was right; I did not apply it to my own claim. Two more dissolved the same way. get_by_reregistration_and_user() has three callers that are separate AJAX entry points reading it once each, and a fourth inside a loop keyed on a different campaign per iteration — a cache would never be hit. get_audience_ids(), get_audiences(), count_by_audience(), get_by_key() and count_by_reregistration() each have one or a handful of callers across distinct flows, one call per request. One survives, on a mechanical argument rather than a speculative one: CustomFieldReader::get_by_audience() is the base read behind six sibling accessors (_with_parents, _grouped, get_profile_fields, get_sensitive_fields, _keyed), so a single render can repeat the same query for the same audience. That repeat is what a cache would remove, and unlike the others it holds even without a persistent object-cache backend. Still not implemented here: it needs a measured render, and invalidation through CustomFieldWriter. Every one of the twelve sites now states its verdict instead of promising a follow-up. No site still says "tracked in the caching follow-up". Claude-Session: https://claude.ai/code/session_01D1wR59A8Z7d3QGYnm8q2KQ Co-authored-by: Claude <noreply@anthropic.com>
…every survivor justified (#1035) (#1043) Every annotation in the PHPCS scope was neutralised in place — rewritten to name a sniff that cannot fire, never by renaming the `phpcs:` token itself, which turns the directive into an ordinary comment — and the suite re-run. The violations that surfaced were mapped back to the annotations covering them, and that mapping decided each verdict. phpcs:ignore 773 -> 434, phpcs:disable 128 -> 123, without a reason 260 -> 0, naming a sniff that never fires 72 -> 0. 50 dead disable blocks and 15 duplicate ignore lines removed: the nonce blocks sat in handlers that already call check_admin_referer/check_ajax_referer earlier in the same function, and the template blocks named PrefixAllGlobals in partials that only read their aliased variables. 375 DirectDatabaseQuery annotations collapsed into 49 file-level disables, chosen by scanning every $wpdb table reference and keeping only files where all of them are ffc_*; the 13 that also touch wp_posts/wp_users/wp_usermeta keep per-line annotations. The bug this found: PHPCS annotations are a flat on/off switch per sniff, not a stack, so an inner `phpcs:enable X` ends an enclosing file-level disable at that point. Live in AudienceReader, AudienceWriter, AudienceEnvironmentRepository and AbstractRepository, with nothing red to show it. tests/Unit/PhpcsSuppressionTest.php now fails on a bare annotation, one with no reason, and one whose enable cancels an enclosing disable. No behaviour changes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D1wR59A8Z7d3QGYnm8q2KQ
…ts to 9 (#1044) The batch accumulated five #1030 bullets that differ only in which slice of the vacuous-test register they closed, three #1028 bullets on one audit, two #1035 bullets and two #1024 bullets. Each issue now gets one bullet stating where it ended up, which is what a release note is for — the linked PRs hold the per-slice detail. One number was wrong and is corrected here. The #1035 bullet claimed 373 annotations removed, taken from the PR title, which had summed classification counts rather than removals. Measured across the merge commit, the tree went from 964 annotations to 603: 361 gone. Trimming also brought every bullet back under ~390 characters, from a worst case of 599. No entry was dropped and every issue reference is preserved. Claude-Session: https://claude.ai/code/session_01D1wR59A8Z7d3QGYnm8q2KQ Co-authored-by: Claude <noreply@anthropic.com>
…1035) (#1045) #1035 collapsed 375 per-line DirectDatabaseQuery annotations into 49 file-level disables. That trade is only honest while every table the file touches is one of the plugin's own ffc_* — which is the test I ran by hand to choose those files, and which nothing then enforced. A core-table query added to any of them would have been silently unflagged, exactly the case the 13 mixed files were deliberately left per-line to avoid. test_file_level_direct_query_disables_only_cover_plugin_tables() now runs that same test on every CI: for each file whose disable covers the DirectDatabaseQuery family, fail if it names a $wpdb core-table property or builds a table from $wpdb->prefix with a non-ffc_ literal. Verified both ways — adding a $wpdb->users read to AudienceReader fails it, and so does removing the one allowlist entry. That entry is MigrationForeignKeys, which is unavoidable rather than overlooked: the foreign keys it creates all point at wp_users, so it has to read that table's engine and name it in the REFERENCES clause. The reason sits inline, as the AjaxWiringTest allowlists do. It sees literals only, so a table name reached through a variable or a constant is a false negative — acceptable for a deny-check whose job is to catch the ordinary case of someone adding a core-table query. CLAUDE.md gains the audit's durable findings, which until now lived only in commit messages and issue comments: the two suppression guards and what each fails on, the flat-switch behaviour of phpcs:enable, the file-level DDQ policy and its scope gate, the neutralise-and-measure method for deadness (and why it is out of CI), and the three parsing facts that cost the audit a parse error and two wrong classifications. Claude-Session: https://claude.ai/code/session_01D1wR59A8Z7d3QGYnm8q2KQ Co-authored-by: Claude <noreply@anthropic.com>
…3 suppressions (#1035) (#1046) Answers the question #1035 left open — not how to vigilate the suppressions but how to stop needing them. Core\RequestInput already made such annotations unnecessary (lines calling it carried none while the raw $_GET read below did), but it only had get_get_string. It gains the three readers it was missing: get_get_key (sanitize_key, the right sanitiser for the slug-shaped page/tab/action/status parameters of a wp-admin URL), get_get_int (mirrors get_post_int) and has_get (presence without reading, since `?ffc_saved=` is a legitimate flag). 80 of the 96 GET call sites now route through them. Annotation lines 557 -> 484, sniff mentions 681 -> 603, NonceVerification 164 -> 92. GET only, and that is the design. Routing a read through the helper removes the warning, not the risk — RequestInput does not verify nonces. The 96 GET/REQUEST sites are graded Recommended by WPCS itself, where there was never a nonce to check; the 76 POST sites are Missing, where the sniff is working and the annotation is the record that a human checked. Those are untouched. The 16 left behind are deliberate: 14 $_REQUEST reads (it merges POST, so a GET reader is the wrong tool), an empty() guard where '0' must read as absent, and one raw read that needs the un-sanitised value. Four converted sites are write handlers; each was read to confirm a capability check plus check_admin_referer/wp_verify_nonce already guards it. Three guard sites broke under a mechanical swap — has_get() hid the isset from the possibly-undefined-index sniff — and were converted whole instead. The tests that alias-mock RequestInput needed the new readers taught to them; all 43 mocks now stub them faithfully, reading from $_GET so each test's own setup keeps driving the assertion. The 73 dead annotations were removed by measurement, not assumption. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D1wR59A8Z7d3QGYnm8q2KQ
…ngs (#1035) (#1047) rule can be switched off wholesale — phpcs.xml.dist, .stylelintrc.json, phpstan.neon.dist and phpunit.xml.dist. Each entry there is a phpcs:disable with repo-wide reach, so the same measure-first method was run over all of them. The outcome is mostly "the exclusion was right", and the point of this commit is that the verdicts are now recorded rather than folklore. Two stylelint rules ON: declaration-block-no-shorthand-property-overrides and declaration-block-no-redundant-longhand-properties. Both measured zero findings, so they cost nothing today and guard a real bug class — a shorthand silently wiping an earlier longhand. no-descending-specificity (92) and no-duplicate-selectors stay off; all 4 duplicate hits are deliberate section splits, a token block and a layout block under one selector, each commented. Two phpcs rules stay OFF, and this is the finding worth keeping. UnusedFunctionParameter reports 43, of which 33 are structural false positives: 14 are parameters the method's included templates/*.php partial consumes — the sniff cannot see across an include, and extracting markup into templates/ is this project's own convention, so the architecture creates the class — and 19 are signatures the caller fixes (WP hooks, a guard interface). Enabling it would mean 33 new annotations, exactly the churn #1035 removed. CommentedOutCode is 9 for 9 false: the heuristic reads an enum in a comment ("// 'daily' or 'span'") as code. The 10 remaining unused parameters were read, and three were real dead weight. return_to_draft() and bulk_return_to_draft() took a reviewer id while the update sets reviewed_by to NULL — the docblock promised an attribution the method deletes; collect_form_data() took a user id and reads $_POST. Dropped, with their callers and tests. A fourth, RateLimitChecker:: check_verification()'s unused $token, is left alone deliberately: it is a public method on a Security façade with five call sites, so removing a parameter there is the deprecation-cycle case, not a cleanup. It is a dormant per-token limit worth a decision, not a silent edit. Two PHPStan excludePaths, includes/views and includes/libraries, are removed: neither has ever existed in the history, and PHPStan's (?) optional-path suffix meant nothing ever reported them. The surviving two are documented as the markup-only carve-out they are, in step with the coverage scope — and CLAUDE.md now records that includes/self-scheduling/views is deliberately NOT excluded, so "a directory named views is markup" is not a repo-wide truth. Both @ob_end_clean() loops are replaced by a level-guarded form. The naive rewrite is worse than the original: while ( ob_get_level() > 0 ) spins forever when a buffer cannot be removed (zlib compression, one PHP owns), which is precisely what the @ plus the false return was handling. The form used here checks the level and breaks on a failed close, so it terminates by construction, drops the silence operator and the empty statement, and says what the failure case is. PHPCS green, stylelint green, PHPStan unchanged, 423 tests green. Claude-Session: https://claude.ai/code/session_01D1wR59A8Z7d3QGYnm8q2KQ Co-authored-by: Claude <noreply@anthropic.com>
…#1049) Surfaced by the unused-parameter measurement in #1035. The parameter is dead at both ends: RateLimiter::check_verification() forwards a token that RateLimitChecker::check_verification() never reads, and none of the five call sites — four in VerificationHandler, one in SubmissionRestController — passes one. Throttling is keyed on the IP alone, 10/hour and 30/day. Deprecating rather than implementing, because the limit it advertises is not needed. Keying on the token would only add defence against a distributed brute-force of a single auth code — many IPs, each under the hourly ceiling, all guessing one code — and the code space rules that out: AuthCodeService::generate_auth_code() emits 12 characters over [A-Z0-9], so 36^12 ≈ 4.7e18 (~62 bits), with global uniqueness verified across the three tables that hold one. At 30 attempts/day/IP, reaching 1% would need orders of magnitude more source IPs than exist. Leaving it is worse than removing it: a $token accepted and ignored on a Security method advertises a per-token limit that does not exist, which is the #936 shape — a signature that leads a reader, or a future caller, to assume a protection that isn't there. Deprecating rather than removing outright, because RateLimiter is a public static façade and no code scan sees an external integration calling it with two arguments. That is the case CLAUDE.md reserves the versioned cycle for, and the #730 precedent (the success/fail keys on get_audit_log_summary()) is the same shape: zero internal consumers, invisible to any diagnostic, retired by announcement. Removal lands in 6.24.0, the 2nd feature release after this notice, matching that precedent. Both docblocks now say the argument is ignored, name the removal version and the issue, and the checker's records why a per-token limit is not warranted — plus the trigger that would reverse the decision: implement it instead if the auth code ever shortens, or a flow appears that accepts a short human-typed code. Docblocks and CHANGELOG only; no signature or behaviour change. Claude-Session: https://claude.ai/code/session_01D1wR59A8Z7d3QGYnm8q2KQ Co-authored-by: Claude <noreply@anthropic.com>
Bumps FFC_VERSION in the three sync sites (plugin header, constant, readme.txt Stable tag) and renames the [Unreleased] heading to [6.22.0] (2026-09-04), opening a fresh empty [Unreleased] above it. No source change. The batch is the #1027/#1028/#1030/#1034/#1035 suppression-audit arc plus the #865 seeder fix, the #1022 update-screen compatibility fields, the #1024 activity-log labels and the #1048 deprecation notice. The cache key also rotates for the pt_BR language update that landed after 6.21.0. The release commit's short SHA is backfilled onto the [6.22.0] heading in a later PR, after develop is reset to main. Claude-Session: https://claude.ai/code/session_01D1wR59A8Z7d3QGYnm8q2KQ Co-authored-by: Claude <noreply@anthropic.com>
Coverage Report for CI Build 33856860715Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Warning No base build found for commit Coverage: 89.925%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
rpgmem
marked this pull request as ready for review
September 4, 2026 09:08
rpgmem
enabled auto-merge (squash)
September 4, 2026 09:08
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
2 participants
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.
Release PR: consolidates the 23-commit
developbatch intomainunder one version bump.Draft on purpose — this is the production deploy. Flip to ready + auto-merge only when the batch has been validated against the testes site.
The batch
The suppression-audit arc (#1027, #1028, #1030, #1034, #1035) — five issues that started from "why is this line suppressed?" and ended with the gates actually running:
phpcs:ignoresuppressions carry no reason — establish per case whether each is still needed #1028 enabledWordPress.DB.DirectDatabaseQueryandWordPress.Security.ValidatedSanitizedInput. Neither is in theWordPress-Extrastandard the ruleset uses, so neither had ever run — which is also why 333phpcs:ignoreannotations were inert. All 179 sites the two sniffs report were read individually; no unsanitised input was found.phpcs:ignorestill without a reason, plus 85 unclassifiedphpcs:disableblocks #1035 audited the survivors: 361 more removed (964 → 603 sniff mentions), every one that stays now says why, and a guard fails CI on a bare, unexplained or self-cancelling annotation. Four classes had an innerphpcs:enablesilently cancelling their own file-leveldisable. The ~50 file-levelDirectDatabaseQuerydisables are now scope-gated: adding awp_posts/wp_usersquery to one fails CI. 80 admin$_GETreads moved toCore\RequestInput, which gained the readers it was missing. The config-level exclusions were audited the same way — two stylelint rules turned on, two PHPStanexcludePathsthat never existed removed.@phpstan-ignoresuppressions carry a placeholder "Description." instead of a reason #1027 did the same for PHPStan: all 31 suppressions used the non-filtering@phpstan-ignore-next-lineform and now name what they silence.Fixes
html/#865 — a failed first seed left the certificate-template pool permanently empty, because the seeder recorded the seed version even when it created nothing. This is condition 1 for retiring the legacyhtml/fallback; the removal is deliberately scheduled for 6.23.0, not this release, so an affected install receives the repair before losing the safety net.@phpstan-ignoresuppressions carry a placeholder "Description." instead of a reason #1027 —ReregistrationSubmissionReader::get_by_reregistration()could return null from a method declared: array. Surfaced by narrowing the suppression that hid it.ucwords()fallback; submission link/unlink also recorded the wrong level.GitHubUpdaterhard-coded the compatibility fields instead of readingreadme.txt.Deprecated
$tokenparameter oncheck_verification()(announce 6.22.0, remove 6.24.0) #1048 — ⚠check_verification()'s$tokenargument, on bothRateLimiterandRateLimitChecker. Never read; no caller passes it. Removed in 6.24.0.Version
6.21.0 → 6.22.0(minor: a deprecation notice plus user-visible fixes). Bumped in the three sync sites by #1050, which also renamed[Unreleased]to## [6.22.0] (2026-09-04).No
assets/**JS or CSS changed since v6.21.0 — the cache-key rotation this release carries is for the pt_BR language update (025e1a02).After the merge
release.ymlfires onv*).developtomain— not a rebase; the squash already contains every develop commit.## [6.22.0]heading, in a PR that lands after the reset.🤖 Generated with Claude Code
https://claude.ai/code/session_01D1wR59A8Z7d3QGYnm8q2KQ
Generated by Claude Code