fix: stop form stat meta duplicates in wp_give_formmeta (#8250) - #8265
Open
faisalahammad wants to merge 1 commit into
Open
fix: stop form stat meta duplicates in wp_give_formmeta (#8250)#8265faisalahammad wants to merge 1 commit into
faisalahammad wants to merge 1 commit into
Conversation
…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.
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
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.
Summary
Stop
_give_form_earnings,_give_form_sales, and_give_form_goal_progressfrom accumulating duplicate rows inwp_give_formmetaon 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, theGive_Donate_Formearnings/sales resets, the recount admin tools, and the v3TransferDonationsmigration) all calledgive_update_meta($id, $key, $val)without$meta_type='form'. The dispatcher fell through to the defaultupdate_post_metabranch, so the write landed inwp_postmeta. The V2DonationFormsRepositoryreads the same keys fromwp_give_formmetaviaattachMeta, 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, growingwp_give_formmetato hundreds of rows per form. The V2 admin list joined these rows withSELECT DISTINCTand 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_metadatafilter onGive_DB_Metadelegated toupdate_metadata('form', ...), butformis not a registered WP meta_type, so the filter loop never re-entered and the function returnednull. WP core then wrote towp_postmetaitself.Changes
includes/misc-functions.phpAdd a pre-switch guard in
give_get_meta,give_update_meta, andgive_delete_meta. When$meta_typeis empty, the post type isgive_forms, andgive_has_upgrade_completed('v20_move_metadata_into_new_table')returns true, force$meta_type = 'form'. Sites without the v20 upgrade see no behavior change.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.phpWhen
is_custom_meta_table_active()is true, perform the actualwp_give_formmetaread/write/delete in the filter handler and return a non-null value. WP core sees the non-null return atmeta.php:250and short-circuits, so it never re-querieswp_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 theis_filter_callbackguard 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 bypassgive_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():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.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.phpRegister the new migration.
src/Campaigns/Models/Campaign.phpNull-guard
defaultForm()whendefaultFormIdis null. Returnnullinstead of throwing aTypeErroringetById().src/DonationForms/V2/DataTransferObjects/DonationFormQueryData.phpCoerce null stat values to 0 before constructing
Moneyand casting toint. Null-guardCampaign::findByFormId()ingetGoalSettings()and fall back toGoalSource::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, callsgive_update_metathree times with the same key, asserts one row inwp_give_formmetawith the latest value. Toggles the upgrade flag off, asserts the legacywp_postmetapath. TearDown clears the upgrade flag so the change does not leak into other tests.tests/Unit/Campaigns/CampaignDefaultFormTest.php: assertsdefaultForm()returns null whendefaultFormIdis null.tests/Unit/DonationForms/V2/DonationFormQueryDataFromObjectTest.php: builds a stdClass whose meta-key accesses all return null, assertsfromObject()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 fullcomposer testmatrix.Reproduction
Before the fix, on a site that has completed the v20 upgrade:
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
wp eval 'give_update_goal_progress($form_id);'twice on a form.SELECT COUNT(*) FROM wp_give_formmeta WHERE form_id = $form_id AND meta_key = '_give_form_goal_progress';returns 1.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.SHOW INDEX FROM wp_give_formmeta WHERE Key_name = 'form_id_meta_key';returns one row.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.mdin the build artifact.Risks
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.is_custom_meta_table_active(). If a third-party plugin was hookingupdate_post_metadatato 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_metahandler makes for payment meta.