Resolve misc to-dos (#26)

* Resolve delete tenant storage todo

* Delete outdated todo (resolved in #25)

* Delete resource syncing todo (resolved in #11)

* Make it clear that getArgsForTenant() is used during cache invalidation

* Delete redundant __call() and __callStatic() annotations

* Fix code style (php-cs-fixer)

* Revert %tenant_id% to-do removal

* Test all cached resolvers instead of just the domain one

* Make docblock more concise, delete renaming to-do (the name seems fine)

* Fix method in tests

* If route is the only resolver arg, use the tenant as the cache key instead of encoded route instance

* Resolve to-do

* make docblock more clear

* Add comments to getResolverArgument()

* Rename $id to $tenantKey

* Fix code style (php-cs-fixer)

* Add regression test for forgetting tenant parameters of cached tenants

* Forget route parameter when tenant gets resolved

* Add parameter type

* Simplify getCacheKey()

* Resolvers wip

* Resolvers wip

* Fix code style (php-cs-fixer)

* Bring back the route instance check to getCacheKey, fix test

* add todo

* add assertion

---------

Co-authored-by: PHP CS Fixer <phpcsfixer@example.com>
Co-authored-by: Samuel Štancl <samuel.stancl@gmail.com>
Co-authored-by: Samuel Štancl <samuel@archte.ch>

src/Contracts/Domain.php

@@ -11,8 +11,6 @@ use Illuminate\Database\Eloquent\Relations\BelongsTo;
  *
  * @see \Stancl\Tenancy\Database\Models\Domain
  *
- * @method __call(string $method, array $parameters) IDE support. This will be a model. // todo check if we can remove these now
- * @method static __callStatic(string $method, array $parameters) IDE support. This will be a model.
  * @mixin \Illuminate\Database\Eloquent\Model
  */
 interface Domain

src/Database/DatabaseConfig.php

@@ -162,10 +162,7 @@ class DatabaseConfig
     }
 
     /**
-     * Purge host database connection.
+     * Purge the previous tenant connection before opening it for another tenant.
-     *
-     * It's possible database has previous tenant connection.
-     * This will clean up the previous connection before creating it for the current tenant.
      */
     public function purgeHostConnection(): void
     {
@@ -205,7 +202,7 @@ class DatabaseConfig
     public function manager(): Contracts\TenantDatabaseManager
     {
         // Laravel caches the previous PDO connection, so we purge it to be able to change the connection details
-        $this->purgeHostConnection(); // todo come up with a better name
+        $this->purgeHostConnection();
 
         // Create the tenant host connection config
         $tenantHostConnectionName = $this->getTenantHostConnectionName();

src/Listeners/DeleteTenantStorage.php

@@ -11,9 +11,6 @@ class DeleteTenantStorage
 {
     public function handle(DeletingTenant $event): void
     {
-        // todo@lukas since this is using the 'File' facade instead of low-level PHP functions, Tenancy might affect this?
-        //            Therefore, when Tenancy is initialized, this might look INSIDE the tenant's storage, instead of the main storage dir?
-        //            The DeletingTenant event will be fired in the central context in 99% of cases, but sometimes it might run in the tenant context (from another tenant) so we want to make sure this works well in all contexts.
         File::deleteDirectory($event->tenant->run(fn () => storage_path()));
     }
 }

src/Listeners/UpdateSyncedResource.php

@@ -16,8 +16,6 @@ use Stancl\Tenancy\Events\SyncedResourceSaved;
 use Stancl\Tenancy\Exceptions\ModelNotSyncMasterException;
 use Stancl\Tenancy\Tenancy;
 
-// todo@v4 review all code related to resource syncing
-
 class UpdateSyncedResource extends QueueableListener
 {
     public static bool $shouldQueue = false;

src/Resolvers/Contracts/CachedTenantResolver.php

@@ -62,11 +62,13 @@ abstract class CachedTenantResolver implements TenantResolver
     }
 
     /**
-     * Get all the arg combinations for resolve() that can be used to find this tenant.
+     * Get all possible argument combinations for resolve() which can be used for caching the tenant.
+     *
+     * This is used during tenant cache invalidation.
      *
      * @return array[]
      */
-    abstract public function getArgsForTenant(Tenant $tenant): array; // todo@v4 make it clear that this is only used for cache *invalidation*
+    abstract public function getArgsForTenant(Tenant $tenant): array;
 
     public static function shouldCache(): bool
     {

src/Resolvers/PathTenantResolver.php

@@ -7,6 +7,7 @@ namespace Stancl\Tenancy\Resolvers;
 use Illuminate\Routing\Route;
 use Stancl\Tenancy\Contracts\Tenant;
 use Stancl\Tenancy\Exceptions\TenantCouldNotBeIdentifiedByPathException;
+use Stancl\Tenancy\PathIdentificationManager;
 
 class PathTenantResolver extends Contracts\CachedTenantResolver
 {
@@ -37,6 +38,26 @@ class PathTenantResolver extends Contracts\CachedTenantResolver
         ];
     }
 
+    public function resolved(Tenant $tenant, mixed ...$args): void
+    {
+        /** @var Route $route */
+        $route = $args[0];
+
+        $route->forgetParameter(PathIdentificationManager::getTenantParameterName());
+    }
+
+    public function getCacheKey(mixed ...$args): string
+    {
+        // todo@samuel: fix the coupling here. when this is called from the cachedresolver, $args are the tenant key. when it's called from within this class, $args are a Route instance
+        // the logic shouldn't have to be coupled to where it's being called from
+
+        // $args[0] can be either a Route instance with the tenant key as a parameter
+        // Or the tenant key
+        $args = [$args[0] instanceof Route ? $args[0]->parameter(static::tenantParameterName()) : $args[0]];
+
+        return '_tenancy_resolver:' . static::class . ':' . json_encode($args);
+    }
+
     public static function tenantParameterName(): string
     {
         return config('tenancy.identification.resolvers.' . static::class . '.tenant_parameter_name') ?? 'tenant';

tests/CachedTenantResolverTest.php

@@ -2,73 +2,88 @@
 
 declare(strict_types=1);
 
+use Illuminate\Routing\Route;
 use Illuminate\Support\Facades\DB;
-use Stancl\Tenancy\Resolvers\DomainTenantResolver;
 use Stancl\Tenancy\Tests\Etc\Tenant;
+use Stancl\Tenancy\Resolvers\PathTenantResolver;
+use Stancl\Tenancy\Resolvers\DomainTenantResolver;
+use Illuminate\Support\Facades\Route as RouteFacade;
+use Stancl\Tenancy\Middleware\InitializeTenancyByPath;
+use Stancl\Tenancy\PathIdentificationManager;
+use Stancl\Tenancy\Resolvers\RequestDataTenantResolver;
 
-// todo@v4 test this with other resolvers as well?
+test('tenants can be resolved using cached resolvers', function (string $resolver) {
+    $tenant = Tenant::create(['id' => $tenantKey = 'acme']);
 
-test('tenants can be resolved using the cached resolver', function () {
+    $tenant->domains()->create(['domain' => $tenantKey]);
-    $tenant = Tenant::create();
-    $tenant->domains()->create([
-        'domain' => 'acme',
-    ]);
 
-    expect($tenant->is(app(DomainTenantResolver::class)->resolve('acme')))->toBeTrue()->toBeTrue();
+    expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenantKey))))->toBeTrue();
-});
+})->with([
+    DomainTenantResolver::class,
+    PathTenantResolver::class,
+    RequestDataTenantResolver::class,
+]);
 
-test('the underlying resolver is not touched when using the cached resolver', function () {
+test('the underlying resolver is not touched when using the cached resolver', function (string $resolver) {
-    $tenant = Tenant::create();
+    $tenant = Tenant::create(['id' => $tenantKey = 'acme']);
-    $tenant->domains()->create([
+
-        'domain' => 'acme',
+    $tenant->createDomain($tenantKey);
-    ]);
 
     DB::enableQueryLog();
 
-    config(['tenancy.identification.resolvers.' . DomainTenantResolver::class . '.cache' => false]);
+    config(['tenancy.identification.resolvers.' . $resolver . '.cache' => false]);
 
-    expect($tenant->is(app(DomainTenantResolver::class)->resolve('acme')))->toBeTrue();
+    expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenantKey))))->toBeTrue();
     DB::flushQueryLog();
-    expect($tenant->is(app(DomainTenantResolver::class)->resolve('acme')))->toBeTrue();
+    expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenantKey))))->toBeTrue();
+
     pest()->assertNotEmpty(DB::getQueryLog()); // not empty
 
