Improve comments, test reverting on failure during configuration

src/Bootstrappers/LogTenancyBootstrapper.php

@@ -15,18 +15,18 @@ use Stancl\Tenancy\Contracts\TenancyBootstrapper;
 use Stancl\Tenancy\Contracts\Tenant;
 
 /**
- * This bootstrapper makes it possible to configure tenant-specific logging.
+ * Enable tenant-specific logging.
  *
- * By default, all the storage path channels are configured to use tenant
+ * All the storage path channels are configured to use tenant
- * storage directories (see the $storagePathChannels property).
+ * directories by default (see the $storagePathChannels property).
  *
  * For this to work correctly:
  * - this bootstrapper must run *after* FilesystemTenancyBootstrapper,
- * since FilesystemTenancyBootstrapper alters how storage_path() works in the tenant context
+ *   since FilesystemTenancyBootstrapper makes storage_path() return the tenant-specific storage path
  * - storage path suffixing has to be enabled (= config('tenancy.filesystem.suffix_storage_path')
  *   has to be true), since the storage path suffix is what separates logs by tenant
  *
- * The bootstrapper also supports custom channel overrides via the $channelOverrides property (see the property's docblock).
+ * Also supports custom channel overrides (see the $channelOverrides property).
  *
  * @see Stancl\Tenancy\Bootstrappers\FilesystemTenancyBootstrapper
  */
@@ -37,11 +37,13 @@ class LogTenancyBootstrapper implements TenancyBootstrapper
     protected array $configuredChannels = [];
 
     /**
-     * Logging channels that use the storage_path() helper for storing the logs.
+     * Logging channels whose paths use storage_path() by default in the logging config.
-     * Or you can bypass this default behavior by using overrides, since they take
-     * precedence over the default behavior.
      *
-     * All channels included here will be configured to use tenant-specific storage paths.
+     * All channels included here will be configured to use tenant-specific storage paths
+     * generated using storage_path() in the tenant context.
+     *
+     * This is the default behavior. The $channelOverrides property can be used to override
+     * this behavior (the overrides take precedence over $storagePathChannels).
      *
      * Requires FilesystemTenancyBootstrapper to run before this bootstrapper,
      * and storage path suffixing to be enabled.
@@ -54,8 +56,10 @@ class LogTenancyBootstrapper implements TenancyBootstrapper
      * Custom channel configuration overrides.
      *
      * All channels included here will be configured using the provided override.
-     * The overrides take precedence over the default storage path channels
+     * The overrides take precedence over the default ($storagePathChannels) behavior.
-     * behavior.
+     *
+     * You can either map tenant attributes to channel config keys using an array,
+     * or provide a closure that returns the full channel config array.
      *
      * Examples:
      * - Array mapping (the default approach): ['slack' => ['url' => 'webhookUrl']]
@@ -81,9 +85,11 @@ class LogTenancyBootstrapper implements TenancyBootstrapper
             $this->configureChannels($this->configuredChannels, $tenant);
             $this->forgetChannels($this->configuredChannels);
         } catch (\Throwable $exception) {
-            // Revert to default config if anything goes wrong during channel configuration
+            // If an exception is thrown while updating the logging config, the logging config
-            $this->config->set('logging.channels', $this->defaultConfig);
+            // could be left in a corrupt (tenant) state, so we revert to the original config
-            $this->forgetChannels($this->configuredChannels);
+            // to e.g. avoid logging the failure in the tenant log (which would happen if
+            // the channel wasn't resolved before).
+            $this->revert();
 
             throw $exception;
         }
@@ -97,10 +103,11 @@ class LogTenancyBootstrapper implements TenancyBootstrapper
     }
 
     /**
-     * Channels to configure and forget so they can be re-resolved afterwards.
+     * Channels to configure and forget from the log manager so they can be
+     * re-resolved with the new, tenant-specific config on the next use.
      *
      * Includes:
-     * - the default channel
+     * - the default channel (primarily because it can be 'stack')
      * - all channels in the $storagePathChannels array
      * - all channels that have custom overrides in the $channelOverrides property
      */
