[4.x] Change tenant storage listeners into jobs#1446
Draft
[4.x] Change tenant storage listeners into jobs#1446
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.86% -0.18%
- Complexity 1156 1166 +10
============================================
Files 184 186 +2
Lines 3381 3402 +21
============================================
+ Hits 2909 2921 +12
- Misses 472 481 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughSynchronous tenant storage listeners were replaced by queued jobs: Changes
Sequence Diagram(s)sequenceDiagram
participant Event as Event Dispatcher
participant Pipeline as JobPipeline / Queue
participant Job as Create/Delete Tenant Job
participant Tenancy as tenancy() context
participant FS as Filesystem
Event->>Pipeline: Fire TenantCreated (maps tenant, enqueue CreateTenantStorage)
Pipeline->>Job: Dequeue & execute CreateTenantStorage
Job->>Tenancy: tenancy()->run($tenant, callback)
Tenancy->>FS: resolve storage_path(), create dirs
FS-->>Tenancy: dirs created
Job-->>Pipeline: complete
Event->>Pipeline: Fire DeletingTenant (maps tenant, enqueue DeleteTenantStorage)
Pipeline->>Job: Dequeue & execute DeleteTenantStorage
Job->>Tenancy: tenancy()->central(...) & tenancy()->run($tenant,...)
Tenancy->>FS: resolve central & tenant paths
Job->>FS: compare realpaths, check dir
FS->>FS: deleteDirectory(path) if safe
Job-->>Pipeline: complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![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
🧹 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.
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>
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 |
✅ Actions performedReviews resumed. |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai full review |
✅ 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.
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.This PR changes
CreateTenantStorageandDeleteTenantStorageinto jobs and puts these commented jobs into the JobPipelines, so that they can be queued with the rest of the jobs.Summary by CodeRabbit