-    config(['tenancy.identification.resolvers.' . DomainTenantResolver::class . '.cache' => true]);
+    config(['tenancy.identification.resolvers.' . $resolver . '.cache' => true]);
 
-    expect($tenant->is(app(DomainTenantResolver::class)->resolve('acme')))->toBeTrue();
+    expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenantKey))))->toBeTrue();
     DB::flushQueryLog();
-    expect($tenant->is(app(DomainTenantResolver::class)->resolve('acme')))->toBeTrue();
+    expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenantKey))))->toBeTrue();
     expect(DB::getQueryLog())->toBeEmpty(); // empty
-});
+})->with([
+    DomainTenantResolver::class,
+    PathTenantResolver::class,
+    RequestDataTenantResolver::class,
+]);
 
-test('cache is invalidated when the tenant is updated', function () {
+test('cache is invalidated when the tenant is updated', function (string $resolver) {
-    $tenant = Tenant::create();
+    $tenant = Tenant::create(['id' => $tenantKey = 'acme']);
-    $tenant->createDomain([
+    $tenant->createDomain($tenantKey);
-        'domain' => 'acme',
-    ]);
 
     DB::enableQueryLog();
 
-    config(['tenancy.identification.resolvers.' . DomainTenantResolver::class . '.cache' => true]);
+    config(['tenancy.identification.resolvers.' . $resolver . '.cache' => true]);
 
-    expect($tenant->is(app(DomainTenantResolver::class)->resolve('acme')))->toBeTrue();
+    expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenantKey))))->toBeTrue();
-    DB::flushQueryLog();
+    expect(DB::getQueryLog())->not()->toBeEmpty();
-    expect($tenant->is(app(DomainTenantResolver::class)->resolve('acme')))->toBeTrue();
-    expect(DB::getQueryLog())->toBeEmpty(); // empty
-
-    $tenant->update([
-        'foo' => 'bar',
-    ]);
 
     DB::flushQueryLog();