@@ -110,13 +117,13 @@ class LogTenancyBootstrapper implements TenancyBootstrapper
          * Include the default channel in the list of channels to configure/re-resolve.
          *
          * Including the default channel is harmless (if it's not overridden or not in $storagePathChannels,
-         * it'll just be forgotten and re-resolved on the next use), and for the case where 'stack' is the default,
+         * it'll just be forgotten and re-resolved on the next use with the original config), and for the
-         * this is necessary since the 'stack' channel will be resolved and saved in the log manager,
+         * case where 'stack' is the default, this is necessary since the 'stack' channel will be resolved
-         * and its stale config could accidentally be used instead of the stack member channels.
+         * and saved in the log manager, and its stale config could accidentally be used instead of the stack member channels.
          *
-         * For example, when you use 'stack' with the 'slack' channel and you want to configure the webhook URL,
+         * For example, when you use 'stack' with the 'slack' channel,
-         * both 'stack' and 'slack' must be re-resolved after updating the config for the channels to use the correct webhook URLs.
+         * if only 'slack' is forgotten, 'stack' would still use the stale cached 'slack' driver,
-         * If only one of the mentioned channels would be re-resolved, the other's (stale) webhook URL could be used for logging.
+         * and if only 'stack' is forgotten, the 'slack' channel's config would remain unchanged (central).
          */
         $defaultChannel = $this->config->get('logging.default');
 
@@ -135,7 +142,7 @@ class LogTenancyBootstrapper implements TenancyBootstrapper
      *
      * Only the channels that are in the $storagePathChannels array
      * or have custom overrides in the $channelOverrides property
-     * will be configured.
+     * will be configured (overrides take precedence over storage path channels).
      */
     protected function configureChannels(array $channels, Tenant $tenant): void
     {
@@ -144,8 +151,8 @@ class LogTenancyBootstrapper implements TenancyBootstrapper
                 $this->overrideChannelConfig($channel, static::$channelOverrides[$channel], $tenant);
             } elseif (in_array($channel, static::$storagePathChannels)) {
                 // Set storage path channels to use tenant-specific directory (default behavior).
-                // The tenant log will be located at e.g. "storage/tenant{$tenantKey}/logs/laravel.log"
+                // The tenant log will be located at e.g. "storage/tenant{$tenantKey}/logs/laravel.log",
-                // (assuming FilesystemTenancyBootstrapper is used before this bootstrapper).
+                // assuming FilesystemTenancyBootstrapper is used before this bootstrapper.
                 $originalChannelPath = $this->config->get("logging.channels.{$channel}.path");
                 $centralStoragePath = Str::before(storage_path(), $this->config->get('tenancy.filesystem.suffix_base') . $tenant->getTenantKey());
 
@@ -185,8 +192,9 @@ class LogTenancyBootstrapper implements TenancyBootstrapper
     }
 
     /**
-     * Forget all passed channels so they can be re-resolved
+     * Forget all passed channels from the log manager so that
-     * with updated config on the next logging attempt.
+     * they can be re-resolved with the updated (tenant-specific)
+     * config on the next logging attempt.
      */
     protected function forgetChannels(array $channels): void
     {

tests/Bootstrappers/LogTenancyBootstrapperTest.php

@@ -431,3 +431,32 @@ test('tenant logs inherit the path from the central log path config', function (
         ->toContain($tenant->id)
         ->not()->toContain('central');
 });
+
+test('logging config is reverted to the original state if configuration fails', function() {
+    config([
+        'logging.channels.slack.url' => $originalSlackUrl = 'default-webhook',
+        'logging.channels.single.path' => $originalSinglePath = storage_path('logs/default-single-path.log'),
+    ]);
+
+    $tenant = Tenant::create(['loggingPath' => storage_path('logs/tenant-single-path.log')]);
+
+    // Valid override first, the config will be updated properly,
+    // then an invalid override that will cause the configuration to fail and throw an exception.
+    LogTenancyBootstrapper::$channelOverrides = [
+        'single' => ['path' => 'loggingPath'], // Valid override
+        'slack' => fn () => 'invalid override',
+    ];
+
+    expect(fn() => tenancy()->initialize($tenant))->toThrow(InvalidArgumentException::class);
+
+    // Single channel config reverted to original state after the exception was thrown
+    expect(config('logging.channels.single.path'))->toBe($originalSinglePath);
+
+    // Exception thrown before slack config got changed
+    expect(config('logging.channels.slack.url'))->toBe($originalSlackUrl);
+
+    // The single channel uses the original path for logging
+    Log::channel('single')->info('bootstrap failed');
+    expect(file_exists($originalSinglePath))->toBeTrue();
+    expect(file_get_contents($originalSinglePath))->toContain('bootstrap failed');
+});