-
-
Notifications
You must be signed in to change notification settings - Fork 479
[4.x] Add LogTenancyBootstrapper #1381
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1381 +/- ##
============================================
+ Coverage 85.36% 85.82% +0.45%
- Complexity 1128 1156 +28
============================================
Files 179 181 +2
Lines 3294 3400 +106
============================================
+ Hits 2812 2918 +106
Misses 482 482 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ook used by the slack channel correctly)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a new LogTenancyBootstrapper to provide tenant-specific logging configuration. The bootstrapper automatically configures storage path channels to use tenant-specific directories and supports custom channel overrides for more complex logging scenarios.
Key changes:
- Implements
LogTenancyBootstrapperwith automatic storage path channel configuration and custom override support - Adds comprehensive test coverage for default behavior, custom overrides, and real-world usage scenarios
- Registers the new bootstrapper in the test environment
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/Bootstrappers/LogTenancyBootstrapper.php |
Main implementation of the log tenancy bootstrapper with storage path handling and channel override functionality |
tests/Bootstrappers/LogTenancyBootstrapperTest.php |
Comprehensive test suite covering default behavior, custom overrides, stack channels, and real logging scenarios |
tests/TestCase.php |
Adds import and singleton registration for the new bootstrapper in test environment |
Comments suppressed due to low confidence (1)
tests/Bootstrappers/LogTenancyBootstrapperTest.php:345
- The test relies on catching exceptions to verify webhook URLs, but this approach is fragile and may not work reliably across different environments or Laravel versions. Consider mocking the HTTP client or using a more deterministic testing approach.
try {
| } 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" | ||
| $this->config->set("logging.channels.{$channel}.path", storage_path('logs/laravel.log')); |
Copilot
AI
Jul 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded filename 'laravel.log' should be made configurable or use the original filename from the channel configuration. This prevents customization of log filenames and could overwrite existing configurations.
| $this->config->set("logging.channels.{$channel}.path", storage_path('logs/laravel.log')); | |
| $logFilename = $this->getTenantLogFilename($tenant); | |
| $this->config->set("logging.channels.{$channel}.path", storage_path("logs/{$logFilename}")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, this is fine for the default behavior, it can still be customized. Resolving this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of the box, if no customization is used, $storagePathChannels includes daily which does not use laravel.log names, but day-specific names. Seems like something that should be checked and tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is checked and tested already, I'd just add a short comment with an explanation of how daily works.
daily driver uses RotatingFileHandler that parses the file name. The current code (= storage_path('logs/laravel.log')) corresponds to the daily log channel config. It is correct, so I'd just clarify this since this can indeed be quite confusing
…is set (otherwise, just skip the override and keep the default config value)
| /** | ||
| * Custom channel configuration overrides. | ||
| * | ||
| * Examples: | ||
| * - Array mapping (the default approach): ['slack' => ['url' => 'webhookUrl']] maps $tenant->webhookUrl to slack.url (if $tenant->webhookUrl is not null, otherwise, the override is ignored) | ||
| * - Closure: ['slack' => fn ($config, $tenant) => $config->set('logging.channels.slack.url', $tenant->slackUrl)] | ||
| */ | ||
| public static array $channelOverrides = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the key in this array is e.g. slack, and we can provide either a "partial array override" or a closure that returns the override dynamically based on $tenant, why would the closure approach be:
function (Config\Repository $config, Tenant $tenant): void {
$config->set('something', something based on $tenant);
}As opposed to returning a value that'd directly override the channel:
function (array $channel, Tenant $tenant): array {
return array_merge($channel, [overrides based on $tenant]);
}The current approach would let you do for instance $channelOverrides['foo'] = fn ($config, $tenant) => $config->set('bar', ...) which doesn't make sense. And requiring the user to do $config->set() manually in the first place is unnecessary complexity for the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be resolved now f878aaf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Swapped the parameter order. Example for current usage of overrides:
LogTenancyBootstrapper::$channelOverrides = [
'single' => function (Tenant $tenant, array $channel) {
return array_merge($channel, ['path' => storage_path("logs/override-{$tenant->id}.log")]);
},
];Also updated the comments to clarify the bootstrapper's behavior. So I think this review can be resolved now
…ehavior, improve test by making assertions more specific
…or, keep simpler/less direct assertions in tests that don't require direct assertions, add comments to clarify the behavior
This PR adds the LogTenancyBootstrapper to provide tenant-specific logging configuration. The bootstrapper automatically configures storage path channels to use tenant-specific directories (NOTE: for this to work correctly, the bootstrapper has to run AFTER FilesystemTenancyBootstrapper, otherwise, the logs still won't be separated, unless you use overrides) and supports custom channel overrides for custom logging scenarios (e.g. the slack channel, see the bootstrapper's comments).
Note that the bootstrapper first modifies the channel config, then forgets the channel so that on the next logging attempt, the channel is re-resolved with the modified config. Otherwise, the channel would just use the initial config (in most cases). When using a channel stack, the
stackchannel itself also has to be forgotten, since the LogManager could retain e.g. the originalstackchannel's webhook URL, while the underlyingslackchannel would use the updated one, and while logging, the app would actually use the initial webhook URL instead of the updated one -- encountered this very specific issue while testing).Also, note: adding
'attachment' => 'false'to the slack channel's config makes the slack channel work with Discord webhooks (just a cool thing we figured out whlle testing the bootstrapper).