[4.x] Change tenant storage listeners into jobs#1446
Merged
Conversation
Also move the commented jobs to the JobPipelines and update FilesystemTenancyBootstrapperTest accordingly.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1446 +/- ##
============================================
- Coverage 86.03% 85.84% -0.20%
- Complexity 1156 1163 +7
============================================
Files 184 185 +1
Lines 3381 3397 +16
============================================
+ Hits 2909 2916 +7
- Misses 472 481 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
📝 WalkthroughWalkthroughAdds a queued job to remove tenant storage, updates the existing listener to a deprecated, safer implementation that avoids deleting central storage, adjusts the service provider stub comments to recommend the job pipeline, and updates tests to exercise deletion behavior across Changes
Sequence DiagramsequenceDiagram
participant Event as DeletingTenant Event
participant Pipeline as JobPipeline
participant Job as DeleteTenantStorage Job
participant Config as Config
participant FS as Filesystem
Event->>Pipeline: map $event->tenant -> DeleteTenantStorage
Pipeline->>Job: invoke job (queued or sync)
Job->>Config: read tenancy.filesystem.suffix_storage_path
alt suffix disabled
Job-->>Job: return early (no deletion)
else suffix enabled
Job->>Job: resolve central storage path (tenancy()->central)
Job->>Job: resolve tenant storage path (tenancy()->run)
alt paths equal
Job-->>Job: return early (protect central storage)
else paths differ
Job->>FS: check tenant path is directory
alt exists
Job->>FS: delete tenant directory
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php (1)
188-206: Make suffix assumptions explicit in the new creation test.The assertion on Line 203 assumes default
tenantsuffix behavior. Pinning the relevant config in this test will make it less brittle to future default changes.✅ Suggested test hardening
test('tenant storage gets created when TenantCreated listens to CreateTenantStorage', function() { config([ 'tenancy.bootstrappers' => [ FilesystemTenancyBootstrapper::class, ], + 'tenancy.filesystem.suffix_storage_path' => true, + 'tenancy.filesystem.suffix_base' => 'tenant', ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php` around lines 188 - 206, The test assumes a hard-coded "tenant" suffix when building $tenantStoragePath; make that explicit by pinning the suffix in the test (e.g. add config(['tenancy.storage.suffix' => 'tenant']);) and/or build $tenantStoragePath from the config value instead of the literal string so FilesystemTenancyBootstrapperTest's test('tenant storage gets created when TenantCreated listens to CreateTenantStorage', ...) and the final $this->assertDirectoryExists(...) no longer rely on implicit defaults.
🤖 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/DeleteTenantStorage.php`:
- Around line 25-29: DeleteTenantStorage currently calls
tenancy()->run($this->tenant, fn () => storage_path()) and then
File::deleteDirectory($path) which can remove the central storage when
tenancy.filesystem.suffix_storage_path is false; change the logic in
DeleteTenantStorage so you first resolve both the tenant-specific path and the
central storage path and verify the tenant path is not identical to the central
one (or contains the tenant identifier) before calling File::deleteDirectory;
use tenancy()->run($this->tenant, ...) and storage_path() for comparisons and
bail out / log and skip deletion if the resolved $path equals the default
storage_path() or fails a tenant-specific containment check.
---
Nitpick comments:
In `@tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php`:
- Around line 188-206: The test assumes a hard-coded "tenant" suffix when
building $tenantStoragePath; make that explicit by pinning the suffix in the
test (e.g. add config(['tenancy.storage.suffix' => 'tenant']);) and/or build
$tenantStoragePath from the config value instead of the literal string so
FilesystemTenancyBootstrapperTest's test('tenant storage gets created when
TenantCreated listens to CreateTenantStorage', ...) and the final
$this->assertDirectoryExists(...) no longer rely on implicit defaults.
🪄 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: a284bf55-8fa0-481b-9971-86bdd3e8225d
📒 Files selected for processing (5)
assets/TenancyServiceProvider.stub.phpsrc/Jobs/CreateTenantStorage.phpsrc/Jobs/DeleteTenantStorage.phpsrc/Listeners/DeleteTenantStorage.phptests/Bootstrappers/FilesystemTenancyBootstrapperTest.php
💤 Files with no reviewable changes (1)
- src/Listeners/DeleteTenantStorage.php
Also fix and improve related test.
Contributor
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/DeleteTenantStorage.php`:
- Around line 43-44: The directory deletion currently calls
File::deleteDirectory($path) and ignores its return value, so failures are
silent; update the DeleteTenantStorage job to capture the return from
File::deleteDirectory($path) (when is_dir($path) && ! $tenantPathIsCentral) and
if it returns false throw an exception (e.g., RuntimeException) including $path
in the message so the queue marks the job as failed and can be retried/alerted;
ensure you do this inside the method that performs the deletion (referencing
File::deleteDirectory, $path, and $tenantPathIsCentral).
In `@tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php`:
- Line 203: The test hard-codes '/tenant' when building $tenantStoragePath;
change it to use the configured suffix base instead so it follows tenancy
configuration. Replace the literal '/tenant' with a dynamic value obtained from
config('tenancy.filesystem.suffix_base') (or the same helper used in
production), i.e. build $tenantStoragePath by concatenating $centralStoragePath
with the configured suffix base and $tenant->getTenantKey(); update any related
expectations to use that same config-derived value.
- Around line 195-199: The test currently registers listeners for TenantCreated
using Event::listen with
JobPipeline::make([...])->shouldBeQueued(false)->toListener(), which disables
queuing and therefore doesn't exercise serialization/queued execution order;
change the listeners for both storage and database jobs (e.g., the
JobPipeline::make([...]) entries for CreateTenantStorage and the corresponding
CreateTenantDatabase) to use queued execution (remove or set
shouldBeQueued(true) / omit shouldBeQueued(false)) and then assert the jobs are
dispatched to the queue and executed in the expected order (use the framework's
queue fake / assertions to assert queuing and execution order for TenantCreated
handling).
🪄 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: 899f92bb-5773-4f84-b10c-6703b0e57ea8
📒 Files selected for processing (2)
src/Jobs/DeleteTenantStorage.phptests/Bootstrappers/FilesystemTenancyBootstrapperTest.php
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php (1)
195-199:⚠️ Potential issue | 🟠 MajorQueued JobPipeline path is still not exercised in tests.
Lines 195-199 and 210-214 force
shouldBeQueued(false), so these tests only validate synchronous execution and miss the queue/serialization path that this PR targets.#!/bin/bash # Verify storage job tests are synchronous-only and lack queue assertions rg -n -C2 'CreateTenantStorage|DeleteTenantStorage|shouldBeQueued\(' tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php rg -n -C2 'Queue::fake|Queue::assertPushed|Bus::fake|Bus::assertDispatched' tests/Bootstrappers/FilesystemTenancyBootstrapperTest.phpExpected verification outcome:
- First command shows
shouldBeQueued(false)for both storage job pipelines.- Second command returns no queue-fake/assert coverage in this file.
Also applies to: 210-214
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php` around lines 195 - 199, Tests currently force synchronous execution by calling shouldBeQueued(false) on the Event listener pipelines (JobPipeline::make([...])->shouldBeQueued(false)), so the queue/serialization path (CreateTenantStorage, DeleteTenantStorage) is never exercised; change the tests to cover the queued path by removing or changing shouldBeQueued(false) for the TenantCreated/TenantDeleted listeners and use a queue fake (e.g., Queue::fake()) around the event dispatch, then assert the job was queued (Queue::assertPushed or similar) for CreateTenantStorage and DeleteTenantStorage after dispatching TenantCreated/TenantDeleted events to validate the serialized/queued behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php`:
- Around line 195-199: Tests currently force synchronous execution by calling
shouldBeQueued(false) on the Event listener pipelines
(JobPipeline::make([...])->shouldBeQueued(false)), so the queue/serialization
path (CreateTenantStorage, DeleteTenantStorage) is never exercised; change the
tests to cover the queued path by removing or changing shouldBeQueued(false) for
the TenantCreated/TenantDeleted listeners and use a queue fake (e.g.,
Queue::fake()) around the event dispatch, then assert the job was queued
(Queue::assertPushed or similar) for CreateTenantStorage and DeleteTenantStorage
after dispatching TenantCreated/TenantDeleted events to validate the
serialized/queued behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5bf88f56-0bfa-4b2b-8dd5-223cb4f6d6ab
📒 Files selected for processing (1)
tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Without this, updating Tenancy in existing projects would break TenancyServiceProvider. The logic of the deprecated versions is up-to-date with the changes made in the jobs up until now.
1. Remove the CreateTenantStorage job altogether since as the docblock says the class should be redundant now that FilesystemTenancyBootstrapper creates this path automatically when storage_path suffixing is enabled 2. Remove docblock on the DeleteTenantStorage job - a class's docblock should describe what it does, not how it handles edge cases. Here the former isn't even necessary and the latter is well explained by comments in the implementation. 3. Remove the CreateTenantStorage test following the class's removal, the test would pass on its own even without the job with just tenancy initialization as mentioned above. 4. Slightly improve the structure of the DeleteTenantStorage job and deprecated listener 5. Improve deprecation notices so they include full steps for upgrading to the new approach.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php`:
- Around line 194-201: The test assumes FilesystemTenancyBootstrapper is
disabled but never sets it explicitly, so make the scenario robust by explicitly
disabling that bootstrapper before calling
tenancy()->initialize(Tenant::create()); e.g. adjust the tenancy bootstrappers
config or call the config helper to remove/disable FilesystemTenancyBootstrapper
so the test no longer depends on package defaults and the subsequent expectation
comparing storage_path() values remains valid.
🪄 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: 3dbd449c-f66b-46ee-a4cb-e8439e2e4583
📒 Files selected for processing (5)
assets/TenancyServiceProvider.stub.phpsrc/Jobs/DeleteTenantStorage.phpsrc/Listeners/CreateTenantStorage.phpsrc/Listeners/DeleteTenantStorage.phptests/Bootstrappers/FilesystemTenancyBootstrapperTest.php
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php (1)
194-201:⚠️ Potential issue | 🟡 MinorMake scenario 1 explicit instead of relying on defaults.
Line 197 says FilesystemTenancyBootstrapper is not enabled, but this scenario never disables it explicitly. That makes the assertion fragile if default
tenancy.bootstrapperschanges.Suggested hardening
test('tenant storage gets deleted during tenant deletion when the DeletingTenant pipeline contains DeleteTenantStorage', function() { Event::listen(DeletingTenant::class, JobPipeline::make([DeleteTenantStorage::class])->send(function (DeletingTenant $event) { return $event->tenant; })->shouldBeQueued(false)->toListener() ); $centralStoragePath = storage_path(); + config([ + 'tenancy.bootstrappers' => [], + ]); tenancy()->initialize(Tenant::create());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php` around lines 194 - 201, The test relies on the default bootstrappers and should explicitly disable the FilesystemTenancyBootstrapper for scenario 1: before calling tenancy()->initialize(Tenant::create()), set the tenancy bootstrappers config or call the relevant config helper to ensure FilesystemTenancyBootstrapper is not present (e.g. adjust config('tenancy.bootstrappers') to remove the FilesystemTenancyBootstrapper class), then proceed to capture $centralStoragePath and assert $tenantStoragePath equals it; reference the FilesystemTenancyBootstrapper and tenancy()->initialize(Tenant::create()) to locate where to add this config change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php`:
- Around line 194-201: The test relies on the default bootstrappers and should
explicitly disable the FilesystemTenancyBootstrapper for scenario 1: before
calling tenancy()->initialize(Tenant::create()), set the tenancy bootstrappers
config or call the relevant config helper to ensure
FilesystemTenancyBootstrapper is not present (e.g. adjust
config('tenancy.bootstrappers') to remove the FilesystemTenancyBootstrapper
class), then proceed to capture $centralStoragePath and assert
$tenantStoragePath equals it; reference the FilesystemTenancyBootstrapper and
tenancy()->initialize(Tenant::create()) to locate where to add this config
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 06cd283c-1022-49db-934d-e8d87529232b
📒 Files selected for processing (1)
tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php (1)
194-201:⚠️ Potential issue | 🟡 MinorMake scenario 1 deterministic instead of relying on implicit defaults.
Line [197] states FilesystemTenancyBootstrapper is not enabled, but that state is not explicitly set before Line [195]. If defaults change, this case can fail for the wrong reason.
♻️ Suggested hardening
$centralStoragePath = storage_path(); + config([ + 'tenancy.bootstrappers' => [], + ]); tenancy()->initialize(Tenant::create());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php` around lines 194 - 201, Make the test deterministic by explicitly disabling the FilesystemTenancyBootstrapper before calling tenancy()->initialize(Tenant::create()) so the test does not rely on implicit defaults; in the FilesystemTenancyBootstrapperTest adjust the setup for this scenario to remove or override the filesystem bootstrapper from the active bootstrappers (e.g., update the tenancy bootstrappers config or call the appropriate tenancy API to disable/remove FilesystemTenancyBootstrapper) prior to initialization, then assert that storage_path() for tenant matches centralStoragePath as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php`:
- Around line 194-201: Make the test deterministic by explicitly disabling the
FilesystemTenancyBootstrapper before calling
tenancy()->initialize(Tenant::create()) so the test does not rely on implicit
defaults; in the FilesystemTenancyBootstrapperTest adjust the setup for this
scenario to remove or override the filesystem bootstrapper from the active
bootstrappers (e.g., update the tenancy bootstrappers config or call the
appropriate tenancy API to disable/remove FilesystemTenancyBootstrapper) prior
to initialization, then assert that storage_path() for tenant matches
centralStoragePath as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5669e154-d847-49f1-a287-fa34f574b2b9
📒 Files selected for processing (1)
tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
CreateTenantStorageandDeleteTenantStoragelisteners were used alongside JobPipelines. When theTenantCreatedJobPipeline hadshouldBeQueued(true)and theListeners\CreateTenantStoragewas uncommented, the listener would throw an exception (Stancl\Tenancy\Database\Exceptions\TenantDatabaseDoesNotExistException Database tenantX.sqlite does not exist.) because at the time of executing the listener, the tenant DB wasn't created yet.The same issue could likely also occur in the
DeleteTenantStoragelistener as it usestenancy()->run()to resolve the tenant's storage path which wouldn't work if the tenant's database (or other resources) was already deleted, making initialization impossible.This PR changes
DeleteTenantStorageinto a job and puts it (commented) into the job pipeline, so that it can be queued with the rest of the jobs. It also removesCreateTenantStoragebecause it should be redundant with the FilesystemTenancyBootstrapper creating the same paths automatically when storage path is suffixed.The old classes are kept but deprecated for backwards compatibility.
We've also added some edge case hardening to
DeleteTenantStorageto make sure it never deletes the central storage path directory, which previously could in theory occur due to a misconfiguration if a user enabled this job/listener but disabled storage path suffixing.Summary by CodeRabbit
New Features
Improvements
Tests
Deprecations