perf: optimize template table copy#1595
Conversation
📝 WalkthroughWalkthrough
ChangesTable Selection Subsystem
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
MERGE_SUMMARY Implemented GH#1350 by optimizing template table selection during site duplication. Summary:
Verification:
Notes:
aidevops.sh v3.29.41 plugin for OpenCode v1.17.11 with gpt-5.5 spent 10m and 86,781 tokens on this with the user in an interactive session. |
🔨 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: |
Admin Merge Fallback (t2247)Branch protection blocked the plain Merge method: Original branch-protection errorRemediation: If this bypass was unintended, revert with aidevops.sh v3.29.41 plugin for OpenCode v1.17.11 with unknown spent 12m and 90,643 tokens on this with the user in an interactive session. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@inc/duplication/data.php`:
- Line 277: The emptiness check currently relies on get_table_row_count(), which
can hide a transient count failure by returning 0 and incorrectly treat a table
as empty. Update the table-existence predicate in the duplication/data.php logic
to probe for a single row instead of doing a full COUNT, and make the check fail
open so probe errors still preserve the schema copy path. Apply the same change
anywhere the same empty-table check is duplicated, including the other affected
block near the schema-copy logic.
- Line 195: The new public filter hooks are using the wrong prefix and should be
renamed to match the repository hook contract. Update the hook names in the
duplication flow methods that call apply_filters, including the table-copy hook
in the data duplication logic and the other related filter sites noted in this
diff, replacing every mucd_* hook name with the corresponding wu_* name. Then
update any tests and expectations that reference these hook names so they match
the renamed hooks.
🪄 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: 370bd115-ca40-46ea-a747-665fe50e98f3
📒 Files selected for processing (2)
inc/duplication/data.phptests/WP_Ultimo/Duplication/MUCD_Data_Test.php
| * @param int $from_site_id Source site ID. | ||
| * @param int $to_site_id Target site ID. | ||
| */ | ||
| return (bool) apply_filters('mucd_should_copy_table', $copy, $table, $table_base_name, $from_site_id, $to_site_id); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Rename the new filters to use the wu_ prefix.
These are new public extension points, but mucd_* violates the repository hook naming contract. Rename them before release, and update the tests accordingly. As per coding guidelines, “Hooks (actions and filters) must use the wu_ prefix”. <coding_guidelines>
Proposed hook rename
-return (bool) apply_filters('mucd_should_copy_table', $copy, $table, $table_base_name, $from_site_id, $to_site_id);
+return (bool) apply_filters('wu_mucd_should_copy_table', $copy, $table, $table_base_name, $from_site_id, $to_site_id);
-$runtime_tables = (array) apply_filters('mucd_runtime_tables_to_ignore', $runtime_tables);
+$runtime_tables = (array) apply_filters('wu_mucd_runtime_tables_to_ignore', $runtime_tables);
-return (bool) apply_filters('mucd_skip_empty_tables', true, $from_site_id, $to_site_id);
+return (bool) apply_filters('wu_mucd_skip_empty_tables', true, $from_site_id, $to_site_id);
-$required_tables = (array) apply_filters('mucd_required_tables_to_copy', $required_tables);
+$required_tables = (array) apply_filters('wu_mucd_required_tables_to_copy', $required_tables);Also applies to: 251-251, 300-300, 333-333
🤖 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/data.php` at line 195, The new public filter hooks are using
the wrong prefix and should be renamed to match the repository hook contract.
Update the hook names in the duplication flow methods that call apply_filters,
including the table-copy hook in the data duplication logic and the other
related filter sites noted in this diff, replacing every mucd_* hook name with
the corresponding wu_* name. Then update any tests and expectations that
reference these hook names so they match the renamed hooks.
Source: Coding guidelines
| return false; | ||
| } | ||
|
|
||
| return 0 === self::get_table_row_count($table); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail open and avoid full table counts for emptiness checks.
get_table_row_count() casts a failed count query to 0, so a transient SQL/count failure can make an optional table look empty and skip its schema. The predicate only needs row existence; probe one row and copy on probe errors.
Proposed fail-open existence probe
- return 0 === self::get_table_row_count($table);
+ return ! self::table_has_rows($table);
}
@@
- private static function get_table_row_count($table) {
+ private static function table_has_rows($table) {
+ global $wpdb;
- $count = self::do_sql_query('SELECT COUNT(*) FROM `' . $table . '`', 'var', false);
+ $wpdb->last_error = '';
+ $has_rows = self::do_sql_query('SELECT 1 FROM `' . $table . '` LIMIT 1', 'var', false);
- return (int) $count;
+ if ('' !== $wpdb->last_error) {
+ return true;
+ }
+
+ return null !== $has_rows;
}Also applies to: 346-350
🤖 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/data.php` at line 277, The emptiness check currently relies
on get_table_row_count(), which can hide a transient count failure by returning
0 and incorrectly treat a table as empty. Update the table-existence predicate
in the duplication/data.php logic to probe for a single row instead of doing a
full COUNT, and make the check fail open so probe errors still preserve the
schema copy path. Apply the same change anywhere the same empty-table check is
duplicated, including the other affected block near the schema-copy logic.
superdav42
added
the
review-feedback-scanned
1 participant
Summary
MUCD_Data::db_copy_tables().MUCD_Dataregression tests for runtime, empty optional, non-empty optional, required, and forced-copy behavior.Closes #1350
Testing
composer installvendor/bin/phpcs inc/duplication/data.phpvendor/bin/phpstan analyse inc/duplication/data.phpvendor/bin/phpunit --filter MUCD_Data_Testaidevops.sh v3.29.41 plugin for OpenCode v1.17.11 with gpt-5.5 spent 10m and 84,233 tokens on this with the user in an interactive session.
Summary by CodeRabbit
New Features
Bug Fixes