Change tenant storage listeners into jobs (#1446)

The `CreateTenantStorage` and `DeleteTenantStorage` listeners were used
alongside JobPipelines. When the `TenantCreated` JobPipeline had
`shouldBeQueued(true)` and the `Listeners\CreateTenantStorage` was
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 `DeleteTenantStorage`
listener as it uses `tenancy()->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 `DeleteTenantStorage` into a job and puts it (commented)
into the job pipeline, so that it can be queued with the rest of the
jobs. It also removes `CreateTenantStorage` because 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 `DeleteTenantStorage` to
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.

Co-authored-by: Samuel Štancl <samuel@archte.ch>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

assets/TenancyServiceProvider.stub.php

@@ -53,8 +53,6 @@ class TenancyServiceProvider extends ServiceProvider
                 ])->send(function (Events\TenantCreated $event) {
                     return $event->tenant;
                 })->shouldBeQueued(false),
-
-                // Listeners\CreateTenantStorage::class,
             ],
             Events\SavingTenant::class => [],
             Events\TenantSaved::class => [],
@@ -63,12 +61,11 @@ class TenancyServiceProvider extends ServiceProvider
             Events\DeletingTenant::class => [
                 JobPipeline::make([
                     Jobs\DeleteDomains::class,
+                    // Jobs\DeleteTenantStorage::class,
                     // Jobs\RemoveStorageSymlinks::class,
                 ])->send(function (Events\DeletingTenant $event) {
                     return $event->tenant;
                 })->shouldBeQueued(false),
-
-                // Listeners\DeleteTenantStorage::class,
             ],
             Events\TenantDeleted::class => [
                 JobPipeline::make([

src/Jobs/DeleteTenantStorage.php

@@ -0,0 +1,43 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Stancl\Tenancy\Jobs;
+
+use Illuminate\Bus\Queueable;
+use Illuminate\Contracts\Queue\ShouldQueue;
+use Illuminate\Foundation\Bus\Dispatchable;
+use Illuminate\Queue\InteractsWithQueue;
+use Illuminate\Queue\SerializesModels;
+use Illuminate\Support\Facades\File;
+use Stancl\Tenancy\Contracts\Tenant;
+
+class DeleteTenantStorage implements ShouldQueue
+{
+    use Dispatchable, InteractsWithQueue, Queueable, SerializesModels;
+
+    public function __construct(
+        public Tenant $tenant,
+    ) {}
+
+    public function handle(): void
+    {
+        if (config('tenancy.filesystem.suffix_storage_path') === false) {
+            // Skip storage deletion if path suffixing is disabled
+            return;
+        }
+
+        $centralStoragePath = tenancy()->central(fn () => storage_path());
+        $tenantStoragePath = tenancy()->run($this->tenant, fn () => storage_path());
+
+        if ($tenantStoragePath === $centralStoragePath) {
+            // Check again to ensure the tenant storage path is distinct from the central storage path
+            // to avoid any accidental central storage path deletion
+            return;
+        }
+
+        if (is_dir($tenantStoragePath)) {
+            File::deleteDirectory($tenantStoragePath);
+        }
+    }
+}

src/Listeners/CreateTenantStorage.php

@@ -7,11 +7,7 @@ namespace Stancl\Tenancy\Listeners;
 use Stancl\Tenancy\Events\Contracts\TenantEvent;
 
 /**
- * Can be used to manually create framework directories in the tenant storage when storage_path() is scoped.
+ * @deprecated FilesystemTenancyBootstrapper creates the path automatically when suffix_storage_path is enabled.
- *
- * Useful when using real-time facades which use the framework/cache directory.
- *
- * Generally not needed anymore as the directory is also created by the FilesystemTenancyBootstrapper.
  */
 class CreateTenantStorage
 {

src/Listeners/DeleteTenantStorage.php

@@ -7,14 +7,29 @@ namespace Stancl\Tenancy\Listeners;
 use Illuminate\Support\Facades\File;
 use Stancl\Tenancy\Events\Contracts\TenantEvent;
 
+/**
+ * @deprecated Use Stancl\Tenancy\Jobs\DeleteTenantStorage in a job pipeline instead.
+ */
 class DeleteTenantStorage
 {
     public function handle(TenantEvent $event): void
     {
-        $path = tenancy()->run($event->tenant, fn () => storage_path());
+        if (config('tenancy.filesystem.suffix_storage_path') === false) {
+            // Skip storage deletion if path suffixing is disabled
+            return;
+        }
 
-        if (is_dir($path)) {
+        $centralStoragePath = tenancy()->central(fn () => storage_path());
-            File::deleteDirectory($path);
+        $tenantStoragePath = tenancy()->run($event->tenant, fn () => storage_path());
+
+        if ($tenantStoragePath === $centralStoragePath) {
+            // Check again to ensure the tenant storage path is distinct from the central storage path
+            // to avoid any accidental central storage path deletion
+            return;
+        }
+
+        if (is_dir($tenantStoragePath)) {
+            File::deleteDirectory($tenantStoragePath);
         }
     }
 }

tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php

@@ -13,7 +13,7 @@ use Stancl\Tenancy\Events\TenancyInitialized;
 use Stancl\Tenancy\Jobs\CreateStorageSymlinks;
 use Stancl\Tenancy\Jobs\RemoveStorageSymlinks;
 use Stancl\Tenancy\Listeners\BootstrapTenancy;
-use Stancl\Tenancy\Listeners\DeleteTenantStorage;
+use Stancl\Tenancy\Jobs\DeleteTenantStorage;
 use Stancl\Tenancy\Listeners\RevertToCentralContext;
 use Stancl\Tenancy\Bootstrappers\FilesystemTenancyBootstrapper;
 use function Stancl\Tenancy\Tests\pest;
@@ -184,21 +184,63 @@ test('create and delete storage symlinks jobs work', function() {
     $this->assertDirectoryDoesNotExist(public_path("public-$tenantKey"));
 });
 
-test('tenant storage can get deleted after the tenant when DeletingTenant listens to DeleteTenantStorage', function() {
+test('tenant storage gets deleted during tenant deletion when the DeletingTenant pipeline contains DeleteTenantStorage', function() {
-    Event::listen(DeletingTenant::class, DeleteTenantStorage::class);
+    Event::listen(DeletingTenant::class,
+        JobPipeline::make([DeleteTenantStorage::class])->send(function (DeletingTenant $event) {
+            return $event->tenant;
+        })->shouldBeQueued(false)->toListener()
+    );
+
+    $centralStoragePath = storage_path();
+    tenancy()->initialize(Tenant::create());
+
+    // FilesystemTenancyBootstrapper not enabled,
+    // tenant and central storage path is the same,
+    // the storage deletion will be skipped.
+    $tenantStoragePath = storage_path();
+    expect($tenantStoragePath)->toBe($centralStoragePath);
+    expect(File::isDirectory($centralStoragePath))->toBeTrue();
+    tenant()->delete();
+
+    expect(File::isDirectory($centralStoragePath))->toBeTrue();
+
+    config([
+        'tenancy.bootstrappers' => [FilesystemTenancyBootstrapper::class],
+        'tenancy.filesystem.suffix_storage_path' => false,
+    ]);
+
+    tenancy()->initialize(Tenant::create());
+
+    $tenantStoragePath = storage_path();
+
+    // FilesystemTenancyBootstrapper enabled,
+    // but tenant and central storage path is still the same
+    // because suffix_storage_path is false.
+    // The storage deletion will be skipped.
+    expect($tenantStoragePath)->toBe($centralStoragePath);
+    expect(File::isDirectory($centralStoragePath))->toBeTrue();
+    tenant()->delete();
+
+    expect(File::isDirectory($centralStoragePath))->toBeTrue();
+
+    config([
+        'tenancy.bootstrappers' => [FilesystemTenancyBootstrapper::class],
+        'tenancy.filesystem.suffix_storage_path' => true,
+    ]);
 
     tenancy()->initialize(Tenant::create());
     $tenantStoragePath = storage_path();
 
-    Storage::fake('test');
+    // FilesystemTenancyBootstrapper enabled,
-
+    // suffix_storage_path enabled, so the two paths are distinct.
+    // Tenant storage will be deleted.
+    expect($tenantStoragePath)->not()->toBe($centralStoragePath);
     expect(File::isDirectory($tenantStoragePath))->toBeTrue();
 
-    Storage::put('test.txt', 'testing file');
-
     tenant()->delete();
 
     expect(File::isDirectory($tenantStoragePath))->toBeFalse();
+    expect(File::isDirectory($centralStoragePath))->toBeTrue();
 });
 
 test('the framework/cache directory is created when storage_path is scoped', function (bool $suffixStoragePath) {
@@ -256,4 +298,3 @@ test('scoped disks are scoped per tenant', function () {
     expect(file_get_contents(storage_path() . "/app/public/scoped_disk_prefix/foo.txt"))->toBe('central2');
     expect(file_get_contents(storage_path() . "/tenant{$tenant->id}/app/public/scoped_disk_prefix/foo.txt"))->toBe('tenant');
 });
-