[4.x] Skip database deletion when create_database = false, add ignoreFailures option#1394
[4.x] Skip database deletion when create_database = false, add ignoreFailures option#1394ju5t wants to merge 3 commits intoarchtechx:masterfrom
create_database = false, add ignoreFailures option#1394Conversation
ju5t
changed the title
ju5t
changed the title
create_database exists
stancl
changed the title
create_database existscreate_database = false
📝 WalkthroughWalkthroughAdded two static configuration flags to the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1394 +/- ##
============================================
- Coverage 86.08% 85.98% -0.10%
- Complexity 1154 1158 +4
============================================
Files 184 184
Lines 3377 3382 +5
============================================
+ Hits 2907 2908 +1
- Misses 470 474 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
stancl
changed the title
create_database = falsecreate_database = false, add ignoreFailures option
There was a problem hiding this comment.
Pull request overview
Adjusts tenant database deletion behavior in the tenancy job pipeline to better handle tenants that were created without a dedicated database, and adds an opt-in “continue on deletion failure” mode.
Changes:
- Adds a default skip for database deletion when the tenant’s internal
create_databaseflag is explicitlyfalse. - Introduces an opt-in static flag to ignore database deletion exceptions and continue the delete pipeline.
- Wraps deletion in conditional logic + try/catch.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (static::$skipWhenCreateDatabaseIsFalse && $this->tenant->getInternal('create_database') === false) { | ||
| // If database creation was skipped, we presume deletion should also be skipped. |
There was a problem hiding this comment.
The PR description says deletion is skipped when the tenant has create_database set to true, but this implementation skips only when getInternal('create_database') === false. Please align the PR description with the actual behavior (or adjust the condition if the description is the intended behavior).
| try { | ||
| $this->tenant->database()->manager()->deleteDatabase($this->tenant); | ||
| } catch (\Throwable $e) { | ||
| if (! static::$ignoreFailures) { | ||
| throw $e; | ||
| } | ||
| } | ||
|
|
||
| event(new DatabaseDeleted($this->tenant)); |
There was a problem hiding this comment.
When $ignoreFailures is enabled, an exception from deleteDatabase() is swallowed but DatabaseDeleted is still dispatched afterwards. That event then signals a successful deletion even though the database may still exist. Consider only dispatching DatabaseDeleted when deletion actually succeeded (and optionally emitting/logging a separate failure signal when exceptions are ignored).
| try { | |
| $this->tenant->database()->manager()->deleteDatabase($this->tenant); | |
| } catch (\Throwable $e) { | |
| if (! static::$ignoreFailures) { | |
| throw $e; | |
| } | |
| } | |
| event(new DatabaseDeleted($this->tenant)); | |
| $deleted = false; | |
| try { | |
| $this->tenant->database()->manager()->deleteDatabase($this->tenant); | |
| $deleted = true; | |
| } catch (\Throwable $e) { | |
| if (! static::$ignoreFailures) { | |
| throw $e; | |
| } | |
| } | |
| if ($deleted) { | |
| event(new DatabaseDeleted($this->tenant)); | |
| } |
| } catch (\Throwable $e) { | ||
| if (! static::$ignoreFailures) { | ||
| throw $e; | ||
| } |
There was a problem hiding this comment.
Catching \Throwable will also swallow PHP Errors (type errors, parse errors, etc.) when $ignoreFailures is true, which can hide real programming bugs and make workers continue in a broken state. Prefer catching \Exception (or rethrow \Error), and consider at least reporting/logging the exception when it’s intentionally ignored.
| } catch (\Throwable $e) { | |
| if (! static::$ignoreFailures) { | |
| throw $e; | |
| } | |
| } catch (\Exception $e) { | |
| if (! static::$ignoreFailures) { | |
| throw $e; | |
| } | |
| report($e); |
| if (static::$skipWhenCreateDatabaseIsFalse && $this->tenant->getInternal('create_database') === false) { | ||
| // If database creation was skipped, we presume deletion should also be skipped. | ||
| // To avoid this skip, either unset the `create_database` attribute (or make it true), or | ||
| // set the $skipWhenCreateDatabaseIsFalse static property to false. | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| $this->tenant->database()->manager()->deleteDatabase($this->tenant); | ||
| } catch (\Throwable $e) { | ||
| if (! static::$ignoreFailures) { | ||
| throw $e; | ||
| } | ||
| } |
There was a problem hiding this comment.
The new behaviors (skipping deletion when create_database is false, and the $ignoreFailures path) don’t appear to be covered by tests. Please add coverage to verify (1) deletion is skipped when create_database is explicitly false, and (2) the expected events/side-effects when failures are ignored vs rethrown.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Jobs/DeleteDatabase.php`:
- Around line 33-40: The DeletingDatabase event is dispatched before the
early-exit guard, causing a “deleting” event to be emitted even when deletion is
skipped; move the event(...) call so it occurs only after the guard check that
uses static::$skipWhenCreateDatabaseIsFalse and
$this->tenant->getInternal('create_database') === false, i.e., perform the skip
check first and only call event(new DeletingDatabase($this->tenant)) when
deletion will actually proceed.
- Around line 42-50: The DatabaseDeleted event is dispatched even when
deleteDatabase() fails and errors are suppressed; modify the DeleteDatabase job
so the event is only fired on actual success: attempt
$this->tenant->database()->manager()->deleteDatabase($this->tenant) inside the
try, set a success flag or move the event dispatch into the try block, rethrow
the exception when static::$ignoreFailures is false (as currently done), and
only call event(new DatabaseDeleted($this->tenant)) when the deletion completed
without throwing.
- Around line 44-47: In DeleteDatabase job replace the catch block catching
\Throwable with one catching \Exception to match project convention; locate the
try/catch inside the DeleteDatabase class (method that performs the DB delete)
and change "catch (\Throwable $e)" to "catch (\Exception $e)" while preserving
the existing logic that rethrows when static::$ignoreFailures is false and
otherwise continues to handle failures the same way.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cd9ada02-86eb-4ee0-8e4a-b0f3781b3e83
📒 Files selected for processing (1)
src/Jobs/DeleteDatabase.php
| event(new DeletingDatabase($this->tenant)); | ||
|
|
||
| $this->tenant->database()->manager()->deleteDatabase($this->tenant); | ||
| if (static::$skipWhenCreateDatabaseIsFalse && $this->tenant->getInternal('create_database') === false) { | ||
| // If database creation was skipped, we presume deletion should also be skipped. | ||
| // To avoid this skip, either unset the `create_database` attribute (or make it true), or | ||
| // set the $skipWhenCreateDatabaseIsFalse static property to false. | ||
| return; | ||
| } |
There was a problem hiding this comment.
Emit DeletingDatabase only when deletion will actually run.
Line 33 dispatches DeletingDatabase before the skip guard, so Line 39 can return after broadcasting a “deleting” signal without attempting deletion.
Proposed fix
- event(new DeletingDatabase($this->tenant));
-
if (static::$skipWhenCreateDatabaseIsFalse && $this->tenant->getInternal('create_database') === false) {
// If database creation was skipped, we presume deletion should also be skipped.
// To avoid this skip, either unset the `create_database` attribute (or make it true), or
// set the $skipWhenCreateDatabaseIsFalse static property to false.
return;
}
+
+ event(new DeletingDatabase($this->tenant));📝 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.
| event(new DeletingDatabase($this->tenant)); | |
| $this->tenant->database()->manager()->deleteDatabase($this->tenant); | |
| if (static::$skipWhenCreateDatabaseIsFalse && $this->tenant->getInternal('create_database') === false) { | |
| // If database creation was skipped, we presume deletion should also be skipped. | |
| // To avoid this skip, either unset the `create_database` attribute (or make it true), or | |
| // set the $skipWhenCreateDatabaseIsFalse static property to false. | |
| return; | |
| } | |
| if (static::$skipWhenCreateDatabaseIsFalse && $this->tenant->getInternal('create_database') === false) { | |
| // If database creation was skipped, we presume deletion should also be skipped. | |
| // To avoid this skip, either unset the `create_database` attribute (or make it true), or | |
| // set the $skipWhenCreateDatabaseIsFalse static property to false. | |
| return; | |
| } | |
| event(new DeletingDatabase($this->tenant)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Jobs/DeleteDatabase.php` around lines 33 - 40, The DeletingDatabase event
is dispatched before the early-exit guard, causing a “deleting” event to be
emitted even when deletion is skipped; move the event(...) call so it occurs
only after the guard check that uses static::$skipWhenCreateDatabaseIsFalse and
$this->tenant->getInternal('create_database') === false, i.e., perform the skip
check first and only call event(new DeletingDatabase($this->tenant)) when
deletion will actually proceed.
| try { | ||
| $this->tenant->database()->manager()->deleteDatabase($this->tenant); | ||
| } catch (\Throwable $e) { | ||
| if (! static::$ignoreFailures) { | ||
| throw $e; | ||
| } | ||
| } | ||
|
|
||
| event(new DatabaseDeleted($this->tenant)); |
There was a problem hiding this comment.
Gate DatabaseDeleted on actual deletion success.
With ignoreFailures = true, a thrown deletion error is suppressed but Line 50 still emits DatabaseDeleted. That event should only be dispatched when deleteDatabase() succeeded.
Proposed fix
+ $deleted = false;
try {
$this->tenant->database()->manager()->deleteDatabase($this->tenant);
+ $deleted = true;
} catch (\Throwable $e) {
if (! static::$ignoreFailures) {
throw $e;
}
}
-
- event(new DatabaseDeleted($this->tenant));
+ if ($deleted) {
+ event(new DatabaseDeleted($this->tenant));
+ }📝 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.
| try { | |
| $this->tenant->database()->manager()->deleteDatabase($this->tenant); | |
| } catch (\Throwable $e) { | |
| if (! static::$ignoreFailures) { | |
| throw $e; | |
| } | |
| } | |
| event(new DatabaseDeleted($this->tenant)); | |
| $deleted = false; | |
| try { | |
| $this->tenant->database()->manager()->deleteDatabase($this->tenant); | |
| $deleted = true; | |
| } catch (\Throwable $e) { | |
| if (! static::$ignoreFailures) { | |
| throw $e; | |
| } | |
| } | |
| if ($deleted) { | |
| event(new DatabaseDeleted($this->tenant)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Jobs/DeleteDatabase.php` around lines 42 - 50, The DatabaseDeleted event
is dispatched even when deleteDatabase() fails and errors are suppressed; modify
the DeleteDatabase job so the event is only fired on actual success: attempt
$this->tenant->database()->manager()->deleteDatabase($this->tenant) inside the
try, set a success flag or move the event dispatch into the try block, rethrow
the exception when static::$ignoreFailures is false (as currently done), and
only call event(new DatabaseDeleted($this->tenant)) when the deletion completed
without throwing.
| } catch (\Throwable $e) { | ||
| if (! static::$ignoreFailures) { | ||
| throw $e; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify local convention and throw/catch patterns around database deletion paths.
# 1) Compare catch types used in jobs/features.
rg -n -C2 'catch\s*\(\\(Throwable|Exception)\s+\$[A-Za-z_][A-Za-z0-9_]*\)' src
# 2) Inspect deleteDatabase declarations/implementations and nearby throws.
rg -n -C4 'function\s+deleteDatabase\s*\(' src
rg -n -C3 'throw\s+new\s+\\?(Error|TypeError|Exception|RuntimeException|LogicException)' srcRepository: archtechx/tenancy
Length of output: 16254
🏁 Script executed:
#!/bin/bash
# Check what exceptions are thrown by database connection methods
# and if Error classes are used in the database layer
# 1) Search for Error throws or usage in database managers
rg -n '\\Error|\\TypeError' src/Database --type php
# 2) Check parent deleteDatabase class chain
ast-grep --pattern $'class $_ {
$$$
deleteDatabase($_, $_) {
$$$
}
$$$
}'
# 3) Confirm statement() calls are used and what they throw (Laravel convention)
rg -n 'statement\(' src/Database/TenantDatabaseManagers --type php -A1
# 4) Verify exception hierarchy - any Throwable-only catch blocks
rg -n 'catch.*\\Throwable' src --type php -B2 -A2Repository: archtechx/tenancy
Length of output: 7611
Change catch (\Throwable $e) to catch (\Exception $e) to align with package convention.
This codebase consistently uses generic \Exception for feature-level errors across jobs, bootstrappers, and concerns. Catching \Throwable deviates from this established pattern and is unnecessary since database operations only throw exception subclasses, not engine errors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Jobs/DeleteDatabase.php` around lines 44 - 47, In DeleteDatabase job
replace the catch block catching \Throwable with one catching \Exception to
match project convention; locate the try/catch inside the DeleteDatabase class
(method that performs the DB delete) and change "catch (\Throwable $e)" to
"catch (\Exception $e)" while preserving the existing logic that rethrows when
static::$ignoreFailures is false and otherwise continues to handle failures the
same way.
Database deletion is now skipped by default if the tenant has the
create_databaseinternal attribute set to true, meaning it was likely created without a database. This skip can be opted out of by changing a static property.It also adds an opt-in static property for ignoring any other failures during database deletion, to allow continuing execution of the delete pipeline.
Summary by CodeRabbit