From 4578c9ed7d87d2392331728e4a8d1f4e9292b6cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Samuel=20=C5=A0tancl?= Date: Sun, 31 Aug 2025 23:14:07 +0200 Subject: [PATCH] Features refactor Features are now *always* bootstrapped, even if Tenancy is not resolved from the container. Previous implementations include https://github.com/tenancy-for-laravel/v4/pull/19 https://github.com/archtechx/tenancy/pull/1021 Bug originally reported here https://github.com/archtechx/tenancy/issues/949 This implementation is much simpler, we do not distinguish between features that should be "always bootstrapped" and features that should only be bootstrapped after Tenancy is resolved. All features should work without issues if they're bootstrapped when TSP::boot() is called. We also add a Tenancy::bootstrapFeatures() method that can be used to bootstrap any features dynamically added at runtime that weren't bootstrapped in TSP::boot(). The function keeps track of which features were already bootstrapped so it doesn't bootstrap them again. The only potentialy risky thing in this implementation is that we're now resolving Tenancy in TSP::boot() (previously Tenancy was not being resolved) but that shouldn't be causing any issues. --- src/Contracts/Feature.php | 4 +- .../SQLiteDatabaseManager.php | 2 + src/Features/CrossDomainRedirect.php | 3 +- src/Features/DisallowSqliteAttach.php | 7 +++- src/Features/TelescopeTags.php | 3 +- src/Features/TenantConfig.php | 3 +- src/Features/UserImpersonation.php | 4 +- src/Features/ViteBundler.php | 13 ++---- src/Tenancy.php | 41 ++++++++++++++++++- src/TenancyServiceProvider.php | 14 +++---- tests/Features/NoAttachTest.php | 2 +- tests/Features/RedirectTest.php | 2 + tests/Features/TenantConfigTest.php | 24 +++++------ tests/Features/ViteBundlerTest.php | 1 + tests/TenantUserImpersonationTest.php | 2 + 15 files changed, 80 insertions(+), 45 deletions(-) diff --git a/src/Contracts/Feature.php b/src/Contracts/Feature.php index 74289981..25363cf5 100644 --- a/src/Contracts/Feature.php +++ b/src/Contracts/Feature.php @@ -4,10 +4,8 @@ declare(strict_types=1); namespace Stancl\Tenancy\Contracts; -use Stancl\Tenancy\Tenancy; - /** Additional features, like Telescope tags and tenant redirects. */ interface Feature { - public function bootstrap(Tenancy $tenancy): void; + public function bootstrap(): void; } diff --git a/src/Database/TenantDatabaseManagers/SQLiteDatabaseManager.php b/src/Database/TenantDatabaseManagers/SQLiteDatabaseManager.php index 64b96fc1..818539c2 100644 --- a/src/Database/TenantDatabaseManagers/SQLiteDatabaseManager.php +++ b/src/Database/TenantDatabaseManagers/SQLiteDatabaseManager.php @@ -94,6 +94,7 @@ class SQLiteDatabaseManager implements TenantDatabaseManager return false; } + // todo@sqlite we can just respect Laravel config for WAL now if (static::$WAL) { $pdo = new PDO('sqlite:' . $path); $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); @@ -123,6 +124,7 @@ class SQLiteDatabaseManager implements TenantDatabaseManager } try { + // todo@sqlite we should also remove any other files for the DB e.g. WAL return unlink($this->getPath($name)); } catch (Throwable) { return false; diff --git a/src/Features/CrossDomainRedirect.php b/src/Features/CrossDomainRedirect.php index a48be6ea..57786274 100644 --- a/src/Features/CrossDomainRedirect.php +++ b/src/Features/CrossDomainRedirect.php @@ -6,11 +6,10 @@ namespace Stancl\Tenancy\Features; use Illuminate\Http\RedirectResponse; use Stancl\Tenancy\Contracts\Feature; -use Stancl\Tenancy\Tenancy; class CrossDomainRedirect implements Feature { - public function bootstrap(Tenancy $tenancy): void + public function bootstrap(): void { RedirectResponse::macro('domain', function (string $domain) { /** @var RedirectResponse $this */ diff --git a/src/Features/DisallowSqliteAttach.php b/src/Features/DisallowSqliteAttach.php index f428a051..16dd47f0 100644 --- a/src/Features/DisallowSqliteAttach.php +++ b/src/Features/DisallowSqliteAttach.php @@ -10,14 +10,13 @@ use Illuminate\Database\SQLiteConnection; use Illuminate\Support\Facades\DB; use PDO; use Stancl\Tenancy\Contracts\Feature; -use Stancl\Tenancy\Tenancy; class DisallowSqliteAttach implements Feature { protected static bool|null $loadExtensionSupported = null; public static string|false|null $extensionPath = null; - public function bootstrap(Tenancy $tenancy): void + public function bootstrap(): void { // Handle any already resolved connections foreach (DB::getConnections() as $connection) { @@ -40,16 +39,20 @@ class DisallowSqliteAttach implements Feature protected function loadExtension(PDO $pdo): bool { if (static::$loadExtensionSupported === null) { + // todo@sqlite refactor to local static static::$loadExtensionSupported = method_exists($pdo, 'loadExtension'); } if (static::$loadExtensionSupported === false) { return false; } + if (static::$extensionPath === false) { return false; } + // todo@sqlite we may want to check for 64 bit + $suffix = match (PHP_OS_FAMILY) { 'Linux' => 'so', 'Windows' => 'dll', diff --git a/src/Features/TelescopeTags.php b/src/Features/TelescopeTags.php index 0a580d23..225049df 100644 --- a/src/Features/TelescopeTags.php +++ b/src/Features/TelescopeTags.php @@ -7,11 +7,10 @@ namespace Stancl\Tenancy\Features; use Laravel\Telescope\IncomingEntry; use Laravel\Telescope\Telescope; use Stancl\Tenancy\Contracts\Feature; -use Stancl\Tenancy\Tenancy; class TelescopeTags implements Feature { - public function bootstrap(Tenancy $tenancy): void + public function bootstrap(): void { if (! class_exists(Telescope::class)) { return; diff --git a/src/Features/TenantConfig.php b/src/Features/TenantConfig.php index 5bc84060..10283da3 100644 --- a/src/Features/TenantConfig.php +++ b/src/Features/TenantConfig.php @@ -12,7 +12,6 @@ use Stancl\Tenancy\Contracts\Feature; use Stancl\Tenancy\Contracts\Tenant; use Stancl\Tenancy\Events\RevertedToCentralContext; use Stancl\Tenancy\Events\TenancyBootstrapped; -use Stancl\Tenancy\Tenancy; class TenantConfig implements Feature { @@ -27,7 +26,7 @@ class TenantConfig implements Feature protected Repository $config, ) {} - public function bootstrap(Tenancy $tenancy): void + public function bootstrap(): void { Event::listen(TenancyBootstrapped::class, function (TenancyBootstrapped $event) { /** @var Tenant $tenant */ diff --git a/src/Features/UserImpersonation.php b/src/Features/UserImpersonation.php index 3db563a4..ac478d07 100644 --- a/src/Features/UserImpersonation.php +++ b/src/Features/UserImpersonation.php @@ -17,9 +17,9 @@ class UserImpersonation implements Feature /** The lifespan of impersonation tokens (in seconds). */ public static int $ttl = 60; - public function bootstrap(Tenancy $tenancy): void + public function bootstrap(): void { - $tenancy->macro('impersonate', function (Tenant $tenant, string $userId, string $redirectUrl, string|null $authGuard = null, bool $remember = false): Model { + Tenancy::macro('impersonate', function (Tenant $tenant, string $userId, string $redirectUrl, string|null $authGuard = null, bool $remember = false): Model { return UserImpersonation::modelClass()::create([ Tenancy::tenantKeyColumn() => $tenant->getTenantKey(), 'user_id' => $userId, diff --git a/src/Features/ViteBundler.php b/src/Features/ViteBundler.php index 987187c7..003984f7 100644 --- a/src/Features/ViteBundler.php +++ b/src/Features/ViteBundler.php @@ -7,19 +7,14 @@ namespace Stancl\Tenancy\Features; use Illuminate\Foundation\Application; use Illuminate\Support\Facades\Vite; use Stancl\Tenancy\Contracts\Feature; -use Stancl\Tenancy\Tenancy; class ViteBundler implements Feature { - /** @var Application */ - protected $app; + public function __construct( + protected Application $app, + ) {} - public function __construct(Application $app) - { - $this->app = $app; - } - - public function bootstrap(Tenancy $tenancy): void + public function bootstrap(): void { Vite::createAssetPathsUsing(function ($path, $secure = null) { return global_asset($path); diff --git a/src/Tenancy.php b/src/Tenancy.php index 86eb6d8d..81e9b1ea 100644 --- a/src/Tenancy.php +++ b/src/Tenancy.php @@ -11,6 +11,7 @@ use Illuminate\Foundation\Bus\PendingDispatch; use Illuminate\Support\Traits\Macroable; use Stancl\Tenancy\Concerns\DealsWithRouteContexts; use Stancl\Tenancy\Concerns\ManagesRLSPolicies; +use Stancl\Tenancy\Contracts\Feature; use Stancl\Tenancy\Contracts\TenancyBootstrapper; use Stancl\Tenancy\Contracts\Tenant; use Stancl\Tenancy\Exceptions\TenantCouldNotBeIdentifiedByIdException; @@ -40,7 +41,7 @@ class Tenancy public static array $findWith = []; /** - * A list of bootstrappers that have been initialized. + * List of bootstrappers that have been initialized. * * This is used when reverting tenancy, mainly if an exception * occurs during bootstrapping, to ensure we don't revert @@ -53,6 +54,23 @@ class Tenancy */ public array $initializedBootstrappers = []; + /** + * List of features that have been bootstrapped. + * + * Since features may be bootstrapped multiple times during + * the request cycle (in TSP::boot() and any other times the user calls + * bootstrapFeatures()), we keep track of which features have already + * been bootstrapped so we do not bootstrap them again. Features are + * bootstrapped once and irreversible. + * + * The main point of this is that some features *need* to be bootstrapped + * very early (see #949), so we bootstrap them directly in TSP, but we + * also need the ability to *change* which features are used at runtime + * (mainly tests of this package) and bootstrap features again after making + * changes to config('tenancy.features'). + */ + protected array $bootstrappedFeatures = []; + /** Initialize tenancy for the passed tenant. */ public function initialize(Tenant|int|string $tenant): void { @@ -154,6 +172,27 @@ class Tenancy return in_array($bootstrapper, static::getBootstrappers(), true); } + /** + * Bootstrap configured Tenancy features. + * + * Normally, features are bootstrapped directly in TSP::boot(). However, if + * new features are enabled at runtime (e.g. during tests), this method may + * be called to bootstrap new features. It's idempotent and keeps track of + * which features have already been bootstrapped. Keep in mind that feature + * bootstrapping is irreversible. + */ + public function bootstrapFeatures(): void + { + foreach (config('tenancy.features') ?? [] as $feature) { + /** @var class-string $feature */ + + if (! in_array($feature, $this->bootstrappedFeatures)) { + app($feature)->bootstrap(); + $this->bootstrappedFeatures[] = $feature; + } + } + } + /** * @return Builder */ diff --git a/src/TenancyServiceProvider.php b/src/TenancyServiceProvider.php index 557306b2..a7f27e63 100644 --- a/src/TenancyServiceProvider.php +++ b/src/TenancyServiceProvider.php @@ -40,15 +40,6 @@ class TenancyServiceProvider extends ServiceProvider // Make sure Tenancy is stateful. $this->app->singleton(Tenancy::class); - // Make sure features are bootstrapped as soon as Tenancy is instantiated. - $this->app->extend(Tenancy::class, function (Tenancy $tenancy) { - foreach ($this->app['config']['tenancy.features'] ?? [] as $feature) { - $this->app[$feature]->bootstrap($tenancy); - } - - return $tenancy; - }); - // Make it possible to inject the current tenant by type hinting the Tenant contract. $this->app->bind(Tenant::class, function ($app) { return $app[Tenancy::class]->tenant; @@ -176,6 +167,11 @@ class TenancyServiceProvider extends ServiceProvider return $instance; }); + // Bootstrap features that are already enabled in the config. + // If more features are enabled at runtime, this method may be called + // multiple times, it keeps track of which features have already been bootstrapped. + $this->app->make(Tenancy::class)->bootstrapFeatures(); + Route::middlewareGroup('clone', []); Route::middlewareGroup('universal', []); Route::middlewareGroup('tenant', []); diff --git a/tests/Features/NoAttachTest.php b/tests/Features/NoAttachTest.php index a1588a24..1ec62f2a 100644 --- a/tests/Features/NoAttachTest.php +++ b/tests/Features/NoAttachTest.php @@ -63,7 +63,7 @@ test('sqlite ATTACH statements can be blocked', function (bool $disallow) { return json_encode(DB::select(request('q2'))); }); - tenancy(); // trigger features: todo@samuel remove after feature refactor + tenancy()->bootstrapFeatures(); if ($disallow) { expect(fn () => pest()->post('/central-sqli', [ diff --git a/tests/Features/RedirectTest.php b/tests/Features/RedirectTest.php index a4102070..a871f529 100644 --- a/tests/Features/RedirectTest.php +++ b/tests/Features/RedirectTest.php @@ -12,6 +12,8 @@ test('tenant redirect macro replaces only the hostname', function () { 'tenancy.features' => [CrossDomainRedirect::class], ]); + tenancy()->bootstrapFeatures(); + Route::get('/foobar', function () { return 'Foo'; })->name('home'); diff --git a/tests/Features/TenantConfigTest.php b/tests/Features/TenantConfigTest.php index b06ddba9..cef5efbf 100644 --- a/tests/Features/TenantConfigTest.php +++ b/tests/Features/TenantConfigTest.php @@ -11,16 +11,21 @@ use Stancl\Tenancy\Listeners\RevertToCentralContext; use Stancl\Tenancy\Tests\Etc\Tenant; use function Stancl\Tenancy\Tests\pest; +beforeEach(function () { + config([ + 'tenancy.features' => [TenantConfig::class], + 'tenancy.bootstrappers' => [], + ]); + + tenancy()->bootstrapFeatures(); +}); + afterEach(function () { TenantConfig::$storageToConfigMap = []; }); test('nested tenant values are merged', function () { expect(config('whitelabel.theme'))->toBeNull(); - config([ - 'tenancy.features' => [TenantConfig::class], - 'tenancy.bootstrappers' => [], - ]); Event::listen(TenancyInitialized::class, BootstrapTenancy::class); Event::listen(TenancyEnded::class, RevertToCentralContext::class); @@ -32,6 +37,9 @@ test('nested tenant values are merged', function () { 'whitelabel' => ['config' => ['theme' => 'dark']], ]); + // todo0 one reason why this fails is that tenancy.features was empty + // at register() time + tenancy()->initialize($tenant); expect(config('whitelabel.theme'))->toBe('dark'); tenancy()->end(); @@ -39,10 +47,6 @@ test('nested tenant values are merged', function () { test('config is merged and removed', function () { expect(config('services.paypal'))->toBe(null); - config([ - 'tenancy.features' => [TenantConfig::class], - 'tenancy.bootstrappers' => [], - ]); Event::listen(TenancyInitialized::class, BootstrapTenancy::class); Event::listen(TenancyEnded::class, RevertToCentralContext::class); @@ -68,10 +72,6 @@ test('config is merged and removed', function () { test('the value can be set to multiple config keys', function () { expect(config('services.paypal'))->toBe(null); - config([ - 'tenancy.features' => [TenantConfig::class], - 'tenancy.bootstrappers' => [], - ]); Event::listen(TenancyInitialized::class, BootstrapTenancy::class); Event::listen(TenancyEnded::class, RevertToCentralContext::class); diff --git a/tests/Features/ViteBundlerTest.php b/tests/Features/ViteBundlerTest.php index 3934698f..17ee8e08 100644 --- a/tests/Features/ViteBundlerTest.php +++ b/tests/Features/ViteBundlerTest.php @@ -27,6 +27,7 @@ beforeEach(function () { test('vite bundler ensures vite assets use global_asset when asset_helper_override is enabled', function () { config(['tenancy.features' => [ViteBundler::class]]); + tenancy()->bootstrapFeatures(); withBootstrapping(); diff --git a/tests/TenantUserImpersonationTest.php b/tests/TenantUserImpersonationTest.php index 8c9c4124..48fbe691 100644 --- a/tests/TenantUserImpersonationTest.php +++ b/tests/TenantUserImpersonationTest.php @@ -42,6 +42,8 @@ beforeEach(function () { ], ]); + tenancy()->bootstrapFeatures(); + Event::listen( TenantCreated::class, JobPipeline::make([CreateDatabase::class])->send(function (TenantCreated $event) {