-    expect($tenant->is(app(DomainTenantResolver::class)->resolve('acme')))->toBeTrue();
+
-    pest()->assertNotEmpty(DB::getQueryLog()); // not empty
+    expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenantKey))))->toBeTrue();
-});
+    expect(DB::getQueryLog())->toBeEmpty();
+
+    // Tenant cache gets invalidated when the tenant is updated
+    $tenant->touch();
+
+    DB::flushQueryLog();
+
+    expect($tenant->is(app($resolver)->resolve(getResolverArgument($resolver, $tenantKey))))->toBeTrue();
+
+    expect(DB::getQueryLog())->not()->toBeEmpty(); // Cache was invalidated, so the tenant was retrievevd from the DB
+})->with([
+    DomainTenantResolver::class,
+    PathTenantResolver::class,
+    RequestDataTenantResolver::class,
+]);
 
 test('cache is invalidated when a tenants domain is changed', function () {
-    $tenant = Tenant::create();
+    $tenant = Tenant::create(['id' => $tenantKey = 'acme']);
-    $tenant->createDomain([
+    $tenant->createDomain($tenantKey);
-        'domain' => 'acme',
-    ]);
 
     DB::enableQueryLog();
 
@@ -91,3 +106,59 @@ test('cache is invalidated when a tenants domain is changed', function () {
     expect($tenant->is(app(DomainTenantResolver::class)->resolve('bar')))->toBeTrue();
     pest()->assertNotEmpty(DB::getQueryLog()); // not empty
 });
+
+test('PathTenantResolver forgets the tenant route parameter when the tenant is resolved from cache', function() {
+    config(['tenancy.identification.resolvers.' . PathTenantResolver::class . '.cache' => true]);
+
+    Tenant::create(['id' => 'foo']);
+
+    RouteFacade::get('/', fn () => request()->route()->parameter('tenant') ? 'Tenant parameter present' : 'No tenant parameter')
+        ->name('tenant-route')
+        ->prefix('{tenant}')
+        ->middleware(InitializeTenancyByPath::class);
+
+    // Tenant gets cached on first request
+    pest()->get("/foo")->assertSee('No tenant parameter');
+
+    // Tenant is resolved from cache on second request
+    // The tenant parameter should be forgotten
+    DB::flushQueryLog();
+    pest()->get("/foo")->assertSee('No tenant parameter');
+    pest()->assertEmpty(DB::getQueryLog()); // resolved from cache
+});
+
+/**
+ * Return the argument for the resolver – tenant key, or a route instance with the tenant parameter.
+ *
+ * PathTenantResolver uses a route instance with the tenant parameter as the argument,
+ * unlike other resolvers which use a tenant key as the argument.
+ *
+ * This method is used in the tests where we test all the resolvers
+ * to make getting the resolver arguments less repetitive (primarily because of the PathTenantResolver).
+ */
+function getResolverArgument(string $resolver, string $tenantKey): string|Route
+{
+    // PathTenantResolver uses a route instance as the argument
+    if ($resolver === PathTenantResolver::class) {
+        $routeName = 'tenant-route';
+
+        // Find or create a route instance for the resolver
+        $route = RouteFacade::getRoutes()->getByName($routeName) ?? RouteFacade::get('/', fn () => null)
+            ->name($routeName)
+            ->prefix('{tenant}')
+            ->middleware(InitializeTenancyByPath::class);
+
+        // To make the tenant available on the route instance
+        // Make the 'tenant' route parameter the tenant key
+        // Setting the parameter on the $route->parameters property is required
+        // Because $route->setParameter() throws an exception when $route->parameters is not set yet
+        $route->parameters[PathIdentificationManager::getTenantParameterName()] = $tenantKey;
+
+        // Return the route instance with the tenant key as the 'tenant' parameter
+        return $route;
+    }
+
+    // Resolvers other than PathTenantResolver use the tenant key as the argument
+    // Return the tenant key
+    return $tenantKey;
+}