From 04ab6c85c7e6663444641341eab221a0b0206396 Mon Sep 17 00:00:00 2001 From: lukinovec Date: Thu, 30 Jul 2026 15:38:56 +0200 Subject: [PATCH 01/12] Add tests for scopeCache() path/lock_path handling The tests cover the current (mostly incorrect) scopeCache() behavior (= hardcoding the /framework/cache/data path regardless of what was configured). The 'file cache stores are separated per tenant' is not a regression test -- it covers the default path, which already worked correctly, there were just no tests for it. The rest are regression tests (see the "NOTE ABOUT REGRESSION" comments -- these are temporary, added them just so that it's clear what's currently wrong or broken) that should be fixed by the FS bootstrapper fix in the next commit. --- .../FilesystemTenancyBootstrapperTest.php | 249 ++++++++++++++++++ 1 file changed, 249 insertions(+) diff --git a/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php b/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php index 4e834917..73352dcd 100644 --- a/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php +++ b/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php @@ -1,6 +1,7 @@ toBe('central2'); expect(file_get_contents(storage_path() . "/tenant{$tenant->id}/app/public/scoped_disk_prefix/foo.txt"))->toBe('tenant'); }); + +test('file cache stores are separated per tenant', function () { + // NOTE ABOUT REGRESSION: this is not a regression test, + // this just covers what wasn't covered before. + config([ + 'tenancy.bootstrappers' => [ + FilesystemTenancyBootstrapper::class, + ], + 'tenancy.cache.stores' => ['file'], + 'cache.stores.file' => [ + 'driver' => 'file', + 'path' => storage_path('framework/cache/data'), + 'lock_path' => storage_path('framework/cache/data'), + ], + ]); + + $tenant1 = Tenant::create(); + $tenant2 = Tenant::create(); + + Cache::store('file')->put('key', 'central'); + + tenancy()->initialize($tenant1); + + expect(Cache::store('file')->get('key'))->toBeNull(); + Cache::store('file')->put('key', 'tenant1'); + + tenancy()->initialize($tenant2); + + expect(Cache::store('file')->get('key'))->toBeNull(); + Cache::store('file')->put('key', 'tenant2'); + + tenancy()->initialize($tenant1); + + expect(Cache::store('file')->get('key'))->toBe('tenant1'); + + tenancy()->end(); + + expect(Cache::store('file')->get('key'))->toBe('central'); +}); + +test('central cache is not lost when tenancy ends', function () { + $path = storage_path('framework/cache/foo_file'); + File::deleteDirectory($path); + + // Use a separate 'foo' store rather than reconfiguring 'file'. + // TestCase::setUp() calls `cache:clear file`, which resolves the 'file' store and leaves it + // in the CacheManager with the default path. A later config() call can't mutate that, so a test + // reconfiguring 'file' would keep using storage/framework/cache/data and pass either way. + // The same applies to the other tests below that configure separate cache stores rather than using 'file'. + config([ + 'tenancy.bootstrappers' => [ + FilesystemTenancyBootstrapper::class, + ], + 'tenancy.cache.stores' => ['foo_file'], + 'cache.stores.foo_file' => [ + 'driver' => 'file', + 'path' => $path, + 'lock_path' => $path, + ], + ]); + + $tenant = Tenant::create(); + + Cache::store('foo_file')->put('foo', 'central'); + + // Just initialize and revert tenancy to trigger FilesystemTenancyBootstrapper + tenancy()->initialize($tenant); + tenancy()->end(); + + // Nothing deleted the 'foo' entry, so its value should stay 'central' even after reverting tenancy. + // NOTE ABOUT REGRESSION: revert() sets the store to the hardcoded storage/framework/cache/data instead of + // setting it to whatever it was before initializing tenancy. So the central 'foo' entry is null. + expect(Cache::store('foo_file')->get('foo'))->toBe('central'); + + File::deleteDirectory($path); +}); + +test('file cache stores with different configured paths do not share a directory', function () { + $fooPath = storage_path('framework/cache/foo'); + $barPath = storage_path('framework/cache/bar'); + + File::deleteDirectory($fooPath); + File::deleteDirectory($barPath); + + config([ + 'tenancy.bootstrappers' => [ + FilesystemTenancyBootstrapper::class, + ], + 'tenancy.cache.stores' => ['foo_file', 'bar_file'], + 'cache.stores.foo_file' => [ + 'driver' => 'file', + 'path' => $fooPath, + 'lock_path' => $fooPath, + ], + 'cache.stores.bar_file' => [ + 'driver' => 'file', + 'path' => $barPath, + 'lock_path' => $barPath, + ], + ]); + + tenancy()->initialize(Tenant::create()); + + Cache::store('foo_file')->put('key', 'foo'); + Cache::store('bar_file')->put('key', 'bar'); + + // NOTE ABOUT REGRESSION: both stores are pointed at the same path, + // so they overwrite and read each other's entries. + // For the same reason, flushing one of them empties the other. + expect(Cache::store('foo_file')->get('key'))->toBe('foo'); + expect(Cache::store('bar_file')->get('key'))->toBe('bar'); + + Cache::store('bar_file')->flush(); + + expect(Cache::store('foo_file')->get('key'))->toBe('foo'); + + tenancy()->end(); + + File::deleteDirectory($fooPath); + File::deleteDirectory($barPath); +}); + +test('a configured lock_path is scoped separately from path', function () { + // A store can configure path and lock_path as two different directories. + // Locks should use lock_path IF CONFIGURED, not fall back into the same directory as path. + $centralStoragePath = storage_path(); + $path = "{$centralStoragePath}/framework/cache/foo"; + $lockPath = "{$centralStoragePath}/framework/cache/foo_locks"; + + // Delete the directories before running assertions, not just after. + // Cache::lock() below defaults to a lock that never expires, + // and this test never releases it, so a stale lock left over from + // a previous run would make lock() fail forever. + File::deleteDirectory($path); + File::deleteDirectory($lockPath); + + config([ + 'tenancy.bootstrappers' => [ + FilesystemTenancyBootstrapper::class, + ], + 'tenancy.cache.stores' => ['foo_file'], + 'cache.stores.foo_file' => [ + 'driver' => 'file', + 'path' => $path, + 'lock_path' => $lockPath, + ], + ]); + + $tenant = Tenant::create(); + tenancy()->initialize($tenant); + + expect(Cache::store('foo_file')->lock('foo')->get())->toBeTrue(); + + $tenantPath = "{$centralStoragePath}/tenant{$tenant->id}/framework/cache/foo"; + $tenantLockPath = "{$centralStoragePath}/tenant{$tenant->id}/framework/cache/foo_locks"; + + // NOTE ABOUT REGRESSION: path and lock_path both get hardcoded to the same default directory + // instead of the ones configured here, so this lock directory never gets created. + expect(File::isDirectory($tenantLockPath))->toBeTrue(); + expect(File::isDirectory($tenantPath))->toBeFalse(); + + tenancy()->end(); + + expect(Cache::store('foo_file')->lock('foo')->get())->toBeTrue(); + + // NOTE ABOUT REGRESSION: revert() has the same hardcoding bug, so lock_path is never restored + // to what it was configured with either. + expect(File::isDirectory($lockPath))->toBeTrue(); + + File::deleteDirectory($path); + File::deleteDirectory($lockPath); +}); + +test('a cache store without a configured lock_path is scoped without error', function () { + // lock_path is optional -- Laravel falls back to using path for locks when it's not set. + // This test covers that path through scopeCache() specifically (which none of the other tests exercise). + $path = storage_path('framework/cache/foo'); + File::deleteDirectory($path); + + config([ + 'tenancy.bootstrappers' => [ + FilesystemTenancyBootstrapper::class, + ], + 'tenancy.cache.stores' => ['foo_file'], + 'cache.stores.foo_file' => [ + 'driver' => 'file', + 'path' => $path, + // No 'lock_path' key at all. + ], + ]); + + tenancy()->initialize(Tenant::create()); + + expect(Cache::store('foo_file')->put('key', 'tenant'))->toBeTrue(); + expect(Cache::store('foo_file')->lock('foo')->get())->toBeTrue(); + + tenancy()->end(); + + expect(Cache::store('foo_file')->put('key', 'central'))->toBeTrue(); + expect(Cache::store('foo_file')->lock('foo')->get())->toBeTrue(); + + File::deleteDirectory($path); +}); + +test('a cache store using a path not based on storage_path() is suffixed in place, not moved under the tenant storage path', function () { + // $path below doesn't start with the central storage path, so scopeCachePath() has no storage + // path prefix to swap for the tenant's -- it appends the tenant suffix directly to $path instead. + // Check that tenant isolation still works for a store configured like this, and that its cache + // ends up at $path itself, not under the tenant's storage path. + $path = '/tmp/tenancy-cache-test'; + File::deleteDirectory($path); + + config([ + 'tenancy.bootstrappers' => [ + FilesystemTenancyBootstrapper::class, + ], + 'tenancy.cache.stores' => ['foo_file'], + 'cache.stores.foo_file' => [ + 'driver' => 'file', + 'path' => $path, + 'lock_path' => $path, + ], + ]); + + $tenant1 = Tenant::create(); + $tenant2 = Tenant::create(); + + tenancy()->initialize($tenant1); + Cache::store('foo_file')->put('key', 'tenant1'); + + tenancy()->initialize($tenant2); + expect(Cache::store('foo_file')->get('key'))->toBeNull(); + + tenancy()->initialize($tenant1); + expect(Cache::store('foo_file')->get('key'))->toBe('tenant1'); + + expect(File::isDirectory("{$path}/tenant{$tenant1->id}"))->toBeTrue(); + + // NOTE ABOUT REGRESSION: storage_path() is already scoped to tenant1 here (storagePath() runs + // before scopeCache() in bootstrap()), so this is the tenant's default cache directory + // the code points every store's cache at, regardless of its configured path. It should stay + // empty here, since foo_file is configured with its own $path that's not storage_path()-based. + expect(File::isDirectory(storage_path('framework/cache/data')))->toBeFalse(); + + tenancy()->end(); + + File::deleteDirectory($path); +}); From 483a3ec802e14521fd16c69b2a14476adc3fef51 Mon Sep 17 00:00:00 2001 From: lukinovec Date: Thu, 30 Jul 2026 15:57:40 +0200 Subject: [PATCH 02/12] Fix scopeCache() discarding configured cache store paths scopeCache() rewrote path and lock_path for every file-driver store to a hardcoded '/framework/cache/data' path, completely ignoring the store's config. Now, scopeCache() remembers each store's original path and lock_path, scopes these paths for the tenant, and restores them to the stored originals on revert. The store's lock_path was always overwritten by the same hardcoded path. But lock_path is configurable too, AND it's actually optional (unlike path). If it's not configured at all (= it's null or just unset), Laravel automatically falls back to the store's path. So in that case, leave lock_path null instead of assigning the path to it. This is not a *huge* change, assigning path to lock_path would essentially achieve the same thing, BUT if someone explicitly sets lock_path to null in the config, we should just respect that and let Laravel fall back to the path instead of setting the lock_path ourselves. Also, on revert(), the same hardcoded path was used in scopeCache(). So if someone used a custom file driver-based store, cached something in central context, initialized and ended tenancy, the central cache got corrupt (see the 'central cache is not lost when tenancy ends' test). --- .../FilesystemTenancyBootstrapper.php | 70 +++++++++++++++++-- 1 file changed, 63 insertions(+), 7 deletions(-) diff --git a/src/Bootstrappers/FilesystemTenancyBootstrapper.php b/src/Bootstrappers/FilesystemTenancyBootstrapper.php index 0a864fbd..c573db8f 100644 --- a/src/Bootstrappers/FilesystemTenancyBootstrapper.php +++ b/src/Bootstrappers/FilesystemTenancyBootstrapper.php @@ -14,6 +14,21 @@ use Stancl\Tenancy\Contracts\Tenant; class FilesystemTenancyBootstrapper implements TenancyBootstrapper { public array $originalDisks = []; + + /** + * The path and lock_path each file cache store had in the central context, keyed by store name. + * + * For example: + * [ + * 'file' => [ + * 'path' => storage_path('framework/cache/data'), + * 'lock_path' => storage_path('framework/cache/data'), + * ], + * ] + * + * Used to scope the store to a tenant, and to restore it back to this when tenancy ends. + */ + protected array $originalCachePaths = []; public string|null $originalAssetUrl; public string $originalStoragePath; @@ -191,10 +206,6 @@ class FilesystemTenancyBootstrapper implements TenancyBootstrapper return; } - $storagePath = $suffix - ? $this->tenantStoragePath($suffix) - : $this->originalStoragePath; - $stores = array_filter($this->app['config']['tenancy.cache.stores'], function ($name) { $store = $this->app['config']["cache.stores.{$name}"]; @@ -206,17 +217,62 @@ class FilesystemTenancyBootstrapper implements TenancyBootstrapper }); foreach ($stores as $name) { - $path = $storagePath . '/framework/cache/data'; + // Store the original configured cache paths, unless they're already stored -- + // scopeCache() is also called in revert(), by which point the originals were + // already captured in bootstrap(). + $this->originalCachePaths[$name] ??= [ + 'path' => $this->app['config']["cache.stores.{$name}.path"], + 'lock_path' => $this->app['config']["cache.stores.{$name}.lock_path"], + ]; + + $path = $this->scopeCachePath($this->originalCachePaths[$name]['path'], $suffix); + $lockPath = $this->originalCachePaths[$name]['lock_path']; + if ($lockPath !== null) { + // Unlike path, lock_path is optional -- if it's not set, FileStore::lock() falls + // back to path itself (see `$this->lockDirectory ?? $this->directory` in FileStore). + // Leave it null here rather than hardcoding it to $path ourselves, so a store that didn't + // configure a separate lock_path doesn't end up with one. + $lockPath = $this->scopeCachePath($lockPath, $suffix); + } + $this->app['config']["cache.stores.{$name}.path"] = $path; - $this->app['config']["cache.stores.{$name}.lock_path"] = $path; + $this->app['config']["cache.stores.{$name}.lock_path"] = $lockPath; /** @var \Illuminate\Cache\FileStore $store */ $store = $this->app['cache']->store($name)->getStore(); $store->setDirectory($path); - $store->setLockDirectory($path); + $store->setLockDirectory($lockPath); } } + /** + * Return a configured path ($configuredPath is e.g. storage_path('framework/cache/data')), + * scoped to the current tenant when called from bootstrap(), + * or unchanged when called from revert() (= when $suffix is false). + */ + protected function scopeCachePath(string $configuredPath, string|false $suffix): string + { + if ($suffix === false) { + return $configuredPath; + } + + if (str_starts_with($configuredPath, $this->originalStoragePath . '/')) { + // Swap the central storage path prefix for the tenant's. + // For example, storage_path('framework/cache/data') becomes storage_path('tenant1/framework/cache/data'). + $scopedPath = str($configuredPath) + ->after($this->originalStoragePath . '/') + ->prepend($this->tenantStoragePath($suffix) . '/') + ->toString(); + } else { + // Append the tenant suffix so tenants don't share the same cache directory. $configuredPath + // isn't guaranteed to be storage_path()-based (e.g. it could point to a shared network + // mount used to keep the file cache off each server's local disk in a multi-server setup). + $scopedPath = rtrim($configuredPath, '/') . '/' . $suffix; + } + + return $scopedPath; + } + public function scopeSessions(string|false $suffix): void { if (! $this->app['config']['tenancy.filesystem.scope_sessions']) { From 958fc8e06d2d732756d123795ce7841e983401c5 Mon Sep 17 00:00:00 2001 From: lukinovec Date: Fri, 31 Jul 2026 14:00:36 +0200 Subject: [PATCH 03/12] Refactor FS bootstrapper scopeCache() didn't feel right since it 1) stored thee original paths, 2) actually scoped things. Separate the concerns so that scopeCache() just does that -- scopes cache. --- .../FilesystemTenancyBootstrapper.php | 39 ++++++++++++++----- 1 file changed, 29 insertions(+), 10 deletions(-) diff --git a/src/Bootstrappers/FilesystemTenancyBootstrapper.php b/src/Bootstrappers/FilesystemTenancyBootstrapper.php index c573db8f..28e28aa7 100644 --- a/src/Bootstrappers/FilesystemTenancyBootstrapper.php +++ b/src/Bootstrappers/FilesystemTenancyBootstrapper.php @@ -46,6 +46,7 @@ class FilesystemTenancyBootstrapper implements TenancyBootstrapper $this->storagePath($suffix); $this->assetHelper($suffix); $this->forgetDisks(); + $this->storeOriginalCachePaths(); $this->scopeCache($suffix); $this->scopeSessions($suffix); @@ -200,13 +201,10 @@ class FilesystemTenancyBootstrapper implements TenancyBootstrapper } } - public function scopeCache(string|false $suffix): void + /** Returns the names of the file-driver stores listed in tenancy.cache.stores. */ + protected function fileCacheStores(): array { - if (! $this->app['config']['tenancy.filesystem.scope_cache']) { - return; - } - - $stores = array_filter($this->app['config']['tenancy.cache.stores'], function ($name) { + return array_filter($this->app['config']['tenancy.cache.stores'], function ($name) { $store = $this->app['config']["cache.stores.{$name}"]; if ($store === null) { @@ -215,15 +213,36 @@ class FilesystemTenancyBootstrapper implements TenancyBootstrapper return $store['driver'] === 'file'; }); + } - foreach ($stores as $name) { - // Store the original configured cache paths, unless they're already stored -- - // scopeCache() is also called in revert(), by which point the originals were - // already captured in bootstrap(). + /** + * Store the original configured cache paths, so that they can be properly scoped + * to a tenant and restored back to the central context when tenancy ends. + */ + protected function storeOriginalCachePaths(): void + { + if (! $this->app['config']['tenancy.filesystem.scope_cache']) { + return; + } + + foreach ($this->fileCacheStores() as $name) { $this->originalCachePaths[$name] ??= [ 'path' => $this->app['config']["cache.stores.{$name}.path"], 'lock_path' => $this->app['config']["cache.stores.{$name}.lock_path"], ]; + } + } + + protected function scopeCache(string|false $suffix): void + { + if (! $this->app['config']['tenancy.filesystem.scope_cache']) { + return; + } + + foreach ($this->fileCacheStores() as $name) { + if (! isset($this->originalCachePaths[$name])) { + continue; + } $path = $this->scopeCachePath($this->originalCachePaths[$name]['path'], $suffix); $lockPath = $this->originalCachePaths[$name]['lock_path']; From 929ad14a6dd59c3d242d6e35fbcbec485b57c697 Mon Sep 17 00:00:00 2001 From: lukinovec Date: Mon, 3 Aug 2026 12:17:23 +0200 Subject: [PATCH 04/12] Make scopeCache public again Making it protected could be a minor bc, and it'd be inconsistent with scopeSessions (which is public). --- src/Bootstrappers/FilesystemTenancyBootstrapper.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Bootstrappers/FilesystemTenancyBootstrapper.php b/src/Bootstrappers/FilesystemTenancyBootstrapper.php index 28e28aa7..23233c08 100644 --- a/src/Bootstrappers/FilesystemTenancyBootstrapper.php +++ b/src/Bootstrappers/FilesystemTenancyBootstrapper.php @@ -233,7 +233,7 @@ class FilesystemTenancyBootstrapper implements TenancyBootstrapper } } - protected function scopeCache(string|false $suffix): void + public function scopeCache(string|false $suffix): void { if (! $this->app['config']['tenancy.filesystem.scope_cache']) { return; From be1e8aa8d4e42c404269e444d974d61a74acdefd Mon Sep 17 00:00:00 2001 From: lukinovec Date: Mon, 3 Aug 2026 12:54:38 +0200 Subject: [PATCH 05/12] Add comments for clarity --- src/Bootstrappers/FilesystemTenancyBootstrapper.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/Bootstrappers/FilesystemTenancyBootstrapper.php b/src/Bootstrappers/FilesystemTenancyBootstrapper.php index 23233c08..f5a1533c 100644 --- a/src/Bootstrappers/FilesystemTenancyBootstrapper.php +++ b/src/Bootstrappers/FilesystemTenancyBootstrapper.php @@ -226,6 +226,8 @@ class FilesystemTenancyBootstrapper implements TenancyBootstrapper } foreach ($this->fileCacheStores() as $name) { + // Capture the paths only once. The config holds the central values on the first + // bootstrap, but not necessarily later (if an earlier bootstrap threw, revert() didn't run). $this->originalCachePaths[$name] ??= [ 'path' => $this->app['config']["cache.stores.{$name}.path"], 'lock_path' => $this->app['config']["cache.stores.{$name}.lock_path"], @@ -241,6 +243,8 @@ class FilesystemTenancyBootstrapper implements TenancyBootstrapper foreach ($this->fileCacheStores() as $name) { if (! isset($this->originalCachePaths[$name])) { + // Only scope stores captured during bootstrap. If one was added to tenancy.cache.stores + // mid-request, it has no original path here -- reading it would throw when tenancy ends. continue; } From aabba924ff555bacc428bf3033ef9b8d55c89d77 Mon Sep 17 00:00:00 2001 From: lukinovec Date: Mon, 3 Aug 2026 13:24:03 +0200 Subject: [PATCH 06/12] Improve comments in the FS bootstrapper test file (delete notes about regression) --- .../FilesystemTenancyBootstrapperTest.php | 25 ++++++++----------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php b/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php index 73352dcd..c4a3e54c 100644 --- a/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php +++ b/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php @@ -301,13 +301,12 @@ test('scoped disks are scoped per tenant', function () { }); test('file cache stores are separated per tenant', function () { - // NOTE ABOUT REGRESSION: this is not a regression test, - // this just covers what wasn't covered before. config([ 'tenancy.bootstrappers' => [ FilesystemTenancyBootstrapper::class, ], 'tenancy.cache.stores' => ['file'], + // Laravel's default 'file' store config (set explicitly here just for clarity). 'cache.stores.file' => [ 'driver' => 'file', 'path' => storage_path('framework/cache/data'), @@ -369,8 +368,7 @@ test('central cache is not lost when tenancy ends', function () { tenancy()->end(); // Nothing deleted the 'foo' entry, so its value should stay 'central' even after reverting tenancy. - // NOTE ABOUT REGRESSION: revert() sets the store to the hardcoded storage/framework/cache/data instead of - // setting it to whatever it was before initializing tenancy. So the central 'foo' entry is null. + // FilesystemTenancyBootstrapper::revert() makes the store use its original configured path. expect(Cache::store('foo_file')->get('foo'))->toBe('central'); File::deleteDirectory($path); @@ -405,9 +403,8 @@ test('file cache stores with different configured paths do not share a directory Cache::store('foo_file')->put('key', 'foo'); Cache::store('bar_file')->put('key', 'bar'); - // NOTE ABOUT REGRESSION: both stores are pointed at the same path, - // so they overwrite and read each other's entries. - // For the same reason, flushing one of them empties the other. + // Each store uses its own directory in the tenant's context, so they don't read or overwrite + // each other's entries, and flushing one doesn't empty the other. expect(Cache::store('foo_file')->get('key'))->toBe('foo'); expect(Cache::store('bar_file')->get('key'))->toBe('bar'); @@ -455,8 +452,8 @@ test('a configured lock_path is scoped separately from path', function () { $tenantPath = "{$centralStoragePath}/tenant{$tenant->id}/framework/cache/foo"; $tenantLockPath = "{$centralStoragePath}/tenant{$tenant->id}/framework/cache/foo_locks"; - // NOTE ABOUT REGRESSION: path and lock_path both get hardcoded to the same default directory - // instead of the ones configured here, so this lock directory never gets created. + // Taking the lock creates the tenant's scoped lock_path directory. + // Nothing wrote a cache entry, so the tenant's scoped path directory doesn't exist (path and lock_path are scoped separately). expect(File::isDirectory($tenantLockPath))->toBeTrue(); expect(File::isDirectory($tenantPath))->toBeFalse(); @@ -464,8 +461,7 @@ test('a configured lock_path is scoped separately from path', function () { expect(Cache::store('foo_file')->lock('foo')->get())->toBeTrue(); - // NOTE ABOUT REGRESSION: revert() has the same hardcoding bug, so lock_path is never restored - // to what it was configured with either. + // After reverting, locks go back to the configured central lock_path. expect(File::isDirectory($lockPath))->toBeTrue(); File::deleteDirectory($path); @@ -537,10 +533,9 @@ test('a cache store using a path not based on storage_path() is suffixed in plac expect(File::isDirectory("{$path}/tenant{$tenant1->id}"))->toBeTrue(); - // NOTE ABOUT REGRESSION: storage_path() is already scoped to tenant1 here (storagePath() runs - // before scopeCache() in bootstrap()), so this is the tenant's default cache directory - // the code points every store's cache at, regardless of its configured path. It should stay - // empty here, since foo_file is configured with its own $path that's not storage_path()-based. + // storage_path() is already scoped to tenant1 here (storagePath() runs before scopeCache() in + // bootstrap()), so this is the tenant's default cache directory. Nothing should be there, since + // foo_file is configured with its own $path that isn't storage_path()-based. expect(File::isDirectory(storage_path('framework/cache/data')))->toBeFalse(); tenancy()->end(); From 6b6279832eacda86b87b6a92215c5c381f60e93c Mon Sep 17 00:00:00 2001 From: lukinovec Date: Mon, 3 Aug 2026 17:07:53 +0200 Subject: [PATCH 07/12] Improve coverage --- .../FilesystemTenancyBootstrapperTest.php | 152 ++++++++++++++++++ 1 file changed, 152 insertions(+) diff --git a/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php b/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php index c4a3e54c..368e65c5 100644 --- a/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php +++ b/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php @@ -542,3 +542,155 @@ test('a cache store using a path not based on storage_path() is suffixed in plac File::deleteDirectory($path); }); + +test('cache stores are not scoped at all when scope_cache is disabled', function () { + $path = storage_path('framework/cache/foo'); + File::deleteDirectory($path); + + config([ + 'tenancy.bootstrappers' => [ + FilesystemTenancyBootstrapper::class, + ], + 'tenancy.filesystem.scope_cache' => false, + 'tenancy.cache.stores' => ['foo_file'], + 'cache.stores.foo_file' => [ + 'driver' => 'file', + 'path' => $path, + 'lock_path' => $path, + ], + ]); + + tenancy()->initialize(Tenant::create()); + + // The store keeps its configured path, so tenants share a single cache directory + expect(config('cache.stores.foo_file.path'))->toBe($path); + Cache::store('foo_file')->put('key', 'tenant'); + expect(File::isDirectory($path))->toBeTrue(); + + tenancy()->end(); + + expect(config('cache.stores.foo_file.path'))->toBe($path); + expect(Cache::store('foo_file')->get('key'))->toBe('tenant'); + + File::deleteDirectory($path); +}); + +test('a store added to tenancy.cache.stores while tenancy is initialized is skipped', function () { + $fooPath = storage_path('framework/cache/foo'); + $barPath = storage_path('framework/cache/bar'); + + File::deleteDirectory($fooPath); + File::deleteDirectory($barPath); + + config([ + 'tenancy.bootstrappers' => [ + FilesystemTenancyBootstrapper::class, + ], + 'tenancy.cache.stores' => ['foo_file'], + 'cache.stores.foo_file' => [ + 'driver' => 'file', + 'path' => $fooPath, + 'lock_path' => $fooPath, + ], + 'cache.stores.bar_file' => [ + 'driver' => 'file', + 'path' => $barPath, + 'lock_path' => $barPath, + ], + ]); + + $tenant = Tenant::create(); + tenancy()->initialize($tenant); + + expect(config('cache.stores.foo_file.path'))->toBe(storage_path('framework/cache/foo')); + + // bar_file wasn't in tenancy.cache.stores during bootstrap, so no original path was stored for it + config(['tenancy.cache.stores' => ['foo_file', 'bar_file']]); + + tenancy()->end(); + + // foo_file gets reverted, but bar_file is left with the path it's configured with + expect(config('cache.stores.foo_file.path'))->toBe($fooPath); + expect(config('cache.stores.bar_file.path'))->toBe($barPath); + + File::deleteDirectory($fooPath); + File::deleteDirectory($barPath); +}); + +test('a store listed in tenancy.cache.stores that has no cache config is skipped', function () { + $path = storage_path('framework/cache/foo'); + File::deleteDirectory($path); + + config([ + 'tenancy.bootstrappers' => [ + FilesystemTenancyBootstrapper::class, + ], + 'tenancy.cache.stores' => ['foo_file', 'nonexistent_store'], + 'cache.stores.foo_file' => [ + 'driver' => 'file', + 'path' => $path, + 'lock_path' => $path, + ], + ]); + + $tenant = Tenant::create(); + + // The store doesn't exist in cache.stores, so there's nothing to scope (resolving it would throw) + tenancy()->initialize($tenant); + + expect(config('cache.stores.foo_file.path'))->toBe(storage_path('framework/cache/foo')); + expect(config('cache.stores.nonexistent_store'))->toBeNull(); + + tenancy()->end(); + + expect(config('cache.stores.foo_file.path'))->toBe($path); + + File::deleteDirectory($path); +}); + +test('the original cache paths are only stored on the first bootstrap', function () { + // If bootstrap() throws after scopeCache(), the bootstrapper isn't reverted, so the config still + // holds the previous tenant's scoped paths during the next bootstrap. Storing the paths again there + // would lose the central ones forever. + $centralStoragePath = storage_path(); + $path = "{$centralStoragePath}/framework/cache/foo"; + File::deleteDirectory($path); + + config([ + 'tenancy.bootstrappers' => [ + FilesystemTenancyBootstrapper::class, + ], + 'tenancy.cache.stores' => ['foo_file'], + 'cache.stores.foo_file' => [ + 'driver' => 'file', + 'path' => $path, + 'lock_path' => $path, + ], + ]); + + $tenant1 = Tenant::create(); + $tenant2 = Tenant::create(); + + // Make scopeSessions() fail for tenant1 by putting a file where its sessions directory should be. + // scopeSessions() runs after scopeCache(), so the cache paths are already scoped when bootstrap() throws. + $sessionsPath = "{$centralStoragePath}/tenant{$tenant1->id}/framework/sessions"; + File::deleteDirectory($sessionsPath); + File::ensureDirectoryExists(dirname($sessionsPath)); + File::put($sessionsPath, ''); + + expect(fn () => tenancy()->initialize($tenant1))->toThrow(Exception::class, "Unable to create tenant session directory [{$sessionsPath}]."); + + // bootstrap() threw, so the bootstrapper isn't in initializedBootstrappers and revert() gets skipped for it. + expect(config('cache.stores.foo_file.path'))->toBe("{$centralStoragePath}/tenant{$tenant1->id}/framework/cache/foo"); + + tenancy()->initialize($tenant2); + + expect(config('cache.stores.foo_file.path'))->toBe("{$centralStoragePath}/tenant{$tenant2->id}/framework/cache/foo"); + + tenancy()->end(); + + expect(config('cache.stores.foo_file.path'))->toBe($path); + + File::deleteDirectory("{$centralStoragePath}/tenant{$tenant1->id}"); + File::deleteDirectory($path); +}); From 0765bfc757ce0bed6e339c09757d971dd966b4eb Mon Sep 17 00:00:00 2001 From: Samuel Stancl Date: Mon, 3 Aug 2026 23:45:10 -0700 Subject: [PATCH 08/12] simplify code --- .../FilesystemTenancyBootstrapper.php | 109 +++++------------- .../FilesystemTenancyBootstrapperTest.php | 2 +- 2 files changed, 32 insertions(+), 79 deletions(-) diff --git a/src/Bootstrappers/FilesystemTenancyBootstrapper.php b/src/Bootstrappers/FilesystemTenancyBootstrapper.php index f5a1533c..d6cf12f1 100644 --- a/src/Bootstrappers/FilesystemTenancyBootstrapper.php +++ b/src/Bootstrappers/FilesystemTenancyBootstrapper.php @@ -14,21 +14,8 @@ use Stancl\Tenancy\Contracts\Tenant; class FilesystemTenancyBootstrapper implements TenancyBootstrapper { public array $originalDisks = []; - - /** - * The path and lock_path each file cache store had in the central context, keyed by store name. - * - * For example: - * [ - * 'file' => [ - * 'path' => storage_path('framework/cache/data'), - * 'lock_path' => storage_path('framework/cache/data'), - * ], - * ] - * - * Used to scope the store to a tenant, and to restore it back to this when tenancy ends. - */ protected array $originalCachePaths = []; + protected array $originalCacheLockPaths = []; public string|null $originalAssetUrl; public string $originalStoragePath; @@ -46,7 +33,6 @@ class FilesystemTenancyBootstrapper implements TenancyBootstrapper $this->storagePath($suffix); $this->assetHelper($suffix); $this->forgetDisks(); - $this->storeOriginalCachePaths(); $this->scopeCache($suffix); $this->scopeSessions($suffix); @@ -201,61 +187,40 @@ class FilesystemTenancyBootstrapper implements TenancyBootstrapper } } - /** Returns the names of the file-driver stores listed in tenancy.cache.stores. */ - protected function fileCacheStores(): array - { - return array_filter($this->app['config']['tenancy.cache.stores'], function ($name) { - $store = $this->app['config']["cache.stores.{$name}"]; - - if ($store === null) { - return false; - } - - return $store['driver'] === 'file'; - }); - } - - /** - * Store the original configured cache paths, so that they can be properly scoped - * to a tenant and restored back to the central context when tenancy ends. - */ - protected function storeOriginalCachePaths(): void - { - if (! $this->app['config']['tenancy.filesystem.scope_cache']) { - return; - } - - foreach ($this->fileCacheStores() as $name) { - // Capture the paths only once. The config holds the central values on the first - // bootstrap, but not necessarily later (if an earlier bootstrap threw, revert() didn't run). - $this->originalCachePaths[$name] ??= [ - 'path' => $this->app['config']["cache.stores.{$name}.path"], - 'lock_path' => $this->app['config']["cache.stores.{$name}.lock_path"], - ]; - } - } - public function scopeCache(string|false $suffix): void { if (! $this->app['config']['tenancy.filesystem.scope_cache']) { return; } - foreach ($this->fileCacheStores() as $name) { - if (! isset($this->originalCachePaths[$name])) { - // Only scope stores captured during bootstrap. If one was added to tenancy.cache.stores - // mid-request, it has no original path here -- reading it would throw when tenancy ends. + foreach ($this->app['config']['tenancy.cache.stores'] as $name) { + $store = $this->app['config']["cache.stores.{$name}"]; + + // Only file stores have a path to scope. Skip stores that don't exist (null) or use another driver. + if ($store === null || $store['driver'] !== 'file') { continue; } - $path = $this->scopeCachePath($this->originalCachePaths[$name]['path'], $suffix); - $lockPath = $this->originalCachePaths[$name]['lock_path']; - if ($lockPath !== null) { - // Unlike path, lock_path is optional -- if it's not set, FileStore::lock() falls - // back to path itself (see `$this->lockDirectory ?? $this->directory` in FileStore). - // Leave it null here rather than hardcoding it to $path ourselves, so a store that didn't - // configure a separate lock_path doesn't end up with one. - $lockPath = $this->scopeCachePath($lockPath, $suffix); + if ($suffix !== false && ! isset($this->originalCachePaths[$name])) { + $this->originalCachePaths[$name] = $store['path']; + $this->originalCacheLockPaths[$name] = $store['lock_path'] ?? null; + } + + // Only scope stores captured during bootstrap. If one was added to tenancy.cache.stores + // mid-request, it has no original path here -- reading it would throw when tenancy ends. + if (! isset($this->originalCachePaths[$name])) { + continue; + } + + $path = $suffix ? $this->tenantCachePath($this->originalCachePaths[$name], $suffix) : $this->originalCachePaths[$name]; + + // Unlike path, lock_path is optional -- if it's not set, FileStore::lock() falls back to path + // itself (see `$this->lockDirectory ?? $this->directory` in FileStore). Leave it null here rather + // than hardcoding it to $path ourselves, so a store that didn't configure a separate lock_path + // doesn't end up with one. + $lockPath = $this->originalCacheLockPaths[$name]; + if ($suffix && $lockPath !== null) { + $lockPath = $this->tenantCachePath($lockPath, $suffix); } $this->app['config']["cache.stores.{$name}.path"] = $path; @@ -268,32 +233,20 @@ class FilesystemTenancyBootstrapper implements TenancyBootstrapper } } - /** - * Return a configured path ($configuredPath is e.g. storage_path('framework/cache/data')), - * scoped to the current tenant when called from bootstrap(), - * or unchanged when called from revert() (= when $suffix is false). - */ - protected function scopeCachePath(string $configuredPath, string|false $suffix): string + /** Scope a configured cache path (path or lock_path) to the tenant identified by $suffix. */ + protected function tenantCachePath(string $configuredPath, string $suffix): string { - if ($suffix === false) { - return $configuredPath; - } - if (str_starts_with($configuredPath, $this->originalStoragePath . '/')) { // Swap the central storage path prefix for the tenant's. // For example, storage_path('framework/cache/data') becomes storage_path('tenant1/framework/cache/data'). - $scopedPath = str($configuredPath) + return str($configuredPath) ->after($this->originalStoragePath . '/') ->prepend($this->tenantStoragePath($suffix) . '/') ->toString(); - } else { - // Append the tenant suffix so tenants don't share the same cache directory. $configuredPath - // isn't guaranteed to be storage_path()-based (e.g. it could point to a shared network - // mount used to keep the file cache off each server's local disk in a multi-server setup). - $scopedPath = rtrim($configuredPath, '/') . '/' . $suffix; } - return $scopedPath; + // Otherwise $configuredPath isn't necessarily storage_path()-based, so just append the suffix directly. + return rtrim($configuredPath, '/') . '/' . $suffix; } public function scopeSessions(string|false $suffix): void diff --git a/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php b/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php index 368e65c5..df424720 100644 --- a/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php +++ b/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php @@ -500,7 +500,7 @@ test('a cache store without a configured lock_path is scoped without error', fun }); test('a cache store using a path not based on storage_path() is suffixed in place, not moved under the tenant storage path', function () { - // $path below doesn't start with the central storage path, so scopeCachePath() has no storage + // $path below doesn't start with the central storage path, so tenantCachePath() has no storage // path prefix to swap for the tenant's -- it appends the tenant suffix directly to $path instead. // Check that tenant isolation still works for a store configured like this, and that its cache // ends up at $path itself, not under the tenant's storage path. From 597e48ec90349220aad3655b757c126544f9731f Mon Sep 17 00:00:00 2001 From: lukinovec Date: Tue, 4 Aug 2026 13:55:52 +0200 Subject: [PATCH 09/12] Consolidate scopeCache() coverage into existing tests Note: 'the original cache paths are only stored on the first bootstrap' test got removed -- it tested that the "Unable to create tenant session directory" exception gets thrown, and that's not in scope of the current PR. --- .../FilesystemTenancyBootstrapperTest.php | 201 ++++-------------- 1 file changed, 46 insertions(+), 155 deletions(-) diff --git a/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php b/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php index df424720..6d8d86e8 100644 --- a/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php +++ b/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php @@ -305,7 +305,10 @@ test('file cache stores are separated per tenant', function () { 'tenancy.bootstrappers' => [ FilesystemTenancyBootstrapper::class, ], - 'tenancy.cache.stores' => ['file'], + // Only stores that use the 'file' driver are scoped. + // The 'redis' store won't be scoped since it doesn't use the 'file' driver, + // and 'nonexistent_store' won't be scoped since it just doesn't exist. + 'tenancy.cache.stores' => ['file', 'redis', 'nonexistent_store'], // Laravel's default 'file' store config (set explicitly here just for clarity). 'cache.stores.file' => [ 'driver' => 'file', @@ -318,12 +321,23 @@ test('file cache stores are separated per tenant', function () { $tenant2 = Tenant::create(); Cache::store('file')->put('key', 'central'); + Cache::store('redis')->put('key', 'central'); - tenancy()->initialize($tenant1); + // 'redis' and 'nonexistent_store' are skipped before scopeCache() attempts to read their config. + // Without the skipping logic, the `$this->originalCachePaths[$name] = $store['path']` + // line in scopeCache() would throw an ErrorException (with 'redis', we'd get an + // 'Undefined array key "path"' exception, and with 'nonexistent_store', + // we'd get 'Trying to access array offset on null'). + expect(fn () => tenancy()->initialize($tenant1))->not()->toThrow(ErrorException::class); expect(Cache::store('file')->get('key'))->toBeNull(); Cache::store('file')->put('key', 'tenant1'); + // The 'redis' store doesn't use the file driver (no path to scope), + // so FilesystemTenancyBootstrapper skips it in scopeCache(). + // The central value is retained in the tenant context. + expect(Cache::store('redis')->get('key'))->toBe('central'); + tenancy()->initialize($tenant2); expect(Cache::store('file')->get('key'))->toBeNull(); @@ -336,11 +350,25 @@ test('file cache stores are separated per tenant', function () { tenancy()->end(); expect(Cache::store('file')->get('key'))->toBe('central'); + + // Turn FilesystemTenancyBootstrapper's cache scoping off + config(['tenancy.filesystem.scope_cache' => false]); + + tenancy()->initialize($tenant1); + + expect(Cache::store('file')->get('key'))->toBe('central'); + Cache::store('file')->put('key', 'written in tenant context'); + + tenancy()->end(); + + expect(Cache::store('file')->get('key'))->toBe('written in tenant context'); }); test('central cache is not lost when tenancy ends', function () { $path = storage_path('framework/cache/foo_file'); + $barPath = storage_path('framework/cache/bar_file'); File::deleteDirectory($path); + File::deleteDirectory($barPath); // Use a separate 'foo' store rather than reconfiguring 'file'. // TestCase::setUp() calls `cache:clear file`, which resolves the 'file' store and leaves it @@ -351,12 +379,19 @@ test('central cache is not lost when tenancy ends', function () { 'tenancy.bootstrappers' => [ FilesystemTenancyBootstrapper::class, ], - 'tenancy.cache.stores' => ['foo_file'], 'cache.stores.foo_file' => [ 'driver' => 'file', 'path' => $path, 'lock_path' => $path, ], + 'cache.stores.bar_file' => [ + 'driver' => 'file', + 'path' => $barPath, + 'lock_path' => $barPath, + ], + // Only include foo_file in tenancy.cache.stores, + // leave bar_file excluded (= not scoped by FilesystemTenancyBootstrapper) for now. + 'tenancy.cache.stores' => ['foo_file'], ]); $tenant = Tenant::create(); @@ -365,13 +400,21 @@ test('central cache is not lost when tenancy ends', function () { // Just initialize and revert tenancy to trigger FilesystemTenancyBootstrapper tenancy()->initialize($tenant); + + // bar_file wasn't in tenancy.cache.stores during bootstrap, so no original path was stored for it, + // and it's left with its configured path (not scoped). + config(['tenancy.cache.stores' => ['foo_file', 'bar_file']]); + Cache::store('bar_file')->put('bar', 'not scoped'); + tenancy()->end(); // Nothing deleted the 'foo' entry, so its value should stay 'central' even after reverting tenancy. // FilesystemTenancyBootstrapper::revert() makes the store use its original configured path. expect(Cache::store('foo_file')->get('foo'))->toBe('central'); + expect(Cache::store('bar_file')->get('bar'))->toBe('not scoped'); File::deleteDirectory($path); + File::deleteDirectory($barPath); }); test('file cache stores with different configured paths do not share a directory', function () { @@ -542,155 +585,3 @@ test('a cache store using a path not based on storage_path() is suffixed in plac File::deleteDirectory($path); }); - -test('cache stores are not scoped at all when scope_cache is disabled', function () { - $path = storage_path('framework/cache/foo'); - File::deleteDirectory($path); - - config([ - 'tenancy.bootstrappers' => [ - FilesystemTenancyBootstrapper::class, - ], - 'tenancy.filesystem.scope_cache' => false, - 'tenancy.cache.stores' => ['foo_file'], - 'cache.stores.foo_file' => [ - 'driver' => 'file', - 'path' => $path, - 'lock_path' => $path, - ], - ]); - - tenancy()->initialize(Tenant::create()); - - // The store keeps its configured path, so tenants share a single cache directory - expect(config('cache.stores.foo_file.path'))->toBe($path); - Cache::store('foo_file')->put('key', 'tenant'); - expect(File::isDirectory($path))->toBeTrue(); - - tenancy()->end(); - - expect(config('cache.stores.foo_file.path'))->toBe($path); - expect(Cache::store('foo_file')->get('key'))->toBe('tenant'); - - File::deleteDirectory($path); -}); - -test('a store added to tenancy.cache.stores while tenancy is initialized is skipped', function () { - $fooPath = storage_path('framework/cache/foo'); - $barPath = storage_path('framework/cache/bar'); - - File::deleteDirectory($fooPath); - File::deleteDirectory($barPath); - - config([ - 'tenancy.bootstrappers' => [ - FilesystemTenancyBootstrapper::class, - ], - 'tenancy.cache.stores' => ['foo_file'], - 'cache.stores.foo_file' => [ - 'driver' => 'file', - 'path' => $fooPath, - 'lock_path' => $fooPath, - ], - 'cache.stores.bar_file' => [ - 'driver' => 'file', - 'path' => $barPath, - 'lock_path' => $barPath, - ], - ]); - - $tenant = Tenant::create(); - tenancy()->initialize($tenant); - - expect(config('cache.stores.foo_file.path'))->toBe(storage_path('framework/cache/foo')); - - // bar_file wasn't in tenancy.cache.stores during bootstrap, so no original path was stored for it - config(['tenancy.cache.stores' => ['foo_file', 'bar_file']]); - - tenancy()->end(); - - // foo_file gets reverted, but bar_file is left with the path it's configured with - expect(config('cache.stores.foo_file.path'))->toBe($fooPath); - expect(config('cache.stores.bar_file.path'))->toBe($barPath); - - File::deleteDirectory($fooPath); - File::deleteDirectory($barPath); -}); - -test('a store listed in tenancy.cache.stores that has no cache config is skipped', function () { - $path = storage_path('framework/cache/foo'); - File::deleteDirectory($path); - - config([ - 'tenancy.bootstrappers' => [ - FilesystemTenancyBootstrapper::class, - ], - 'tenancy.cache.stores' => ['foo_file', 'nonexistent_store'], - 'cache.stores.foo_file' => [ - 'driver' => 'file', - 'path' => $path, - 'lock_path' => $path, - ], - ]); - - $tenant = Tenant::create(); - - // The store doesn't exist in cache.stores, so there's nothing to scope (resolving it would throw) - tenancy()->initialize($tenant); - - expect(config('cache.stores.foo_file.path'))->toBe(storage_path('framework/cache/foo')); - expect(config('cache.stores.nonexistent_store'))->toBeNull(); - - tenancy()->end(); - - expect(config('cache.stores.foo_file.path'))->toBe($path); - - File::deleteDirectory($path); -}); - -test('the original cache paths are only stored on the first bootstrap', function () { - // If bootstrap() throws after scopeCache(), the bootstrapper isn't reverted, so the config still - // holds the previous tenant's scoped paths during the next bootstrap. Storing the paths again there - // would lose the central ones forever. - $centralStoragePath = storage_path(); - $path = "{$centralStoragePath}/framework/cache/foo"; - File::deleteDirectory($path); - - config([ - 'tenancy.bootstrappers' => [ - FilesystemTenancyBootstrapper::class, - ], - 'tenancy.cache.stores' => ['foo_file'], - 'cache.stores.foo_file' => [ - 'driver' => 'file', - 'path' => $path, - 'lock_path' => $path, - ], - ]); - - $tenant1 = Tenant::create(); - $tenant2 = Tenant::create(); - - // Make scopeSessions() fail for tenant1 by putting a file where its sessions directory should be. - // scopeSessions() runs after scopeCache(), so the cache paths are already scoped when bootstrap() throws. - $sessionsPath = "{$centralStoragePath}/tenant{$tenant1->id}/framework/sessions"; - File::deleteDirectory($sessionsPath); - File::ensureDirectoryExists(dirname($sessionsPath)); - File::put($sessionsPath, ''); - - expect(fn () => tenancy()->initialize($tenant1))->toThrow(Exception::class, "Unable to create tenant session directory [{$sessionsPath}]."); - - // bootstrap() threw, so the bootstrapper isn't in initializedBootstrappers and revert() gets skipped for it. - expect(config('cache.stores.foo_file.path'))->toBe("{$centralStoragePath}/tenant{$tenant1->id}/framework/cache/foo"); - - tenancy()->initialize($tenant2); - - expect(config('cache.stores.foo_file.path'))->toBe("{$centralStoragePath}/tenant{$tenant2->id}/framework/cache/foo"); - - tenancy()->end(); - - expect(config('cache.stores.foo_file.path'))->toBe($path); - - File::deleteDirectory("{$centralStoragePath}/tenant{$tenant1->id}"); - File::deleteDirectory($path); -}); From 8244e56618c02ccbb89cc2fbb39761e2b6765652 Mon Sep 17 00:00:00 2001 From: lukinovec Date: Tue, 4 Aug 2026 18:21:21 +0200 Subject: [PATCH 10/12] Handle tenancy.cache.stores changes during revert Also add a separate test ('scopeCache ignores changes to tenancy.cache.stores made in tenant context' ) -- the 'central cache is not lost when tenancy ends' covered the skipping mechanism partially, but having a separate test for the tenancy.cache.stores mid-tenant context changes is definitely cleaner and makes more sense. --- .../FilesystemTenancyBootstrapper.php | 9 ++- .../FilesystemTenancyBootstrapperTest.php | 69 +++++++++++++------ 2 files changed, 57 insertions(+), 21 deletions(-) diff --git a/src/Bootstrappers/FilesystemTenancyBootstrapper.php b/src/Bootstrappers/FilesystemTenancyBootstrapper.php index d6cf12f1..21281955 100644 --- a/src/Bootstrappers/FilesystemTenancyBootstrapper.php +++ b/src/Bootstrappers/FilesystemTenancyBootstrapper.php @@ -193,7 +193,14 @@ class FilesystemTenancyBootstrapper implements TenancyBootstrapper return; } - foreach ($this->app['config']['tenancy.cache.stores'] as $name) { + // On revert, restore exactly the stores captured during bootstrap -- not the current + // (possibly mutated) tenancy.cache.stores. Otherwise, removing a store from that + // config in tenant context would make revert() skip it (so the store would be stuck with a tenant-scoped path). + $stores = $suffix !== false + ? $this->app['config']['tenancy.cache.stores'] + : array_keys($this->originalCachePaths); + + foreach ($stores as $name) { $store = $this->app['config']["cache.stores.{$name}"]; // Only file stores have a path to scope. Skip stores that don't exist (null) or use another driver. diff --git a/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php b/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php index 6d8d86e8..8a919d31 100644 --- a/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php +++ b/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php @@ -366,11 +366,9 @@ test('file cache stores are separated per tenant', function () { test('central cache is not lost when tenancy ends', function () { $path = storage_path('framework/cache/foo_file'); - $barPath = storage_path('framework/cache/bar_file'); File::deleteDirectory($path); - File::deleteDirectory($barPath); - // Use a separate 'foo' store rather than reconfiguring 'file'. + // Use a separate 'foo_file' store rather than reconfiguring 'file'. // TestCase::setUp() calls `cache:clear file`, which resolves the 'file' store and leaves it // in the CacheManager with the default path. A later config() call can't mutate that, so a test // reconfiguring 'file' would keep using storage/framework/cache/data and pass either way. @@ -384,36 +382,67 @@ test('central cache is not lost when tenancy ends', function () { 'path' => $path, 'lock_path' => $path, ], - 'cache.stores.bar_file' => [ - 'driver' => 'file', - 'path' => $barPath, - 'lock_path' => $barPath, - ], - // Only include foo_file in tenancy.cache.stores, - // leave bar_file excluded (= not scoped by FilesystemTenancyBootstrapper) for now. 'tenancy.cache.stores' => ['foo_file'], ]); - $tenant = Tenant::create(); - Cache::store('foo_file')->put('foo', 'central'); // Just initialize and revert tenancy to trigger FilesystemTenancyBootstrapper - tenancy()->initialize($tenant); - - // bar_file wasn't in tenancy.cache.stores during bootstrap, so no original path was stored for it, - // and it's left with its configured path (not scoped). - config(['tenancy.cache.stores' => ['foo_file', 'bar_file']]); - Cache::store('bar_file')->put('bar', 'not scoped'); - + tenancy()->initialize(Tenant::create()); tenancy()->end(); // Nothing deleted the 'foo' entry, so its value should stay 'central' even after reverting tenancy. // FilesystemTenancyBootstrapper::revert() makes the store use its original configured path. expect(Cache::store('foo_file')->get('foo'))->toBe('central'); - expect(Cache::store('bar_file')->get('bar'))->toBe('not scoped'); File::deleteDirectory($path); +}); + +test('scopeCache ignores changes to tenancy.cache.stores made in tenant context', function () { + $fooPath = storage_path('framework/cache/foo_file'); + $barPath = storage_path('framework/cache/bar_file'); + File::deleteDirectory($fooPath); + File::deleteDirectory($barPath); + + config([ + 'tenancy.bootstrappers' => [ + FilesystemTenancyBootstrapper::class, + ], + 'cache.stores.foo_file' => [ + 'driver' => 'file', + 'path' => $fooPath, + 'lock_path' => $fooPath, + ], + 'cache.stores.bar_file' => [ + 'driver' => 'file', + 'path' => $barPath, + 'lock_path' => $barPath, + ], + // Only foo_file is scoped at bootstrap() + 'tenancy.cache.stores' => ['foo_file'], + ]); + + Cache::store('foo_file')->put('key', 'central'); + Cache::store('bar_file')->put('key', 'central'); + + tenancy()->initialize(Tenant::create()); + + Cache::store('foo_file')->put('key', 'tenant'); + + // bar_file wasn't in tenancy.cache.stores during bootstrap, so no original path was captured for it, + // and it uses its configured path (central, not scoped). + config(['tenancy.cache.stores' => ['foo_file', 'bar_file']]); + expect(Cache::store('bar_file')->get('key'))->toBe('central'); + + // Remove foo_file from tenancy.cache.stores (original path was captured during bootstrap) in tenant context + config(['tenancy.cache.stores' => ['bar_file']]); + + tenancy()->end(); + + // revert() still restores foo_file to its central path + expect(Cache::store('foo_file')->get('key'))->toBe('central'); + + File::deleteDirectory($fooPath); File::deleteDirectory($barPath); }); From 7b12d51efed89d02925ab33dc8c86659e97d3d67 Mon Sep 17 00:00:00 2001 From: lukinovec Date: Wed, 5 Aug 2026 13:48:12 +0200 Subject: [PATCH 11/12] Delete unreachable code The check could only pass with a `null` path, which is just bad configuration. So no reason to keep this. --- src/Bootstrappers/FilesystemTenancyBootstrapper.php | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/Bootstrappers/FilesystemTenancyBootstrapper.php b/src/Bootstrappers/FilesystemTenancyBootstrapper.php index 21281955..78204fd9 100644 --- a/src/Bootstrappers/FilesystemTenancyBootstrapper.php +++ b/src/Bootstrappers/FilesystemTenancyBootstrapper.php @@ -213,12 +213,6 @@ class FilesystemTenancyBootstrapper implements TenancyBootstrapper $this->originalCacheLockPaths[$name] = $store['lock_path'] ?? null; } - // Only scope stores captured during bootstrap. If one was added to tenancy.cache.stores - // mid-request, it has no original path here -- reading it would throw when tenancy ends. - if (! isset($this->originalCachePaths[$name])) { - continue; - } - $path = $suffix ? $this->tenantCachePath($this->originalCachePaths[$name], $suffix) : $this->originalCachePaths[$name]; // Unlike path, lock_path is optional -- if it's not set, FileStore::lock() falls back to path From 3e564c1f22e7a518546c126e7e38e33f10f090f4 Mon Sep 17 00:00:00 2001 From: lukinovec Date: Wed, 5 Aug 2026 17:04:08 +0200 Subject: [PATCH 12/12] Improve tests Add more meaningful assertions, make the tests clearer (by improving the names, commnets and the test code itself), merge separate tests that don't need to be separate. Also, don't set lock_path in tests that don't deal with locks (except for the generic "file cache stores are separated per tenant" test where we just want to mirror Laravel's default file store config -- though note that keeping the lock_path unset or null there would make no difference). --- .../FilesystemTenancyBootstrapperTest.php | 191 +++++++++--------- 1 file changed, 93 insertions(+), 98 deletions(-) diff --git a/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php b/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php index 8a919d31..39d5cef7 100644 --- a/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php +++ b/tests/Bootstrappers/FilesystemTenancyBootstrapperTest.php @@ -309,7 +309,7 @@ test('file cache stores are separated per tenant', function () { // The 'redis' store won't be scoped since it doesn't use the 'file' driver, // and 'nonexistent_store' won't be scoped since it just doesn't exist. 'tenancy.cache.stores' => ['file', 'redis', 'nonexistent_store'], - // Laravel's default 'file' store config (set explicitly here just for clarity). + // Laravel's default 'file' store config (set explicitly here for clarity) 'cache.stores.file' => [ 'driver' => 'file', 'path' => storage_path('framework/cache/data'), @@ -323,7 +323,7 @@ test('file cache stores are separated per tenant', function () { Cache::store('file')->put('key', 'central'); Cache::store('redis')->put('key', 'central'); - // 'redis' and 'nonexistent_store' are skipped before scopeCache() attempts to read their config. + // 'redis' and 'nonexistent_store' are skipped by the driver check in scopeCache(), before it reads their path. // Without the skipping logic, the `$this->originalCachePaths[$name] = $store['path']` // line in scopeCache() would throw an ErrorException (with 'redis', we'd get an // 'Undefined array key "path"' exception, and with 'nonexistent_store', @@ -364,38 +364,59 @@ test('file cache stores are separated per tenant', function () { expect(Cache::store('file')->get('key'))->toBe('written in tenant context'); }); -test('central cache is not lost when tenancy ends', function () { - $path = storage_path('framework/cache/foo_file'); - File::deleteDirectory($path); +test('file cache stores get their path scoped on bootstrap and restored back on revert', function () { + $fooPath = storage_path('framework/cache/foo_file'); + $barPath = storage_path('framework/cache/bar_file'); + File::deleteDirectory($fooPath); + File::deleteDirectory($barPath); - // Use a separate 'foo_file' store rather than reconfiguring 'file'. + // Use separate 'foo_file'/'bar_file' stores rather than reconfiguring 'file'. // TestCase::setUp() calls `cache:clear file`, which resolves the 'file' store and leaves it - // in the CacheManager with the default path. A later config() call can't mutate that, so a test - // reconfiguring 'file' would keep using storage/framework/cache/data and pass either way. + // in the CacheManager with the default path. A later config() call can't mutate that, + // so in central context, cache would be written to 'storage/framework/cache/data' no matter how the config changes. // The same applies to the other tests below that configure separate cache stores rather than using 'file'. config([ 'tenancy.bootstrappers' => [ FilesystemTenancyBootstrapper::class, ], + 'tenancy.cache.stores' => ['foo_file', 'bar_file'], 'cache.stores.foo_file' => [ 'driver' => 'file', - 'path' => $path, - 'lock_path' => $path, + 'path' => $fooPath, + ], + 'cache.stores.bar_file' => [ + 'driver' => 'file', + 'path' => $barPath, ], - 'tenancy.cache.stores' => ['foo_file'], ]); - Cache::store('foo_file')->put('foo', 'central'); + Cache::store('foo_file')->put('key', 'central foo'); + Cache::store('bar_file')->put('key', 'central bar'); - // Just initialize and revert tenancy to trigger FilesystemTenancyBootstrapper tenancy()->initialize(Tenant::create()); + + Cache::store('foo_file')->put('key', 'tenant foo'); + Cache::store('bar_file')->put('key', 'tenant bar'); + + // Each store uses its own scoped path. + // The stores don't read or overwrite each other's entries. + expect(Cache::store('foo_file')->get('key'))->toBe('tenant foo'); + expect(Cache::store('bar_file')->get('key'))->toBe('tenant bar'); + + Cache::store('bar_file')->flush(); + + // Only bar_file was flushed + expect(Cache::store('bar_file')->get('key'))->toBeNull(); + expect(Cache::store('foo_file')->get('key'))->toBe('tenant foo'); + tenancy()->end(); - // Nothing deleted the 'foo' entry, so its value should stay 'central' even after reverting tenancy. - // FilesystemTenancyBootstrapper::revert() makes the store use its original configured path. - expect(Cache::store('foo_file')->get('foo'))->toBe('central'); + // revert() points each store back at its original configured path + expect(Cache::store('foo_file')->get('key'))->toBe('central foo'); + expect(Cache::store('bar_file')->get('key'))->toBe('central bar'); - File::deleteDirectory($path); + File::deleteDirectory($fooPath); + File::deleteDirectory($barPath); }); test('scopeCache ignores changes to tenancy.cache.stores made in tenant context', function () { @@ -411,14 +432,12 @@ test('scopeCache ignores changes to tenancy.cache.stores made in tenant context' 'cache.stores.foo_file' => [ 'driver' => 'file', 'path' => $fooPath, - 'lock_path' => $fooPath, ], 'cache.stores.bar_file' => [ 'driver' => 'file', 'path' => $barPath, - 'lock_path' => $barPath, ], - // Only foo_file is scoped at bootstrap() + // Only foo_file is scoped during bootstrap() 'tenancy.cache.stores' => ['foo_file'], ]); @@ -429,76 +448,32 @@ test('scopeCache ignores changes to tenancy.cache.stores made in tenant context' Cache::store('foo_file')->put('key', 'tenant'); - // bar_file wasn't in tenancy.cache.stores during bootstrap, so no original path was captured for it, - // and it uses its configured path (central, not scoped). - config(['tenancy.cache.stores' => ['foo_file', 'bar_file']]); - expect(Cache::store('bar_file')->get('key'))->toBe('central'); - - // Remove foo_file from tenancy.cache.stores (original path was captured during bootstrap) in tenant context + // Mutate tenancy.cache.stores in tenant context (remove 'foo_file', add 'bar_file') config(['tenancy.cache.stores' => ['bar_file']]); + // 'bar_file' wasn't in tenancy.cache.stores during bootstrap. + // No original path is captured for that store, so it continues to use its central path. + expect(Cache::store('bar_file')->get('key'))->toBe('central'); + Cache::store('bar_file')->put('key', 'written in tenant context'); tenancy()->end(); - // revert() still restores foo_file to its central path + // revert() restores the paths for the stores that were scoped during bootstrap expect(Cache::store('foo_file')->get('key'))->toBe('central'); - - File::deleteDirectory($fooPath); - File::deleteDirectory($barPath); -}); - -test('file cache stores with different configured paths do not share a directory', function () { - $fooPath = storage_path('framework/cache/foo'); - $barPath = storage_path('framework/cache/bar'); - - File::deleteDirectory($fooPath); - File::deleteDirectory($barPath); - - config([ - 'tenancy.bootstrappers' => [ - FilesystemTenancyBootstrapper::class, - ], - 'tenancy.cache.stores' => ['foo_file', 'bar_file'], - 'cache.stores.foo_file' => [ - 'driver' => 'file', - 'path' => $fooPath, - 'lock_path' => $fooPath, - ], - 'cache.stores.bar_file' => [ - 'driver' => 'file', - 'path' => $barPath, - 'lock_path' => $barPath, - ], - ]); - - tenancy()->initialize(Tenant::create()); - - Cache::store('foo_file')->put('key', 'foo'); - Cache::store('bar_file')->put('key', 'bar'); - - // Each store uses its own directory in the tenant's context, so they don't read or overwrite - // each other's entries, and flushing one doesn't empty the other. - expect(Cache::store('foo_file')->get('key'))->toBe('foo'); - expect(Cache::store('bar_file')->get('key'))->toBe('bar'); - - Cache::store('bar_file')->flush(); - - expect(Cache::store('foo_file')->get('key'))->toBe('foo'); - - tenancy()->end(); + // 'bar_file' was never scoped, it still reads from the same path it was writing to in tenant context + expect(Cache::store('bar_file')->get('key'))->toBe('written in tenant context'); File::deleteDirectory($fooPath); File::deleteDirectory($barPath); }); test('a configured lock_path is scoped separately from path', function () { - // A store can configure path and lock_path as two different directories. - // Locks should use lock_path IF CONFIGURED, not fall back into the same directory as path. + // A store can configure path and lock_path as two different directories $centralStoragePath = storage_path(); $path = "{$centralStoragePath}/framework/cache/foo"; $lockPath = "{$centralStoragePath}/framework/cache/foo_locks"; // Delete the directories before running assertions, not just after. - // Cache::lock() below defaults to a lock that never expires, + // Cache::store('foo_file')->lock(...) below defaults to a lock that never expires, // and this test never releases it, so a stale lock left over from // a previous run would make lock() fail forever. File::deleteDirectory($path); @@ -519,31 +494,39 @@ test('a configured lock_path is scoped separately from path', function () { $tenant = Tenant::create(); tenancy()->initialize($tenant); - expect(Cache::store('foo_file')->lock('foo')->get())->toBeTrue(); - $tenantPath = "{$centralStoragePath}/tenant{$tenant->id}/framework/cache/foo"; $tenantLockPath = "{$centralStoragePath}/tenant{$tenant->id}/framework/cache/foo_locks"; - // Taking the lock creates the tenant's scoped lock_path directory. - // Nothing wrote a cache entry, so the tenant's scoped path directory doesn't exist (path and lock_path are scoped separately). + // lock('foo')->get() acquires the lock and creates the lock file at $tenantLockPath + expect(Cache::store('foo_file')->lock('foo')->get())->toBeTrue(); + expect(File::isDirectory($tenantLockPath))->toBeTrue(); + // The lock file was created at $tenantLockPath, not $tenantPath expect(File::isDirectory($tenantPath))->toBeFalse(); tenancy()->end(); + // The lock was only acquired in the tenant context, so in the central context, + // it's free (and the lock file doesn't exist in the central context). + expect(File::isDirectory($lockPath))->toBeFalse(); + // Acquire the lock in the central context expect(Cache::store('foo_file')->lock('foo')->get())->toBeTrue(); - - // After reverting, locks go back to the configured central lock_path. + // The lock file was created at the original (central) lock_path expect(File::isDirectory($lockPath))->toBeTrue(); File::deleteDirectory($path); File::deleteDirectory($lockPath); }); -test('a cache store without a configured lock_path is scoped without error', function () { - // lock_path is optional -- Laravel falls back to using path for locks when it's not set. - // This test covers that path through scopeCache() specifically (which none of the other tests exercise). - $path = storage_path('framework/cache/foo'); +test('a file cache store without a configured lock_path defaults to using its scoped path for locks', function () { + // 'lock_path' is optional in the file store config. + // FileStore falls back to using 'path' for locks when 'lock_path' is not specified in the config (or set to null). + // scopeCache() respects the original config and leaves lock_path unset/null. + $centralStoragePath = storage_path(); + $path = "{$centralStoragePath}/framework/cache/foo"; + + // $path is hardcoded here. The directory created at $path in a previous run can persist, + // so delete the directory at $path first. File::deleteDirectory($path); config([ @@ -554,28 +537,43 @@ test('a cache store without a configured lock_path is scoped without error', fun 'cache.stores.foo_file' => [ 'driver' => 'file', 'path' => $path, - // No 'lock_path' key at all. + // 'lock_path' not set (same behavior as if it were set to null) ], ]); - tenancy()->initialize(Tenant::create()); + $tenant = Tenant::create(); + tenancy()->initialize($tenant); - expect(Cache::store('foo_file')->put('key', 'tenant'))->toBeTrue(); + $tenantPath = "{$centralStoragePath}/tenant{$tenant->id}/framework/cache/foo"; + + // Initializing tenancy scopes the store's 'path', but respects the configured + // (unset) 'lock_path' and leaves it alone. + expect(config('cache.stores.foo_file.path'))->toBe($tenantPath); + expect(config('cache.stores.foo_file.lock_path'))->toBeNull(); + + // With no 'lock_path' configured, the lock file will be created in the scoped 'path' directory + expect(File::isDirectory($tenantPath))->toBeFalse(); expect(Cache::store('foo_file')->lock('foo')->get())->toBeTrue(); + expect(File::isDirectory($tenantPath))->toBeTrue(); + + // The lock is still held in the tenant context, so acquiring it again fails + expect(Cache::store('foo_file')->lock('foo')->get())->toBeFalse(); tenancy()->end(); - expect(Cache::store('foo_file')->put('key', 'central'))->toBeTrue(); + // The same 'foo' lock is free in central context. + // revert() points the 'path' back to the original. + expect(File::isDirectory($path))->toBeFalse(); expect(Cache::store('foo_file')->lock('foo')->get())->toBeTrue(); + expect(File::isDirectory($path))->toBeTrue(); + // Clean up directory created at the hardcoded path File::deleteDirectory($path); }); -test('a cache store using a path not based on storage_path() is suffixed in place, not moved under the tenant storage path', function () { - // $path below doesn't start with the central storage path, so tenantCachePath() has no storage - // path prefix to swap for the tenant's -- it appends the tenant suffix directly to $path instead. - // Check that tenant isolation still works for a store configured like this, and that its cache - // ends up at $path itself, not under the tenant's storage path. +test('a cache store using a path not based on storage_path() has the tenant suffix appended', function () { + // tenantCachePath() has no central storage path prefix to swap for the tenant's here, + // so it appends the tenant suffix to $path instead. $path = '/tmp/tenancy-cache-test'; File::deleteDirectory($path); @@ -587,7 +585,6 @@ test('a cache store using a path not based on storage_path() is suffixed in plac 'cache.stores.foo_file' => [ 'driver' => 'file', 'path' => $path, - 'lock_path' => $path, ], ]); @@ -603,11 +600,9 @@ test('a cache store using a path not based on storage_path() is suffixed in plac tenancy()->initialize($tenant1); expect(Cache::store('foo_file')->get('key'))->toBe('tenant1'); + // Tenant's 'foo_file' cache directory is created at $path with + // the tenant suffix appended, not at the tenant-scoped storage_path(). expect(File::isDirectory("{$path}/tenant{$tenant1->id}"))->toBeTrue(); - - // storage_path() is already scoped to tenant1 here (storagePath() runs before scopeCache() in - // bootstrap()), so this is the tenant's default cache directory. Nothing should be there, since - // foo_file is configured with its own $path that isn't storage_path()-based. expect(File::isDirectory(storage_path('framework/cache/data')))->toBeFalse(); tenancy()->end();