fix(template-switch): harden override_site() against context pollution and identity loss#1083
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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)
📝 WalkthroughWalkthroughThe PR enhances ChangesSite Duplication Context Management & Restoration
Sequence DiagramsequenceDiagram
actor Caller
participant Dup as Site_Duplicator
participant WP as WordPress Multisite
participant Cache as Cache System
participant Elm as Elementor
Caller->>Dup: override_site(to_site_id, from_site_id, customer)
Dup->>Dup: Capture caller blog context
Dup->>WP: restore_current_blog() if switched
Dup->>Cache: Flush caches
Dup->>WP: Extend execution time & memory
Dup->>WP: Define WP_IMPORTING
Dup->>Dup: Snapshot identity options (blogname, admin_email, etc.)
Dup->>Cache: Pre-clean user cache for customer
Dup->>Dup: process_duplication(from_site_id, to_site_id)
Dup->>WP: update_blog_option() restore identity options
Dup->>Dup: cleanup_override_context(to_site_id, customer, caller_blog_id)
Dup->>WP: restore_current_blog() if needed
Dup->>Cache: Flush & invalidate target blog cache
Dup->>Cache: Clear user cache for customer
Dup->>Elm: Call backfill_kit_settings()
Elm->>Cache: Clear _elementor_css meta
Elm->>Elm: Instantiate Elementor\Core\Files\CSS\Post
Elm->>Elm: Call update() for Kit CSS regeneration
Elm-->>Dup: Return (errors suppressed)
Dup->>Caller: Return success/failure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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. Review rate limit: 0/1 reviews remaining, refill in 51 minutes and 18 seconds.Comment |
Rework the KursoPro template-switch fixes from PR #1082 to align with the current codebase and fix issues flagged during review: 1. Replace while(ms_is_switched) full-stack unwind with captured caller blog ID + restore-on-exit via cleanup_override_context(). Preserves the caller's switch_to_blog() context instead of unwinding to network root. 2. Change empty($opt_val) to false !== $opt_val on identity restore so intentionally blank values (e.g. empty blogdescription) are preserved instead of silently dropped. 3. Add cleanup_override_context() call to BOTH error return paths (process_duplication failure + save failure), not just the happy path. Prevents poisoned $wpdb/cache state from persisting after a failed override. 4. Remove duplicate force_copy_elementor_kit() public method — the existing backfill_kit_settings() + backfill_elementor_postmeta() + verify_kit_integrity() pipeline already handles Kit copying. Add active CSS regen via Elementor\Core\Files\CSS\Post::update() to backfill_kit_settings() instead. 5. Guard ini_set('memory_limit', '512M') against lowering an existing higher limit by checking wp_convert_hr_to_bytes() first. 6. Use proper @SInCE 2.4.0 version tags (replaces 2.x.x placeholder). Ref #1082
superdav42
force-pushed
the
fix/1082-template-switch-review
branch
from
622d88a to
b09e48b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
inc/helpers/class-site-duplicator.php (1)
282-286: 💤 Low valueConsider adding a note about the intentionally unbalanced
switch_to_blog().The
switch_to_blog($caller_blog_id)at line 285 intentionally lacks a matchingrestore_current_blog()to restore the caller's context. While the comment at lines 282-283 explains the intent, future maintainers might flag this as a bug. Consider adding a brief note that this is deliberate and that the caller is responsible for their own switch stack.📝 Suggested clarification
- // Restore the caller's blog context rather than leaving the - // request in the network root context. + // Restore the caller's blog context rather than leaving the + // request in the network root context. Intentionally unbalanced: + // the caller owns their switch_to_blog stack, we only restore + // the blog they were on when they called override_site(). if ( get_current_blog_id() !== $caller_blog_id ) { switch_to_blog($caller_blog_id); }🤖 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 282 - 286, Update the inline comment around the switch_to_blog($caller_blog_id) call to explicitly document that the unbalanced call is intentional: state that we only switch context back to the caller when get_current_blog_id() differs, that we deliberately do not call restore_current_blog() here, and that the caller is responsible for managing the switch/restore stack (i.e., calling restore_current_blog() if required); reference the symbols switch_to_blog(), get_current_blog_id(), $caller_blog_id and restore_current_blog() so future maintainers understand the decision and responsibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@inc/helpers/class-site-duplicator.php`:
- Around line 282-286: Update the inline comment around the
switch_to_blog($caller_blog_id) call to explicitly document that the unbalanced
call is intentional: state that we only switch context back to the caller when
get_current_blog_id() differs, that we deliberately do not call
restore_current_blog() here, and that the caller is responsible for managing the
switch/restore stack (i.e., calling restore_current_blog() if required);
reference the symbols switch_to_blog(), get_current_blog_id(), $caller_blog_id
and restore_current_blog() so future maintainers understand the decision and
responsibility.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 616178f5-e37e-4b51-82eb-10235d10d53a
📒 Files selected for processing (1)
inc/helpers/class-site-duplicator.php
🔨 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 e0bc728 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:
|
1 participant
Summary
Incorporates the template-switch fixes from #1082 by @kenedytorcatt (KursoPro) with review corrections applied on top.
Original contribution: @kenedytorcatt identified and fixed real production bugs where consecutive
override_site()calls caused customer site corruption —$wpdbcontext pollution fromMUCD_Data::copy_data(), admin_email loss, and stale Elementor Kit CSS. Their fix was validated with 18/18 passes across 6 templates in a production-mirror environment.This PR preserves that work (their commit is included with full git authorship) and addresses issues found during review:
while(ms_is_switched())full-stack unwind with captured$caller_blog_id+ restore-on-exit, so the caller'sswitch_to_blog()context is preserved! empty($opt_val)tofalse !== $opt_valso intentionally blank values (e.g. emptyblogdescription) are not silently droppedcleanup_override_context()to bothreturn falsepaths; previously cache/context cleanup only ran on the happy pathforce_copy_elementor_kit()duplicated the existingbackfill_kit_settings()pipeline added in 2.3.1; moved the one net-new feature (CSS regen via\Elementor\Core\Files\CSS\Post::update()) intobackfill_kit_settings()insteadini_set('memory_limit', '512M')now checkswp_convert_hr_to_bytes()first to avoid lowering an existing higher limit@since 2.x.xplaceholder with@since 2.4.0Verification
Site_Duplicator_Test(16 tests): identical results before/after — no regressionsSite_Template_Switching_*(17 tests): identical results before/after — no regressionsAttribution
Original fix by @kenedytorcatt in #1082. Review refinements in this PR.
Supersedes #1082.
aidevops.sh v3.14.42 plugin for OpenCode v1.14.33 with claude-opus-4-6 spent 8h 12m and 23,197 tokens on this with the user in an interactive session.
Summary by CodeRabbit
Release Notes