fix: preserve incomplete usermeta during duplication#1424
Conversation
📝 WalkthroughWalkthroughThis PR modifies site duplication to read user metadata directly from the database and preserve incomplete serialized object payloads that would be corrupted by normal unserialization and reserializing. It enforces strict type checking across the keep-users flow and adds test coverage for the incomplete object handling. ChangesUsermeta duplication refactor with incomplete object handling
Sequence DiagramsequenceDiagram
participant Caller
participant MUCD_Duplicate
participant WordPress as WordPress Database
participant MetaHelper as Incomplete Object Handler
Caller->>MUCD_Duplicate: duplicate_site(keep_users='yes')
MUCD_Duplicate->>MUCD_Duplicate: Init with strict 'yes' === keep_users check
MUCD_Duplicate->>WordPress: SELECT meta_key, meta_value FROM usermeta
WordPress-->>MUCD_Duplicate: Raw serialized metadata rows
MUCD_Duplicate->>MetaHelper: copy_user_meta_with_preservation()
MetaHelper->>MetaHelper: Unserialize and detect __PHP_Incomplete_Class
alt Incomplete object detected
MetaHelper->>WordPress: Direct SQL INSERT/UPDATE raw meta_value
else Normal serialized data
MetaHelper->>WordPress: update_user_meta() reserialize normally
end
WordPress-->>MetaHelper: Meta copied
MetaHelper-->>MUCD_Duplicate: Completed
MUCD_Duplicate-->>Caller: Duplication finished
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 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: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
inc/duplication/duplicate.php (1)
316-346: 💤 Low valueConsider logging or returning failure status from raw meta operations.
$wpdb->update()and$wpdb->insert()returnfalseon failure, but this is currently ignored. For this hotfix, silent failure is acceptable (better than a checkout crash), but a follow-up could add error logging to surface database write issues during duplication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@inc/duplication/duplicate.php` around lines 316 - 346, The code calls $wpdb->update and $wpdb->insert and currently ignores their return values; capture each call's return (e.g. $res = $wpdb->update(...) / $res = $wpdb->insert(...)) and if the result === false, either log the failure (error_log or a logger) including context ($meta_id, $user_id, $meta_key) or return a failure status from the enclosing function so callers can handle it; update both branches (the $meta_id update branch and the insert branch using $raw_meta_value) to perform this check and take the chosen action.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/WP_Ultimo/Helpers/Site_Duplicator_Test.php`:
- Around line 192-194: The finally block currently uses a truthy check on
$result before calling wpmu_delete_blog($result, true), which can pass a
WP_Error object and crash cleanup; change the guard to ensure $result is a valid
integer site ID (e.g., use is_int($result) or ctype_digit/is_numeric + cast and
> 0) before calling wpmu_delete_blog so only numeric site IDs are deleted and
WP_Error objects are ignored.
---
Nitpick comments:
In `@inc/duplication/duplicate.php`:
- Around line 316-346: The code calls $wpdb->update and $wpdb->insert and
currently ignores their return values; capture each call's return (e.g. $res =
$wpdb->update(...) / $res = $wpdb->insert(...)) and if the result === false,
either log the failure (error_log or a logger) including context ($meta_id,
$user_id, $meta_key) or return a failure status from the enclosing function so
callers can handle it; update both branches (the $meta_id update branch and the
insert branch using $raw_meta_value) to perform this check and take the chosen
action.
🪄 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: 1ebec419-70b6-4265-9ec4-0964690ab798
📒 Files selected for processing (2)
inc/duplication/duplicate.phptests/WP_Ultimo/Helpers/Site_Duplicator_Test.php
| if ($result) { | ||
| wpmu_delete_blog($result, true); | ||
| } |
There was a problem hiding this comment.
Guard cleanup with a strict site-ID check
Line 192 uses a truthy check on $result; if duplication returns WP_Error, the finally block can call wpmu_delete_blog() with an object and crash cleanup. Use an integer check before deleting the blog.
Suggested fix
- } finally {
- if ($result) {
- wpmu_delete_blog($result, true);
- }
+ } finally {
+ if (is_int($result) && $result > 0) {
+ wpmu_delete_blog($result, true);
+ }
wp_delete_user($user_id);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ($result) { | |
| wpmu_delete_blog($result, true); | |
| } | |
| if (is_int($result) && $result > 0) { | |
| wpmu_delete_blog($result, true); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/WP_Ultimo/Helpers/Site_Duplicator_Test.php` around lines 192 - 194, The
finally block currently uses a truthy check on $result before calling
wpmu_delete_blog($result, true), which can pass a WP_Error object and crash
cleanup; change the guard to ensure $result is a valid integer site ID (e.g.,
use is_int($result) or ctype_digit/is_numeric + cast and > 0) before calling
wpmu_delete_blog so only numeric site IDs are deleted and WP_Error objects are
ignored.
superdav42
added
the
review-feedback-scanned
1 participant
Summary
wp_usermetaso source values stay raw until we decide how to copy them.__PHP_Incomplete_Classby writing the raw serialized value directly, avoiding WordPresswp_unslash()/map_deep()object mutation.Production evidence
Controlled duplicate probe on the deployed build showed the remaining checkout failure path:
inc/duplication/duplicate.php:196MUCD_Duplicate::copy_users()update_user_meta()->update_metadata()->wp_unslash()->stripslashes_deep()->map_deep()Error: The script tried to modify a property on an incomplete objectforYoast\WP\SEO\Presenters\Admin\Indexing_Notification_PresenterVerification
php -l inc/duplication/duplicate.phpphp -l tests/WP_Ultimo/Helpers/Site_Duplicator_Test.phpvendor/bin/phpcs inc/duplication/duplicate.php tests/WP_Ultimo/Helpers/Site_Duplicator_Test.phpgit diff --checkNot Run
vendor/bin/phpunit --filter test_duplication_preserves_incomplete_serialized_user_metais blocked locally because/tmp/wordpress-tests-lib/includes/functions.phpis missing.wp evalprobe is blocked because this plugin checkout's configured../wordpresspath is not a WordPress install.Summary by CodeRabbit
Bug Fixes
Tests