Skip to content

fix: stop form stat meta duplicates in wp_give_formmeta (#8250) - #8265

Open
faisalahammad wants to merge 1 commit into
impress-org:developfrom
faisalahammad:fix/8250-form-stat-meta-duplicates
Open

fix: stop form stat meta duplicates in wp_give_formmeta (#8250)#8265
faisalahammad wants to merge 1 commit into
impress-org:developfrom
faisalahammad:fix/8250-form-stat-meta-duplicates

Conversation

@faisalahammad

@faisalahammad faisalahammad commented Jul 18, 2026

Copy link
Copy Markdown

Summary

Stop _give_form_earnings, _give_form_sales, and _give_form_goal_progress from accumulating duplicate rows in wp_give_formmeta on every recalc. Collapse existing duplicates once. Add the composite index the V2 admin list needs to stop doing a Cartesian join. Close two related null-fatals in the same admin flow.

Fixes #8250

Root cause

Stat writers (give_update_goal_progress, give_recount_form_income_donation, the daily cron, the Give_Donate_Form earnings/sales resets, the recount admin tools, and the v3 TransferDonations migration) all called give_update_meta($id, $key, $val) without $meta_type='form'. The dispatcher fell through to the default update_post_meta branch, so the write landed in wp_postmeta. The V2 DonationFormsRepository reads the same keys from wp_give_formmeta via attachMeta, so a row existed in both tables (or only postmeta) depending on which path ran last. For forms with no postmeta mirror, every recalc inserted another row, growing wp_give_formmeta to hundreds of rows per form. The V2 admin list joined these rows with SELECT DISTINCT and the result set blew up into a Cartesian product, slowing the Donation Forms admin from under a second to several minutes (277 s in the reporter's case).

The deeper update_post_metadata filter on Give_DB_Meta delegated to update_metadata('form', ...), but form is not a registered WP meta_type, so the filter loop never re-entered and the function returned null. WP core then wrote to wp_postmeta itself.

Changes

includes/misc-functions.php

Add a pre-switch guard in give_get_meta, give_update_meta, and give_delete_meta. When $meta_type is empty, the post type is give_forms, and give_has_upgrade_completed('v20_move_metadata_into_new_table') returns true, force $meta_type = 'form'. Sites without the v20 upgrade see no behavior change.

if ( '' === $meta_type
    && 'give_forms' === get_post_type( $id )
    && give_has_upgrade_completed( 'v20_move_metadata_into_new_table' )
) {
    $meta_type = 'form';
}

Why: one guard at the dispatcher covers every current and future stat writer. No need to thread $meta_type='form' through 8+ call sites.

includes/database/class-give-db-meta.php

When is_custom_meta_table_active() is true, perform the actual wp_give_formmeta read/write/delete in the filter handler and return a non-null value. WP core sees the non-null return at meta.php:250 and short-circuits, so it never re-queries wp_postmeta. Add the WP action mirrors (add_form_meta, updated_form_meta, deleted_form_meta) so any third-party listener on those hooks keeps firing. Reset the is_filter_callback guard on every early return so follow-up calls do not skip the filter by accident.

Why: direct update_post_meta($id, '_give_form_sales', 9) calls (and the 20+ other places in the codebase that bypass give_update_meta) would still write to postmeta. The deep filter catches them.

src/DonationForms/Migrations/FormStatMetaDedupeAndIndex.php (new)

A one-shot migration. id() = donation-forms-form-stat-meta-dedupe-and-index. run():

  1. DELETE fm FROM wp_give_formmeta fm JOIN wp_give_formmeta keeper ON keeper.form_id = fm.form_id AND keeper.meta_key = fm.meta_key AND keeper.meta_id > fm.meta_id WHERE fm.meta_key IN ('_give_form_earnings', '_give_form_sales', '_give_form_goal_progress'). Keeps the newest row per (form_id, meta_key), deletes the rest. Re-runnable — the self-join is a no-op after the first pass.
  2. SHOW INDEX FROM wp_give_formmeta WHERE Key_name = 'form_id_meta_key'. If absent, ALTER TABLE wp_give_formmeta ADD INDEX form_id_meta_key (form_id, meta_key(191)). The 191 prefix keeps the index under MySQL's 3072-byte key limit on utf8mb4.

Why: collapses the damage already in the wild, then speeds the V2 join. meta_key(191) fits in the index without truncation.

src/DonationForms/ServiceProvider.php

Register the new migration.

src/Campaigns/Models/Campaign.php

Null-guard defaultForm() when defaultFormId is null. Return null instead of throwing a TypeError in getById().

src/DonationForms/V2/DataTransferObjects/DonationFormQueryData.php

Coerce null stat values to 0 before constructing Money and casting to int. Null-guard Campaign::findByFormId() in getGoalSettings() and fall back to GoalSource::FORM() when the form has no campaign relation.

Why: the V2 admin list fatals on forms that do not have the stat meta mirrored in formmeta. Coercion makes the row render with zero values instead of throwing.

Tests

  • tests/Unit/DonationForms/Migrations/FormStatMetaDedupeAndIndexTest.php: 3 tests. Inserts 9 duplicate rows (3 keys x 3 values), runs the migration, asserts 3 rows remain with the newest values. Drops the index, runs again, asserts the index exists. Runs the migration twice on the same data and asserts the count is stable.
  • tests/Unit/Includes/MiscFunctionsFormMetaDispatchTest.php: 2 tests. Marks the v20 upgrade complete, calls give_update_meta three times with the same key, asserts one row in wp_give_formmeta with the latest value. Toggles the upgrade flag off, asserts the legacy wp_postmeta path. TearDown clears the upgrade flag so the change does not leak into other tests.
  • tests/Unit/Campaigns/CampaignDefaultFormTest.php: asserts defaultForm() returns null when defaultFormId is null.
  • tests/Unit/DonationForms/V2/DonationFormQueryDataFromObjectTest.php: builds a stdClass whose meta-key accesses all return null, asserts fromObject() returns a DTO with zero values for earnings/sales and 0 for campaignId.

Local automated suite was not executed (no working local MySQL with the test DB schema). All four files pass php -l. CI on the PR will run the full composer test matrix.

Reproduction

Before the fix, on a site that has completed the v20 upgrade:

// wp eval
give_update_meta( $form_id, '_give_form_sales', 5 );
give_update_meta( $form_id, '_give_form_sales', 7 );
give_update_meta( $form_id, '_give_form_sales', 9 );
// SELECT COUNT(*) FROM wp_give_formmeta WHERE form_id = $form_id AND meta_key = '_give_form_sales'
// 1 (the fix)
// 3 (before the fix)

After the fix, every call updates the existing row in place. The migration collapses any pre-existing duplicates to one row per (form_id, meta_key) on the next admin page load and adds the composite index.

How to test

  1. Back up the database.
  2. Replace the plugin with this branch and activate.
  3. Run wp eval 'give_update_goal_progress($form_id);' twice on a form.
  4. SELECT COUNT(*) FROM wp_give_formmeta WHERE form_id = $form_id AND meta_key = '_give_form_goal_progress'; returns 1.
  5. Visit GiveWP -> Donation Forms in the admin. List loads in under a second on a site that was previously slow.
  6. Verify the migration ran: SELECT option_value FROM wp_options WHERE option_name = 'give_completed_upgrades' AND option_value LIKE '%donation-forms-form-stat-meta-dedupe-and-index%'; returns one row.
  7. Verify the index exists: SHOW INDEX FROM wp_give_formmeta WHERE Key_name = 'form_id_meta_key'; returns one row.
  8. Verify duplicates collapsed: SELECT form_id, meta_key, COUNT(*) FROM wp_give_formmeta WHERE meta_key IN ('_give_form_earnings', '_give_form_sales', '_give_form_goal_progress') GROUP BY form_id, meta_key HAVING COUNT(*) > 1; returns zero rows.

Full test plan in TESTING_INSTRUCTIONS.md in the build artifact.

Risks

  • The dispatcher guard is gated on the v20 upgrade flag, so sites that have not migrated keep the legacy postmeta behavior.
  • The migration is idempotent (self-join re-runs to a no-op, ALTER is gated on SHOW INDEX).
  • meta_key(191) is a composite index, not a unique key. A unique key is the obvious follow-up once the field is confirmed clean across all supported sites. Out of scope here.
  • The deep filter returns non-null for is_custom_meta_table_active(). If a third-party plugin was hooking update_post_metadata to log every stat write, those hooks will no longer fire for form stat keys. This matches the intent of the v20 formmeta migration (single source of truth) and is the same trade-off the existing __update_meta handler makes for payment meta.

…8250)

Stat writers (give_update_goal_progress, give_recount_form_income_donation,
the daily cron, the Give_Donate_Form earnings/sales resets, and the v3
TransferDonations migration) called give_update_meta() without
$meta_type='form', so writes landed in wp_postmeta. The V2 repository
reads the same keys from wp_give_formmeta via attachMeta, so a row
existed in both tables (or only postmeta) depending on which path ran
last. For forms with no postmeta mirror, every recalc inserted another
row, growing wp_give_formmeta to hundreds of rows per form. The V2 admin
list self-joined these rows with SELECT DISTINCT and the result set blew
up into a Cartesian product, slowing the Donation Forms admin from under
a second to several minutes.

Fix the dispatcher (give_get/update/delete_meta in misc-functions.php) so
any call against a give_forms post on a site that has completed the
v20_move_metadata_into_new_table upgrade routes to meta_type='form'.
Sites without the upgrade see no behavior change. Fix the deep
update_post_metadata filter in Give_DB_Meta so it performs the
wp_give_formmeta write itself and returns a non-null value, which makes
WP core short-circuit at meta.php:250 and skip its own postmeta write.
Fire the WP action mirrors (add/updated/deleted_form_meta) so listeners
keep working.

Ship FormStatMetaDedupeAndIndex, a one-shot migration that collapses
existing duplicates for the three stat keys (keeps the newest meta_id)
and adds a composite index form_id_meta_key (form_id, meta_key(191)) so
the V2 attachMeta join uses an index path. The dedupe self-join is a
no-op on re-run, and the ALTER is gated on a SHOW INDEX check.

Also close two related null-fatals in the same admin flow:
Campaign::defaultForm() now null-guards defaultFormId, and
DonationFormQueryData::fromObject() coerces null stat values to 0 and
null-guards Campaign::findByFormId() so the typed campaignId and Money
properties no longer throw a TypeError on forms with no campaign
relation or no mirrored stat row.

Includes unit tests for the migration (dedupe, index, idempotence), the
dispatcher (routes to formmeta when the upgrade is complete, falls back
to postmeta when it is not), the Campaign null-guard, and the DTO
null-coercion.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant