[4.x] General code cleanup (#1278)

* Declare sensitive parameters as sensitive

... just so that they don't show up in logs

* Remove unnecessary null-coalescing

* Simplify return

* Merge isset() calls

* Inline return

* Use nullsafe operator

* Simplify if-else branches

* Use direct empty string comparison instead of strlen()

* Add missing type

* Change interface as events expect a TenantWithDatabase not just a Tenant

* Narrow typehint

* Remove redundant type casts

* Fix style with php-cs-fixer

* Fix typos

* Revert unwanted if-else simplification

* fix phpstan errors

* narrow type

---------

Co-authored-by: Samuel Štancl <samuel@archte.ch>

composer.json

@@ -73,8 +73,8 @@
         "testbench-link": "ln -s vendor ./vendor/orchestra/testbench-core/laravel/vendor",
         "testbench-repair": "mkdir -p ./vendor/orchestra/testbench-core/laravel/storage/framework/sessions && mkdir -p ./vendor/orchestra/testbench-core/laravel/storage/framework/views && mkdir -p ./vendor/orchestra/testbench-core/laravel/storage/framework/cache",
         "coverage": "open coverage/phpunit/html/index.html",
-        "phpstan": "vendor/bin/phpstan",
+        "phpstan": "vendor/bin/phpstan --memory-limit=256M",
-        "phpstan-pro": "vendor/bin/phpstan --pro",
+        "phpstan-pro": "vendor/bin/phpstan --memory-limit=256M --pro",
         "cs": "php-cs-fixer fix --config=.php-cs-fixer.php",
         "test": "./test --no-coverage",
         "test-full": "./test",

src/Actions/CloneRoutesAsTenant.php

@@ -120,14 +120,8 @@ class CloneRoutesAsTenant
             $pathIdentificationUsed = (! $routeHasNonPathIdentificationMiddleware) &&
                 ($routeHasPathIdentificationMiddleware || $pathIdentificationMiddlewareInGlobalStack);
 
-            if (
+            return $pathIdentificationUsed &&
-                $pathIdentificationUsed &&
+                (tenancy()->getRouteMode($route) === RouteMode::UNIVERSAL || tenancy()->routeHasMiddleware($route, 'clone'));
-                (tenancy()->getRouteMode($route) === RouteMode::UNIVERSAL || tenancy()->routeHasMiddleware($route, 'clone'))
-            ) {
-                return true;
-            }
-
-            return false;
         });
     }
 

src/Bootstrappers/BroadcastChannelPrefixBootstrapper.php

@@ -50,7 +50,7 @@ class BroadcastChannelPrefixBootstrapper implements TenancyBootstrapper
 
             $broadcasterOverride($this->broadcastManager);
 
-            // Get the overriden broadcaster
+            // Get the overridden broadcaster
             $newBroadcaster = $this->broadcastManager->driver($broadcaster);
 
             // Register the original broadcaster's channels in the new broadcaster
@@ -64,7 +64,7 @@ class BroadcastChannelPrefixBootstrapper implements TenancyBootstrapper
     {
         // Revert to the original broadcasters
         foreach ($this->originalBroadcasters as $name => $broadcaster) {
-            // Delete the cached (overriden) broadcaster
+            // Delete the cached (overridden) broadcaster
             $this->broadcastManager->purge($name);
 
             // Make manager return the original broadcaster instance

src/Bootstrappers/QueueTenancyBootstrapper.php

@@ -128,7 +128,7 @@ class QueueTenancyBootstrapper implements TenancyBootstrapper
         }
 
         // Revert back to the previous tenant
-        if (tenant() && $previousTenant && $previousTenant->isNot(tenant())) {
+        if (tenant() && $previousTenant?->isNot(tenant())) {
             tenancy()->initialize($previousTenant);
         }
 

src/Commands/CreateUserWithRLSPolicies.php

@@ -66,6 +66,7 @@ class CreateUserWithRLSPolicies extends Command
     protected function makeDatabaseConfig(
         PermissionControlledPostgreSQLSchemaManager $manager,
         string $username,
+        #[\SensitiveParameter]
         string $password,
     ): DatabaseConfig {
         /** @var TenantWithDatabase $tenantModel */

src/Commands/Migrate.php

@@ -81,7 +81,7 @@ class Migrate extends MigrateCommand
                 $tenant->run(function ($tenant) use (&$success) {
                     event(new MigratingDatabase($tenant));
 
-                    $verbosity = (int) $this->output->getVerbosity();
+                    $verbosity = $this->output->getVerbosity();
 
                     if ($this->runningConcurrently) {
                         // The output gets messy when multiple processes are writing to the same stdout

src/Commands/Rollback.php

@@ -76,7 +76,7 @@ class Rollback extends RollbackCommand
                 $tenant->run(function ($tenant) use (&$success) {
                     event(new RollingBackDatabase($tenant));
 
-                    $verbosity = (int) $this->output->getVerbosity();
+                    $verbosity = $this->output->getVerbosity();
 
                     if ($this->runningConcurrently) {
                         // The output gets messy when multiple processes are writing to the same stdout

src/Commands/Tinker.php

@@ -47,7 +47,7 @@ class Tinker extends BaseTinker
                 /** @var string $tenantKey */
                 $tenantKey = search(
                     'Enter the tenant key:',
-                    fn (string $search) => strlen($search) > 0
+                    fn (string $search) => $search !== ''
                         ? tenancy()->model()::where(tenancy()->model()->getTenantKeyName(), 'like', "$search%")->pluck(tenancy()->model()->getTenantKeyName())->all()
                         : []
                 );
@@ -57,7 +57,7 @@ class Tinker extends BaseTinker
                 /** @var string $domain */
                 $domain = search(
                     'Enter the tenant domain:',
-                    fn (string $search) => strlen($search) > 0
+                    fn (string $search) => $search !== ''
                         ? config('tenancy.models.domain')::where('domain', 'like', "$search%")->pluck('domain')->all()
                         : []
                 );

src/Concerns/DealsWithRouteContexts.php

@@ -46,7 +46,7 @@ trait DealsWithRouteContexts
      * the context is determined by the `tenancy.default_route_mode` config.
      *
      * If the default route mode is tenant, all unflagged routes will be tenant by default,
-     * but they will still have to have an identification midddleware (route-level
+     * but they will still have to have an identification middleware (route-level
      * or global) to be accessible. Same applies for universal default route mode.
      */
     public static function getRouteMode(Route $route): RouteMode

src/Concerns/ParallelCommand.php

@@ -14,7 +14,7 @@ use Symfony\Component\Console\Input\InputOption;
 
 trait ParallelCommand
 {
-    public const MAX_PROCESSES = 24;
+    public const int MAX_PROCESSES = 24;
     protected bool $runningConcurrently = false;
 
     abstract protected function childHandle(mixed ...$args): bool;
@@ -70,7 +70,7 @@ trait ParallelCommand
         // perflevel0 refers to P-cores on M-series, and the entire CPU on Intel Macs
         if ($darwin && $ffi->sysctlbyname('hw.perflevel0.logicalcpu', FFI::addr($cores), FFI::addr($size), null, 0) === 0) {
             return $cores->cdata;
-        } else if ($darwin) {
+        } elseif ($darwin) {
             // Reset the size in case the pointer got written to (likely shouldn't happen)
             $size->cdata = FFI::sizeof($cores);
         }
@@ -109,7 +109,7 @@ trait ParallelCommand
         if ($processes === null) {
             // This is used when the option is set but *without* a value (-p).
             $processes = $this->getLogicalCoreCount();
-        } else if ((int) $processes === -1) {
+        } elseif ((int) $processes === -1) {
             // Default value we set for the option -- this is used when the option is *not set*.
             $processes = 1;
         } else {

src/Controllers/TenantAssetController.php

@@ -88,12 +88,12 @@ class TenantAssetController implements HasMiddleware
             if (app()->runningUnitTests()) {
                 // Makes testing the cause of the failure in validatePath() easier
                 throw new Exception($exceptionMessage);
-            } else {
-                // We always use 404 to avoid leaking information about the cause of the error
-                // e.g. when someone is trying to access a nonexistent file outside of the allowed
-                // root folder, we don't want to let the user know whether such a file exists or not.
-                abort(404);
             }
+
+            // We always use 404 to avoid leaking information about the cause of the error
+            // e.g. when someone is trying to access a nonexistent file outside of the allowed
+            // root folder, we don't want to let the user know whether such a file exists or not.
+            abort(404);
         }
     }
 }

src/Database/DatabaseConfig.php

@@ -100,12 +100,12 @@ class DatabaseConfig
 
     public function getUsername(): ?string
     {
-        return $this->tenant->getInternal('db_username') ?? null;
+        return $this->tenant->getInternal('db_username');
     }
 
     public function getPassword(): ?string
     {
-        return $this->tenant->getInternal('db_password') ?? null;
+        return $this->tenant->getInternal('db_password');
     }
 
     /**

src/Exceptions/RouteIsMissingTenantParameterException.php

@@ -13,6 +13,6 @@ class RouteIsMissingTenantParameterException extends Exception
     {
         $parameter = PathTenantResolver::tenantParameterName();
 
-        parent::__construct("The route's first argument is not the tenant id (configured paramter name: $parameter).");
+        parent::__construct("The route's first argument is not the tenant id (configured parameter name: $parameter).");
     }
 }

src/Features/UserImpersonation.php

@@ -30,7 +30,7 @@ class UserImpersonation implements Feature
     }
 
     /** Impersonate a user and get an HTTP redirect response. */
-    public static function makeResponse(string|ImpersonationToken $token, ?int $ttl = null): RedirectResponse
+    public static function makeResponse(#[\SensitiveParameter] string|ImpersonationToken $token, ?int $ttl = null): RedirectResponse
     {
         /** @var ImpersonationToken $token */
         $token = $token instanceof ImpersonationToken ? $token : ImpersonationToken::findOrFail($token);

src/Middleware/InitializeTenancyByPath.php

@@ -43,9 +43,9 @@ class InitializeTenancyByPath extends IdentificationMiddleware
                 $next,
                 $route
             );
-        } else {
-            throw new RouteIsMissingTenantParameterException;
         }
+
+        throw new RouteIsMissingTenantParameterException;
     }
 
     /**

src/Middleware/InitializeTenancyByRequestData.php

@@ -96,9 +96,7 @@ class InitializeTenancyByRequestData extends IdentificationMiddleware
 
                 if (
                     is_array($data) &&
-                    isset($data['iv']) &&
+                    isset($data['iv'], $data['value'], $data['mac'])
-                    isset($data['value']) &&
-                    isset($data['mac'])
                 ) {
                     // We can confidently assert that the cookie is encrypted. If this call were to fail, this method would just
                     // return null and the cookie payload would get skipped.

src/RLS/PolicyManagers/TraitRLSManager.php

@@ -35,7 +35,7 @@ class TraitRLSManager implements RLSPolicyManager
      */
     public static bool $implicitRLS = false;
 
-    /** @var array<class-string<\Illuminate\Database\Eloquent\Model>> */
+    /** @var array<class-string<Model>> */
     public static array $excludedModels = [];
 
     public function generateQueries(): array
@@ -99,7 +99,7 @@ class TraitRLSManager implements RLSPolicyManager
      * Models are either discovered in the directories specified in static::$modelDirectories (by default),
      * or by a custom closure specified in static::$modelDiscoveryOverride.
      *
-     * @return array<\Illuminate\Database\Eloquent\Model>
+     * @return array<Model>
      */
     public function getModels(): array
     {

src/Resolvers/DomainTenantResolver.php

@@ -7,12 +7,12 @@ namespace Stancl\Tenancy\Resolvers;
 use Illuminate\Database\Eloquent\Builder;
 use Illuminate\Database\Eloquent\Model;
 use Illuminate\Database\Eloquent\Relations\Relation;
+use Illuminate\Support\Str;
 use Stancl\Tenancy\Contracts\Domain;
 use Stancl\Tenancy\Contracts\SingleDomainTenant;
 use Stancl\Tenancy\Contracts\Tenant;
 use Stancl\Tenancy\Exceptions\TenantCouldNotBeIdentifiedOnDomainException;
 use Stancl\Tenancy\Tenancy;
-use Illuminate\Support\Str;
 
 class DomainTenantResolver extends Contracts\CachedTenantResolver
 {

src/ResourceSyncing/ResourceSyncing.php

@@ -7,8 +7,8 @@ namespace Stancl\Tenancy\ResourceSyncing;
 use Illuminate\Database\Eloquent\Model;
 use Illuminate\Database\Eloquent\Relations\BelongsToMany;
 use Illuminate\Database\Eloquent\SoftDeletes;
-use Stancl\Tenancy\Contracts\Tenant;
 use Stancl\Tenancy\Contracts\UniqueIdentifierGenerator;
+use Stancl\Tenancy\Database\Contracts\TenantWithDatabase;
 use Stancl\Tenancy\ResourceSyncing\Events\CentralResourceAttachedToTenant;
 use Stancl\Tenancy\ResourceSyncing\Events\CentralResourceDetachedFromTenant;
 use Stancl\Tenancy\ResourceSyncing\Events\SyncedResourceSaved;
@@ -78,7 +78,7 @@ trait ResourceSyncing
     }
 
     /** Default implementation for \Stancl\Tenancy\ResourceSyncing\SyncMaster */
-    public function triggerAttachEvent(Tenant&Model $tenant): void
+    public function triggerAttachEvent(TenantWithDatabase&Model $tenant): void
     {
         if ($this instanceof SyncMaster) {
             /** @var SyncMaster&Model $this */
@@ -87,7 +87,7 @@ trait ResourceSyncing
     }
 
     /** Default implementation for \Stancl\Tenancy\ResourceSyncing\SyncMaster */
-    public function triggerDetachEvent(Tenant&Model $tenant): void
+    public function triggerDetachEvent(TenantWithDatabase&Model $tenant): void
     {
         if ($this instanceof SyncMaster) {
             /** @var SyncMaster&Model $this */

src/ResourceSyncing/SyncMaster.php

@@ -7,25 +7,25 @@ namespace Stancl\Tenancy\ResourceSyncing;
 use Illuminate\Database\Eloquent\Collection;
 use Illuminate\Database\Eloquent\Model;
 use Illuminate\Database\Eloquent\Relations\BelongsToMany;
-use Stancl\Tenancy\Contracts\Tenant;
+use Stancl\Tenancy\Database\Contracts\TenantWithDatabase;
 
 // todo@move move all resource syncing-related things to a separate namespace?
 
 /**
- * @property-read Tenant[]|Collection $tenants
+ * @property-read TenantWithDatabase[]|Collection<TenantWithDatabase> $tenants
  */
 interface SyncMaster extends Syncable
 {
     /**
-     * @return BelongsToMany<Tenant&Model, self&Model>
+     * @return BelongsToMany<TenantWithDatabase&Model, self&Model>
      */
     public function tenants(): BelongsToMany;
 
     public function getTenantModelName(): string;
 
-    public function triggerDetachEvent(Tenant&Model $tenant): void;
+    public function triggerDetachEvent(TenantWithDatabase&Model $tenant): void;
 
-    public function triggerAttachEvent(Tenant&Model $tenant): void;
+    public function triggerAttachEvent(TenantWithDatabase&Model $tenant): void;
 
     public function triggerDeleteEvent(bool $forceDelete = false): void;
 

src/ResourceSyncing/TriggerSyncingEvents.php

@@ -8,6 +8,7 @@ use Illuminate\Database\Eloquent\Model;
 use Illuminate\Database\Eloquent\Relations\MorphPivot;
 use Illuminate\Database\Eloquent\Relations\Pivot;
 use Stancl\Tenancy\Contracts\Tenant;
+use Stancl\Tenancy\Database\Contracts\TenantWithDatabase;
 
 /**
  * Used on pivot models.
@@ -30,7 +31,7 @@ trait TriggerSyncingEvents
             /**
              * @var static&Pivot $pivot
              * @var SyncMaster|null $centralResource
-             * @var (Tenant&Model)|null $tenant
+             * @var (TenantWithDatabase&Model)|null $tenant
              */
             [$centralResource, $tenant] = $pivot->getCentralResourceAndTenant();
 
@@ -43,7 +44,7 @@ trait TriggerSyncingEvents
             /**
              * @var static&Pivot $pivot
              * @var SyncMaster|null $centralResource
-             * @var (Tenant&Model)|null $tenant
+             * @var (TenantWithDatabase&Model)|null $tenant
              */
             [$centralResource, $tenant] = $pivot->getCentralResourceAndTenant();
 

src/Tenancy.php

@@ -141,9 +141,7 @@ class Tenancy
         /** @var class-string<Tenant&Model> $class */
         $class = config('tenancy.models.tenant');
 
-        $model = new $class;
+        return new $class;
-
-        return $model;
     }
 
     /** Name of the column used to relate models to tenants. */

src/TenancyServiceProvider.php

@@ -37,7 +37,7 @@ class TenancyServiceProvider extends ServiceProvider
             return $tenancy;
         });
 
-        // Make it possible to inject the current tenant by typehinting the Tenant contract.
+        // 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;
         });

src/helpers.php

@@ -30,7 +30,7 @@ if (! function_exists('tenant')) {
             return app(Tenant::class);
         }
 
-        return optional(app(Tenant::class))->getAttribute($key) ?? null;
+        return app(Tenant::class)?->getAttribute($key);
     }
 }