fix: bound, surface and contain a failing v2 migration - #863
Open
2ndkauboy wants to merge 7 commits into
Open
Conversation
`Handlers\PluginUpdate::maybe_update_database()` wrote the new database version before it migrated anything, so a single failure spent the one chance a site gets. Any error inside the steps — a fatal, a DB error, a timeout, a warning promoted to an exception — left the version already raised, `db_version_is_current()` reported the database as up-to-date on every later request, and the migration never ran again. The consequence is silent and unrecoverable: the legacy `antispam_bee` option is still there, `antispam_bee_options` was never written, and the site falls back to `Settings::$defaults`. The user's entire configuration is gone with no way to retrigger the migration short of editing `antispambee_db_version` by hand. The version write now happens after the steps have completed. Deferring it cannot make the migration run twice inside one request, because `self::$db_update_triggered` is set before any of it. The steps move into `run_migration_steps()`, reached through `static::` so a test can supply a failing migration without needing an error the suite cannot provoke on purpose. The `null` check becomes `is_scalar()` on the way: a fresh install has nothing to migrate, and a recorded revision that is not a scalar cannot be compared against — which used to fatal once and would now fatal on every request, since nothing raises the version past it. Fixes #798
`translate_lang` and `ignore_reasons` were single-value settings before the multiselect rework, so a 2.x install that never re-saved its settings still holds a plain string there. Both were passed straight into `convert_multiselect_values( array $values, ... )`, and PHP does not coerce a scalar into an `array` parameter even in weak mode, so the migration aborted with an uncaught `TypeError`. 2.x itself cast at every read site — the cast is what got lost on the way to 3.0. `normalize_multiselect_values()` now coerces whatever shape the legacy option holds into a list of non-empty strings. This is the same class of failure as #799, which hardened only *missing* keys: `?? []` triggers on `null`, never on a value of the wrong type. It is also the kind of aborting step #844 makes recoverable — that PR turns this crash into a retry instead of silent, permanent loss of the site's settings, but the crash itself remains, and would simply repeat on every request. Hence the stack: #844 makes the failure survivable, this makes it not happen.
The migration was retried on every request that reads a setting, including a frontend comment POST, with no cap and nothing to tell the site owner it had failed. A step that fatals or times out therefore turned into a repeating 500 that no one could attribute to Antispam Bee. Record an attempt in `antispambee_db_update_failures` *before* running the steps, so the cap holds for uncatchable failures too, and stop attempting after three. Catchable failures are recorded and swallowed, letting the request continue on `Settings::$defaults` instead of fataling. State is keyed by the target plugin version, so a release that ships a fix gets its own attempts. Once the cap is reached, `MigrationFailureNotice` tells administrators what happened, reassures them their v2 settings are intact, and offers a nonce-protected retry that clears the state. Also stop the v3 step from rebuilding over an existing `antispam_bee_options`. The write is the step's last action, so a present non-empty array is either a completed migration or settings the user saved by hand while the version was still stale, and neither should be overwritten. Finally, run the migration on `admin_init` as well, so in practice an admin page load performs it rather than a visitor's comment.
…er it A site whose migration keeps failing had no way out: once the attempt cap was reached the notice stayed forever, because nothing ever wrote the database version again. Add a second button that records the database as migrated without running the migration, for the user who configured the plugin by hand instead. Retrying is safe either way — the v3 step no longer overwrites settings that are already stored — and the notice now says so. Also delete `antispambee_db_update_failures` on uninstall, alongside the other plugin options.
✅ WordPress Plugin Check Report
📊 ReportAll checks passed! No errors or warnings found. 🤖 Generated by WordPress Plugin Check Action • Learn more about Plugin Check |
With the overwrite guard in place, "Retry migration" did nothing whenever settings were already stored: the step skipped its work, the migration counted as succeeded and the notice disappeared, which is indistinguishable from "Keep the current settings" and not what the button promised. A retry the user asked for now clears `antispam_bee_options` first, so the legacy settings are migrated again for real. The guard keeps protecting the automatic retries, which must never discard a configuration nobody asked them to touch. The notice wording follows the state it is describing: with settings stored the action reads "Discard the current settings and migrate again" and says what it replaces, and the introduction no longer claims the plugin runs on its defaults when it does not.
2ndkauboy
requested review from
florianbrinkmann and
stklcode
and removed request for
stklcode
August 29, 2026 19:52
The integration test still expected `maybe_run_plugin_updated_logic()` to let a failing step's exception escape, which is the behaviour this branch replaced: the failure is now recorded and swallowed so the request that triggered it carries on with the defaults instead of fataling. Its two assertions were already right, so only the expectation had to go. Cover the new behaviour while here: the recorded attempt and message the notice reads, and the migration no longer being tried at all once the attempts are spent. Clearing the failure option belongs in the fixture rather than in `reset_plugin_update_state()`, because a new request does not reset the count — that is the whole point of the cap.
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
1 participant
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.
Follow-up to #857, and stacked on it — the diff contains that PR's commit as well, so this should be merged after #857. Addresses the open questions from #798 / #844 about what actually happens when the migration fails.
#857 made a failed migration retryable by bumping
antispambee_db_versiononly afterrun_migration_steps()succeeds. That fixed the "one chance and it is gone" problem, but retrying was still unbounded and completely invisible:PluginUpdate::maybe_run_plugin_updated_logic()is called from insideSettings::get_options(), so any request that reads a setting triggers it — every wp-admin page that touches settings, and a frontend comment POST as well.self::$db_update_triggeredonly guards a single request.antispam_bee_optionsfrom the legacy option and wrote it unconditionally, so settings an admin saved by hand while the version was still stale were clobbered by the next attempt.What this changes
A bounded, fatal-safe attempt cap. The attempt is recorded in
antispambee_db_update_failuresbefore the steps run, not after. A step killed by a timeout or a true fatal never returns, so a counter raised afterwards would stay at zero and the site would keep retrying forever — burning the attempt up front is what makes the cap hold for uncatchable failures too. After three attempts the migration is no longer tried. The state is keyed by the target plugin version, so a release that ships a fix gets its own attempts and a site that gave up recovers on update without manual intervention.Catchable failures degrade instead of fataling.
run_migration_steps()runs insidetry/catch ( Throwable ). The message is recorded and the request continues onSettings::$defaults— for a comment being submitted that means slightly wrong spam settings, which is far better than turning every commenting visitor's request into a fatal.The user finds out.
MigrationFailureNoticeshows an error notice to administrators once the cap is reached, explaining the situation, confirming that the previous settings are still in the database and unharmed, and showing the recorded error message. It offers two nonce-protected actions, worded to match the state it is describing:antispam_bee_optionsand the failure state so the legacy settings are genuinely migrated again. Simply clearing the failure state would not do: the guard below would make the migration skip its work and report success, which is not what the button promises.No more clobbering. The v3 step returns early when
antispam_bee_optionsalready holds a non-empty array. The write is the step's last action, so a run that was cut short left nothing behind — a present array is therefore always either a completed migration or settings the user saved deliberately, and neither should be rebuilt by a retry nobody asked for. A stored value that is not an array is corrupt rather than a configuration and is still replaced. The guard applies to the automatic retries only; a retry the user explicitly requested clears the option first, which is the whole difference between the two.Migration is attempted on
admin_inittoo. The lazy call inSettings::get_options()stays, so settings are never silently wrong, but in practice an admin page load now performs the migration rather than a visitor's comment.Cleanup.
antispambee_db_update_failuresis deleted on uninstall alongside the other options.Multisite
Unchanged and deliberate: every site migrates lazily and independently the first time something on that site reads a setting. There is no network-wide loop, because migrating thousands of sites synchronously inside one request cannot work, and the per-site attempt cap now bounds what a failing migration costs. The reasoning is recorded in the
maybe_update_database()docblock so the choice is explicit rather than accidental.Testing
172 unit tests pass, PHPCS is clean across
src/, PHPStan reports no errors. New cases intests/Unit/Handlers/PluginUpdateTest.phpcover: a successful migration clearing the failure state, a failing step recording an attempt while leaving the version stale, the attempt being persisted before the step runs, no further attempts once the cap is reached, a failure state from another plugin version being discarded, existing settings not being overwritten by an automatic retry while corrupt values still are,reset_for_retry()clearing the stored settings so the manual retry really migrates again, andmark_as_migrated()stopping the retries without touching hand-saved settings.tests/Unit/Handlers/FailingPluginUpdate.phpis a subclass whose step always throws, which works becauserun_migration_steps()is called throughstatic::.To exercise the notice manually, seed the state (the
versionhas to match the plugin header or it is treated as stale and discarded):