GH#820: backfill postmeta for nav_menu_item, attachment, and Elementor posts on site clone#825
Conversation
…d Elementor posts on site clone MUCD_Data::copy_data() uses INSERT...SELECT but misses postmeta rows for nav_menu_item, attachment, and elementor_library posts. The Elementor Kit also gets stub postmeta that INSERT NOT EXISTS silently skips. Add backfill_postmeta() after copy_data() with four targeted methods: - backfill_nav_menu_postmeta(): fills _menu_item_* keys for nav_menu_item - backfill_attachment_postmeta(): fills _wp_attached_file etc. for attachments - backfill_elementor_postmeta(): catch-all for _elementor_* meta on any post - backfill_kit_settings(): force-overwrites Kit settings with update_post_meta Also passes from_site_id in wu_duplicate_site action args so hooks can access the source template without relying on MUCD's hardcoded params. Fixes #820
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 46 minutes and 8 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request fixes a critical postmeta loss bug during site duplication by adding backfill operations for navigation menu items, attachments, Elementor library posts, and Kit settings. It resolves the template source via site meta over function parameters, implements postmeta integrity verification, and updates the duplication action payload to include the source template ID. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
|
Performance Test Results Performance test results for d298d84 are in 🛎️! Note: the numbers in parentheses show the difference to the previous (baseline) test run. Differences below 2% or 0.5 in absolute values are not shown. URL:
|
…olution) + unit tests
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
Adds 8 tests covering all 4 bugs from GH#820: - nav_menu_item postmeta backfill (Bug 2) - attachment postmeta backfill (Bug 3) - elementor_library postmeta backfill (Bug 4) - Kit settings force-overwrite (Bug 1) - same-site skip guard - idempotency (NOT EXISTS prevents duplicates) - wu_duplicate_site action payload includes from_site_id - no-op when template has no Elementor Kit For #820
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/WP_Ultimo/Helpers/Site_Duplicator_Postmeta_Test.php (1)
638-645: These tests are asserting source text, not behavior.A formatting refactor can break them while the feature still works, and dead code can keep them green as long as the string remains in the file. Please drive
process_duplication()/duplicate_site()and assert the resolvedfrom_site_idor emitted hook payload instead.Also applies to: 664-672, 686-695, 710-718
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@inc/helpers/class-site-duplicator.php`:
- Around line 284-296: The backfill_postmeta call is copying raw _elementor_*
and other postmeta after MUCD_Data::copy_data() (which runs db_update_data())
finishes, so those values never pass through the source→target rewrite; move or
re-run the rewrite for these meta values by either (A) performing the
backfill_postmeta step before MUCD_Data::copy_data()/db_update_data() or (B)
invoking the same rewrite/replace routine that db_update_data() uses on the
newly-inserted meta after self::backfill_postmeta(...) completes; locate and
update the call to self::backfill_postmeta and ensure you apply the replacement
logic to keys like _elementor_page_settings, other _elementor_* keys and menu
URLs (and keep verify_kit_integrity after the rewrite) so cloned meta contain
target-domain values.
- Around line 267-269: Do not mutate $args->from_site_id after
copy_data()/copy_files() have run; instead resolve the template override into a
new variable (e.g. $resolved_template_from_site_id or $template_source_id) when
you read wu_template_id and, for the backfill/hook path and
copy_users()/wu_duplicate_site payload only, use that new variable; leave
$args->from_site_id intact so copy_data(), copy_files(), and any earlier logic
continue to reference the original source. Ensure the same non-mutating change
is applied to the other occurrence(s) handling the template override (the block
currently duplicated further down).
In `@tests/WP_Ultimo/Helpers/Site_Duplicator_Postmeta_Test.php`:
- Around line 316-317: The test is calling unserialize() on the result of
get_post_meta($page_id, '_elementor_page_settings', true) which already returns
an unserialized value; remove the extra unserialize() call and assert directly
on the returned value. Update the assertion in Site_Duplicator_Postmeta_Test to
call get_post_meta($page_id, '_elementor_page_settings', true) into $settings
(or reuse the existing $settings variable) and use
$this->assertIsArray($settings) instead of
$this->assertIsArray(unserialize($settings)). Ensure any other tests referencing
get_post_meta(..., true) are similarly changed so they don't call unserialize()
again.
In `@tests/WP_Ultimo/Helpers/Site_Duplicator_Test.php`:
- Around line 604-615: Change the test so it fails immediately when
Site_Duplicator::duplicate_site returns a WP_Error instead of silently skipping
assertions: after calling
Site_Duplicator::duplicate_site($this->template_site_id, 'Action Test Site',
$args) check the result with an assertion that it is not a WP_Error (e.g.
assertFalse(is_wp_error($result)) or assertNotInstanceOf(WP_Error::class,
$result)) and include the WP_Error message for context; then proceed to assert
against $captured and call wpmu_delete_blog($result, true) only when that
assertion passes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1cb836dc-356b-43c1-88c0-d45134f58bbf
📒 Files selected for processing (3)
inc/helpers/class-site-duplicator.phptests/WP_Ultimo/Helpers/Site_Duplicator_Postmeta_Test.phptests/WP_Ultimo/Helpers/Site_Duplicator_Test.php
| $meta_template = (int) get_site_meta($args->to_site_id, 'wu_template_id', true); | ||
| if ($meta_template > 0 && $meta_template !== (int) $args->from_site_id) { | ||
| $args->from_site_id = $meta_template; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Don't overwrite from_site_id in place after copy_data().
copy_data() and copy_files() have already run with the original source by this point, but copy_users() and the wu_duplicate_site payload now read the rewritten value. If wu_template_id differs, the clone can mix tables/files from one site with users and downstream hook context from another. Keep the resolved template ID in a separate variable for the backfill/hook path instead of mutating $args->from_site_id.
💡 Proposed direction
- $meta_template = (int) get_site_meta($args->to_site_id, 'wu_template_id', true);
- if ($meta_template > 0 && $meta_template !== (int) $args->from_site_id) {
- $args->from_site_id = $meta_template;
- }
+ $template_site_id = (int) $args->from_site_id;
+ $meta_template = (int) get_site_meta($args->to_site_id, 'wu_template_id', true);
+ if (0 < $meta_template && $meta_template !== (int) $args->from_site_id) {
+ $template_site_id = $meta_template;
+ }
…
- self::backfill_postmeta($args->from_site_id, $args->to_site_id);
- self::verify_kit_integrity($args->from_site_id, $args->to_site_id);
+ self::backfill_postmeta($template_site_id, $args->to_site_id);
+ self::verify_kit_integrity($template_site_id, $args->to_site_id);
…
- 'from_site_id' => $args->from_site_id,
+ 'from_site_id' => $template_site_id,Also applies to: 298-320
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@inc/helpers/class-site-duplicator.php` around lines 267 - 269, Do not mutate
$args->from_site_id after copy_data()/copy_files() have run; instead resolve the
template override into a new variable (e.g. $resolved_template_from_site_id or
$template_source_id) when you read wu_template_id and, for the backfill/hook
path and copy_users()/wu_duplicate_site payload only, use that new variable;
leave $args->from_site_id intact so copy_data(), copy_files(), and any earlier
logic continue to reference the original source. Ensure the same non-mutating
change is applied to the other occurrence(s) handling the template override (the
block currently duplicated further down).
| self::backfill_postmeta($args->from_site_id, $args->to_site_id); | ||
|
|
||
| /* | ||
| * Verify Kit integrity after backfill. | ||
| * | ||
| * Compares the byte length of _elementor_page_settings between the | ||
| * template and the clone. If the clone has less than 80% of the | ||
| * template's byte count, the Kit fix is re-applied as a safety net. | ||
| * | ||
| * @since 2.3.1 | ||
| * @see https://github.com/Ultimate-Multisite/ultimate-multisite/issues/820 | ||
| */ | ||
| self::verify_kit_integrity($args->from_site_id, $args->to_site_id); |
There was a problem hiding this comment.
Backfilled postmeta is bypassing MUCD's rewrite step.
These copies happen after MUCD_Data::copy_data() has already finished db_update_data() in inc/duplication/data.php:26-150. Any source-domain values embedded in _elementor_*, Kit settings, or custom menu URLs are inserted verbatim here, so they won't go through the same source→target replacement pass as the rest of the clone.
Also applies to: 381-385, 424-586
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@inc/helpers/class-site-duplicator.php` around lines 284 - 296, The
backfill_postmeta call is copying raw _elementor_* and other postmeta after
MUCD_Data::copy_data() (which runs db_update_data()) finishes, so those values
never pass through the source→target rewrite; move or re-run the rewrite for
these meta values by either (A) performing the backfill_postmeta step before
MUCD_Data::copy_data()/db_update_data() or (B) invoking the same rewrite/replace
routine that db_update_data() uses on the newly-inserted meta after
self::backfill_postmeta(...) completes; locate and update the call to
self::backfill_postmeta and ensure you apply the replacement logic to keys like
_elementor_page_settings, other _elementor_* keys and menu URLs (and keep
verify_kit_integrity after the rewrite) so cloned meta contain target-domain
values.
| $result = Site_Duplicator::duplicate_site($this->template_site_id, 'Action Test Site', $args); | ||
|
|
||
| if ( ! is_wp_error($result)) { | ||
| $this->assertIsArray($captured); | ||
| $this->assertArrayHasKey('from_site_id', $captured); | ||
| $this->assertArrayHasKey('site_id', $captured); | ||
| $this->assertEquals($this->template_site_id, $captured['from_site_id']); | ||
| $this->assertEquals($result, $captured['site_id']); | ||
|
|
||
| wpmu_delete_blog($result, true); | ||
| } | ||
| } |
There was a problem hiding this comment.
Make this test fail when it can't reach the payload assertions.
If duplicate_site() returns WP_Error, this test silently skips every assertion about wu_duplicate_site. That turns an environment or duplication failure into a false positive for the new contract.
✅ Tighten the assertion path
$result = Site_Duplicator::duplicate_site($this->template_site_id, 'Action Test Site', $args);
-
- if ( ! is_wp_error($result)) {
- $this->assertIsArray($captured);
- $this->assertArrayHasKey('from_site_id', $captured);
- $this->assertArrayHasKey('site_id', $captured);
- $this->assertEquals($this->template_site_id, $captured['from_site_id']);
- $this->assertEquals($result, $captured['site_id']);
-
- wpmu_delete_blog($result, true);
- }
+ if (is_wp_error($result)) {
+ $this->fail('Site duplication failed: ' . $result->get_error_message());
+ }
+
+ $this->assertIsArray($captured);
+ $this->assertArrayHasKey('from_site_id', $captured);
+ $this->assertArrayHasKey('site_id', $captured);
+ $this->assertEquals($this->template_site_id, $captured['from_site_id']);
+ $this->assertEquals($result, $captured['site_id']);
+
+ wpmu_delete_blog($result, true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/WP_Ultimo/Helpers/Site_Duplicator_Test.php` around lines 604 - 615,
Change the test so it fails immediately when Site_Duplicator::duplicate_site
returns a WP_Error instead of silently skipping assertions: after calling
Site_Duplicator::duplicate_site($this->template_site_id, 'Action Test Site',
$args) check the result with an assertion that it is not a WP_Error (e.g.
assertFalse(is_wp_error($result)) or assertNotInstanceOf(WP_Error::class,
$result)) and include the WP_Error message for context; then proceed to assert
against $captured and call wpmu_delete_blog($result, true) only when that
assertion passes.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
1 participant
Summary
MUCD's
INSERT ... SELECTcopy misses postmeta rows for several post types, leaving cloned sites with broken menus, missing images, and wrong Elementor colors. This PR adds abackfill_postmeta()pass afterMUCD_Data::copy_data()to fill the gaps.What changed
inc/helpers/class-site-duplicator.php— 4 new protected methods + 1 orchestrator:backfill_postmeta()— called after\MUCD_Data::copy_data(), dispatches to the 4 sub-methodsbackfill_nav_menu_postmeta()— fills_menu_item_*keys fornav_menu_itemposts (without these, menus render as empty<li>tags)backfill_attachment_postmeta()— fills_wp_attached_file,_wp_attachment_metadata, etc. for attachments (without these,wp_get_attachment_image_url()returns false)backfill_elementor_postmeta()— catch-all for any_elementor_*meta on any post type (coverselementor_library,e-landing-page,elementor_snippet, and custom CPTs)backfill_kit_settings()— force-overwrites Elementor Kit_elementor_page_settingsand_elementor_datausingupdate_post_meta()(notINSERT NOT EXISTS) because Elementor creates stub Kit postmeta before MUCD runs, andINSERT NOT EXISTSsilently skips the overwriteAlso passes
from_site_idin thewu_duplicate_siteaction args so downstream hooks can access the source template without relying on MUCD's hardcoded params.Why
update_post_metafor Kit (not SQL INSERT)The Kit post gets created with stub Elementor defaults before MUCD's
INSERT ... SELECTruns. Because MUCD usesINSERT NOT EXISTS, the stub row survives and the clone gets default blue (#6EC1E4) instead of the template's actual color palette.update_post_meta()guarantees the overwrite.Testing
Resolves #820
Summary by CodeRabbit
New Features
Tests