fix(site): propagate WP_Error from wp_delete_site() to prevent silent redirect on failure#1125
Conversation
… redirect on failure Site::delete() cast wp_delete_site()'s return to (bool), which made any WP_Error truthy. handle_model_delete_form() then saw true, sent JSON success, and the JS redirected the user even though the deletion had failed — producing a white/blank page or leaving the site intact. Return the WP_Error directly so callers can surface the real failure in the delete modal instead of silently redirecting. Adds regression test confirming WP_Error is propagated (not coerced to true) when wp_delete_site() refuses to remove the requested blog. Resolves t532
📝 WalkthroughWalkthroughThe ChangesError Handling in Site Deletion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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
🤖 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/models/class-site.php`:
- Around line 1777-1783: The catch block in the delete() method currently logs
the throwable and returns false, reintroducing a non-WP_Error failure mode;
change it to return a WP_Error instead by creating a new WP_Error (with a
machine-readable code like 'site_delete_failed'), a translated message including
$e->getMessage(), and any useful data (e.g., 'site_id' => $this->get_id()),
after calling wu_log_add; update the catch block around
wp_delete_site($this->get_id()) so callers can use is_wp_error() uniformly.
🪄 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: 1dc9c749-d9b7-4296-a7c8-972a297d5a87
📒 Files selected for processing (2)
inc/models/class-site.phptests/WP_Ultimo/Models/Site_Test.php
| try { | ||
| $result = (bool) wp_delete_site($this->get_id()); | ||
| $wp_result = wp_delete_site($this->get_id()); | ||
| } catch (\Throwable $e) { | ||
| $result = false; | ||
|
|
||
| wu_log_add('fatal-error', $e->getMessage(), LogLevel::ERROR); | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Return a WP_Error from the exception path too.
This branch reintroduces a generic false failure mode, so callers still cannot handle all delete failures uniformly. Wrapping the caught throwable in a translated WP_Error keeps delete() consistent with the new wp_delete_site() error propagation.
💡 Proposed change
try {
$wp_result = wp_delete_site($this->get_id());
} catch (\Throwable $e) {
wu_log_add('fatal-error', $e->getMessage(), LogLevel::ERROR);
- return false;
+ return new \WP_Error(
+ 'wu_' . $this->model . '_delete_failed',
+ __('Failed to delete the site.', 'ultimate-multisite')
+ );
}As per coding guidelines: 'Use WP_Error for validation and operation failures, not exceptions. Check is_wp_error() on return values.'
🤖 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/models/class-site.php` around lines 1777 - 1783, The catch block in the
delete() method currently logs the throwable and returns false, reintroducing a
non-WP_Error failure mode; change it to return a WP_Error instead by creating a
new WP_Error (with a machine-readable code like 'site_delete_failed'), a
translated message including $e->getMessage(), and any useful data (e.g.,
'site_id' => $this->get_id()), after calling wu_log_add; update the catch block
around wp_delete_site($this->get_id()) so callers can use is_wp_error()
uniformly.
Summary
Changes
Testingvendor/bin/phpunit --filter "Site_Test::test_delete_unsaved_site|Site_Test::test_delete_propagates_wp_error_from_wp_delete_site"Both tests pass. All pre-existing Site_Test / Site_Manager_Test failures are unrelated to this change.
|
|
Performance Test Results Performance test results for b781ce0 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
Site::delete()was castingwp_delete_site()return to(bool), making anyWP_Errortruthy — callers receivedtrueeven when deletion failedhandle_model_delete_form()then sent JSON success, wubox JS redirected the user, and the resulting page appeared blank/white because the site was never actually removedWP_Errordirectly so the delete modal surfaces the real failure instead of silently redirectingChanges
inc/models/class-site.php—Site::delete()now stores the rawwp_delete_site()return, checksis_wp_error()on it, and propagates anyWP_Errorto the caller. On success it still returnstrue; on exception it still returnsfalse.tests/WP_Ultimo/Models/Site_Test.php— addstest_delete_propagates_wp_error_from_wp_delete_sitewhich creates a blog, pre-deletes it from WordPress, then callsSite::delete()and asserts the returnedWP_Erroris not coerced totrue.Testing
vendor/bin/phpunit --filter "Site_Test::test_delete_unsaved_site|Site_Test::test_delete_propagates_wp_error_from_wp_delete_site"Both tests pass. All pre-existing Site_Test / Site_Manager_Test failures are unrelated to this change.
Resolves t532
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests