Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1381 +/- ##
============================================
+ Coverage 86.03% 86.23% +0.19%
- Complexity 1156 1173 +17
============================================
Files 184 185 +1
Lines 3381 3428 +47
============================================
+ Hits 2909 2956 +47
Misses 472 472 ☔ 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.
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')); |
There was a problem hiding this comment.
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.
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.
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.
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
There was a problem hiding this comment.
To clarify further, 'logging.channels.daily.path' => storage_path('logs/laravel.log') is correct, consistent with Laravel's default daily channel path. When using a config like this, the log will be created e.g. at storage/tenantfoo/logs/laravel-2026-04-14.log (this is tested in stack logs are written to all configured channels with tenant-specific paths)
…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.
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.
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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Bootstrappers/LogTenancyBootstrapperTest.php`:
- Around line 162-173: The test initializes tenancy via tenancy()->initialize()
but never calls tenancy()->end(), causing state to leak; update the test in
LogTenancyBootstrapperTest (the case that uses
LogTenancyBootstrapper::$channelOverrides and
tenancy()->initialize(Tenant::create())) to call tenancy()->end() after the
assertions (or add a finally/teardown call) so tenancy is explicitly ended;
ensure the other affected test(s) that initialize tenancy (the block around the
other 175–188 test) are similarly updated to call tenancy()->end() to keep tests
self-contained.
- Around line 23-27: The cleanup currently removes default logs via the
unlink/glob block in LogTenancyBootstrapperTest but misses override-generated
files; update the cleanup blocks (the unlink(storage_path('logs/laravel.log')) /
unlink(storage_path('logs/laravel-' . now()->format('Y-m-d') . '.log')) /
foreach (glob(storage_path('tenant*/logs/*.log')) ...) code in
LogTenancyBootstrapperTest to also remove any override logs by unlinking
storage_path('logs/override-*.log') and
storage_path('logs/custom-override-tenant.log') (or using
glob(storage_path('logs/override-*.log')) and
glob(storage_path('logs/custom-override-tenant.log')) with foreach to unlink
each), and apply the same additions to the second cleanup block later in the
file so stale override files can't mask failing assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2ac72f9d-1fe2-4b9b-89b3-f1ac73f6cb8a
📒 Files selected for processing (1)
tests/Bootstrappers/LogTenancyBootstrapperTest.php
| test('channel config keys remain unchanged if the specified tenant override attribute is null', function() { | ||
| config(['logging.channels.slack.username' => 'Default username']); | ||
|
|
||
| LogTenancyBootstrapper::$channelOverrides = [ | ||
| 'slack' => ['username' => 'nonExistentAttribute'], // $tenant->nonExistentAttribute | ||
| ]; | ||
|
|
||
| tenancy()->initialize(Tenant::create()); | ||
|
|
||
| // The username should remain unchanged since the tenant attribute is null | ||
| expect(config('logging.channels.slack.username'))->toBe('Default username'); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Explicitly end tenancy in tests that call tenancy()->initialize().
Line 169 and Line 184 initialize tenancy without a matching tenancy()->end(). Adding explicit teardown keeps tests self-contained and prevents latent state bleed.
Suggested fix
test('channel config keys remain unchanged if the specified tenant override attribute is null', function() {
@@
tenancy()->initialize(Tenant::create());
@@
expect(config('logging.channels.slack.username'))->toBe('Default username');
+ tenancy()->end();
});
@@
test('channel overrides take precedence over the default storage path channel updating logic', function () {
@@
tenancy()->initialize($tenant);
@@
expect(config('logging.channels.single.path'))->toEndWith('storage/logs/override-tenant1.log');
+ tenancy()->end();
});Also applies to: 175-188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Bootstrappers/LogTenancyBootstrapperTest.php` around lines 162 - 173,
The test initializes tenancy via tenancy()->initialize() but never calls
tenancy()->end(), causing state to leak; update the test in
LogTenancyBootstrapperTest (the case that uses
LogTenancyBootstrapper::$channelOverrides and
tenancy()->initialize(Tenant::create())) to call tenancy()->end() after the
assertions (or add a finally/teardown call) so tenancy is explicitly ended;
ensure the other affected test(s) that initialize tenancy (the block around the
other 175–188 test) are similarly updated to call tenancy()->end() to keep tests
self-contained.
✅ Confirmed as addressed by @lukinovec
There was a problem hiding this comment.
@lukinovec, thanks for the update! Confirmed — the commit adds the missing tenancy()->end() calls to keep those tests self-contained. ✅
There was a problem hiding this comment.
@coderabbitai Ignore that, was meant for another review thread.
FYI - tenancy staying initialized in tests is not an issue. Tenancy context is container state and container state is reset between tests, unlike static properties which we do take care to reset in between tests (beforeEach + afterEach).
There was a problem hiding this comment.
@stancl, understood and thanks for the clarification! I'll keep this in mind going forward — tenancy context is container state and gets reset automatically between tests, so explicit tenancy()->end() calls aren't necessary for test isolation. Only static properties (like LogTenancyBootstrapper::$channelOverrides and $storagePathChannels) need manual reset, which is already handled in beforeEach/afterEach.
✏️ Learnings added
Learnt from: stancl
Repo: archtechx/tenancy PR: 1381
File: tests/Bootstrappers/LogTenancyBootstrapperTest.php:174-185
Timestamp: 2026-04-13T15:47:08.682Z
Learning: In the archtechx/tenancy PHP test suite, tenancy context is container state and is automatically reset between tests by the test framework. Therefore, calling `tenancy()->end()` at the end of a test is NOT required for test isolation — it is not an issue if tenancy stays initialized after a test. Only static properties (e.g., `LogTenancyBootstrapper::$channelOverrides`, `LogTenancyBootstrapper::$storagePathChannels`) need explicit reset between tests, which is handled via `beforeEach`/`afterEach`. Do not flag missing `tenancy()->end()` calls as a test isolation issue.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: lukinovec
Repo: archtechx/tenancy PR: 1448
File: tests/Bootstrappers/BroadcastingConfigBootstrapperTest.php:21-26
Timestamp: 2026-04-13T13:52:54.827Z
Learning: In the archtechx/tenancy PHP test suite, when using `beforeEach`/`afterEach` to mutate/reset static properties (e.g., `TenancyBroadcastManager::$tenantBroadcasters`), avoid hard-coding the expected values solely for reset. Instead, capture the original static values at the start of `beforeEach` (e.g., via `$original*` variables) and restore those exact originals in `afterEach` so tests are isolated and robust against changes in default/static state. Only use `$original*` scoped to individual closures when you truly need mid-test assertions.
Learnt from: lukinovec
Repo: archtechx/tenancy PR: 1446
File: src/Jobs/DeleteTenantStorage.php:43-44
Timestamp: 2026-04-13T15:14:03.457Z
Learning: When using `File::deleteDirectory($path)` from `archtechx/tenancy`, note its return behavior: it only returns `false` when `$path` is *not* a directory. If the call site is already guarded with `is_dir($path)` (or otherwise guarantees `$path` is a directory), then checking the boolean return value of `File::deleteDirectory($path)` for error handling is unnecessary because, after the directory-existence check passes, the method suppresses internal `rmdir` errors and unconditionally returns `true`. Prefer handling issues via upstream validation/guards (e.g., ensure the path is a directory) rather than relying on the return value in that code path.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests/Bootstrappers/LogTenancyBootstrapperTest.php (3)
175-188:⚠️ Potential issue | 🟡 MinorMissing
tenancy()->end()call at end of test.Same issue as the previous test - tenancy is initialized but not explicitly ended.
🧹 Proposed fix
// Should use override, not the default storage path updating behavior expect(config('logging.channels.single.path'))->toEndWith('storage/logs/override-tenant1.log'); + + tenancy()->end(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Bootstrappers/LogTenancyBootstrapperTest.php` around lines 175 - 188, The test "channel overrides take precedence over the default storage path channel updating logic" in LogTenancyBootstrapperTest initializes tenancy via tenancy()->initialize($tenant) but never calls tenancy()->end(), which can leak state into other tests; update this test to call tenancy()->end() at the end (after the expectation) so tenancy is properly cleaned up, referencing the tenancy()->initialize(...) call and adding a corresponding tenancy()->end() teardown step.
162-173:⚠️ Potential issue | 🟡 MinorMissing
tenancy()->end()call at end of test.This test initializes tenancy but doesn't explicitly end it, which can cause state to leak to subsequent tests. The
afterEachhook resets static properties but doesn't calltenancy()->end().🧹 Proposed fix
tenancy()->initialize(Tenant::create()); // The username should remain unchanged since the tenant attribute is null expect(config('logging.channels.slack.username'))->toBe('Default username'); + + tenancy()->end(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Bootstrappers/LogTenancyBootstrapperTest.php` around lines 162 - 173, The test "channel config keys remain unchanged if the specified tenant override attribute is null" initializes tenancy via tenancy()->initialize(Tenant::create()) but does not call tenancy()->end(), risking leaked state; update the test to call tenancy()->end() at the end (after the expect assertion) so the tenancy lifecycle is properly terminated, keeping the use of LogTenancyBootstrapper::$channelOverrides and the same assertion intact.
289-295:⚠️ Potential issue | 🟡 MinorMissing
tenancy()->end()call at end of test.The test initializes tenancy on line 290 but doesn't end it before the test completes. Additionally, the sequential
tenancy()->initialize()calls on lines 264 and 271 may not properly clean up between tenant contexts if the framework doesn't automatically end prior tenancy.🧹 Proposed fix
Log::channel('single')->info('tenant-override'); expect(file_get_contents(storage_path('logs/custom-override-tenant.log')))->toContain('tenant-override'); + + tenancy()->end(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Bootstrappers/LogTenancyBootstrapperTest.php` around lines 289 - 295, The test starts tenancy contexts via tenancy()->initialize() but never calls tenancy()->end(), which can leak tenant state between tests; update the test (LogTenancyBootstrapperTest) to call tenancy()->end() after each tenancy()->initialize() usage (including after the final tenant-override block) to ensure cleanup, and if there are multiple sequential initialize() calls ensure you explicitly end the prior tenancy before initializing a new one so each tenant context is properly terminated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Bootstrappers/LogTenancyBootstrapper.php`:
- Around line 138-142: The closure override block must validate the return value
from invoking $override; call $override($tenant,
$this->config->get($channelConfigKey)) into a variable (e.g., $newConfig),
verify is_array($newConfig) before calling $this->config->set($channelConfigKey,
$newConfig), and if the check fails throw an InvalidArgumentException (or
otherwise abort with a clear error) so invalid/null return values from the
Closure cannot corrupt logging channel config; update the block around
$override, $channelConfigKey and $this->config->set accordingly.
---
Duplicate comments:
In `@tests/Bootstrappers/LogTenancyBootstrapperTest.php`:
- Around line 175-188: The test "channel overrides take precedence over the
default storage path channel updating logic" in LogTenancyBootstrapperTest
initializes tenancy via tenancy()->initialize($tenant) but never calls
tenancy()->end(), which can leak state into other tests; update this test to
call tenancy()->end() at the end (after the expectation) so tenancy is properly
cleaned up, referencing the tenancy()->initialize(...) call and adding a
corresponding tenancy()->end() teardown step.
- Around line 162-173: The test "channel config keys remain unchanged if the
specified tenant override attribute is null" initializes tenancy via
tenancy()->initialize(Tenant::create()) but does not call tenancy()->end(),
risking leaked state; update the test to call tenancy()->end() at the end (after
the expect assertion) so the tenancy lifecycle is properly terminated, keeping
the use of LogTenancyBootstrapper::$channelOverrides and the same assertion
intact.
- Around line 289-295: The test starts tenancy contexts via
tenancy()->initialize() but never calls tenancy()->end(), which can leak tenant
state between tests; update the test (LogTenancyBootstrapperTest) to call
tenancy()->end() after each tenancy()->initialize() usage (including after the
final tenant-override block) to ensure cleanup, and if there are multiple
sequential initialize() calls ensure you explicitly end the prior tenancy before
initializing a new one so each tenant context is properly terminated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c4324a20-f406-4af1-940e-c51805cbddd6
📒 Files selected for processing (2)
src/Bootstrappers/LogTenancyBootstrapper.phptests/Bootstrappers/LogTenancyBootstrapperTest.php
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/Bootstrappers/LogTenancyBootstrapperTest.php (1)
174-185:⚠️ Potential issue | 🟡 MinorExplicitly end tenancy in these tests.
Both tests initialize tenancy without a matching teardown. That makes them rely on external test isolation and leaks tenant state if an assertion fails before cleanup.
Suggested fix
test('channel config keys remain unchanged if the specified tenant override attribute is null', function() { config(['logging.channels.slack.username' => 'Default username']); @@ - tenancy()->initialize(Tenant::create()); - - // The username should remain unchanged since the tenant attribute is null - expect(config('logging.channels.slack.username'))->toBe('Default username'); + tenancy()->initialize(Tenant::create()); + try { + // The username should remain unchanged since the tenant attribute is null + expect(config('logging.channels.slack.username'))->toBe('Default username'); + } finally { + tenancy()->end(); + } }); @@ - tenancy()->initialize($tenant); - - // Should use override, not the default storage path updating behavior - expect(config('logging.channels.single.path'))->toEndWith('storage/logs/override-tenant1.log'); + tenancy()->initialize($tenant); + try { + // Should use override, not the default storage path updating behavior + expect(config('logging.channels.single.path'))->toEndWith('storage/logs/override-tenant1.log'); + } finally { + tenancy()->end(); + } });Also applies to: 187-200
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Bootstrappers/LogTenancyBootstrapperTest.php` around lines 174 - 185, The tests call tenancy()->initialize(Tenant::create()) (in tests referencing LogTenancyBootstrapper::$channelOverrides) but never explicitly end tenancy, risking leaked tenant state on failures; update each affected test (the one shown and the one at 187-200) to explicitly call tenancy()->end() after the assertions or wrap initialization/assertions in a try/finally and call tenancy()->end() in the finally block so tenancy is always torn down even if the expectation fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Bootstrappers/LogTenancyBootstrapper.php`:
- Around line 58-64: The bootstrap method mutates logging.channels incrementally
and doesn't roll back on failure; wrap the calls that modify config (calls to
getChannels(), configureChannels($channels, $tenant), forgetChannels($channels))
in a try/catch so that on any exception you restore the original channels from
$this->defaultConfig via $this->config->set('logging.channels',
$this->defaultConfig) and rethrow the exception; also only mark the bootstrapper
as initialized (so revert() will run) after the configuration has been
successfully applied (i.e., move any "initialized" flag assignment to after the
try block).
---
Duplicate comments:
In `@tests/Bootstrappers/LogTenancyBootstrapperTest.php`:
- Around line 174-185: The tests call tenancy()->initialize(Tenant::create())
(in tests referencing LogTenancyBootstrapper::$channelOverrides) but never
explicitly end tenancy, risking leaked tenant state on failures; update each
affected test (the one shown and the one at 187-200) to explicitly call
tenancy()->end() after the assertions or wrap initialization/assertions in a
try/finally and call tenancy()->end() in the finally block so tenancy is always
torn down even if the expectation fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e73187a2-4af1-49b5-8c03-a51a6234fe1c
📒 Files selected for processing (2)
src/Bootstrappers/LogTenancyBootstrapper.phptests/Bootstrappers/LogTenancyBootstrapperTest.php
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/Bootstrappers/LogTenancyBootstrapperTest.php (1)
174-185:⚠️ Potential issue | 🟠 MajorExplicitly end tenancy in tests that call
tenancy()->initialize().These cases still exit with tenancy active. That keeps the bootstrapper state/live tenant context around for later assertions and can make the suite order-dependent if the application instance is reused. Add
tenancy()->end()in afinallyblock or at the end of each test.Suggested fix
test('channel config keys remain unchanged if the specified tenant override attribute is null', function() { @@ tenancy()->initialize(Tenant::create()); // The username should remain unchanged since the tenant attribute is null expect(config('logging.channels.slack.username'))->toBe('Default username'); + tenancy()->end(); }); @@ test('channel overrides take precedence over the default storage path channel updating logic', function () { @@ tenancy()->initialize($tenant); // Should use override, not the default storage path updating behavior expect(config('logging.channels.single.path'))->toEndWith('storage/logs/override-tenant1.log'); + tenancy()->end(); });Also applies to: 187-200, 276-307
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Bootstrappers/LogTenancyBootstrapperTest.php` around lines 174 - 185, The test calls tenancy()->initialize(Tenant::create()) but never ends tenancy, leaving the bootstrapper/tenant context active for later tests; update this test (and the other affected tests that call tenancy()->initialize()) to ensure tenancy()->end() is always called—either wrap the assertions in a try/finally where tenancy()->end() is called in finally, or call tenancy()->end() at the end of the test so LogTenancyBootstrapper::$channelOverrides and the live tenant context are cleaned up before subsequent assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/Bootstrappers/LogTenancyBootstrapperTest.php`:
- Around line 174-185: The test calls tenancy()->initialize(Tenant::create())
but never ends tenancy, leaving the bootstrapper/tenant context active for later
tests; update this test (and the other affected tests that call
tenancy()->initialize()) to ensure tenancy()->end() is always called—either wrap
the assertions in a try/finally where tenancy()->end() is called in finally, or
call tenancy()->end() at the end of the test so
LogTenancyBootstrapper::$channelOverrides and the live tenant context are
cleaned up before subsequent assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5eb2d039-3573-44ee-a85c-3e1387c7726d
📒 Files selected for processing (2)
src/Bootstrappers/LogTenancyBootstrapper.phptests/Bootstrappers/LogTenancyBootstrapperTest.php
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Bootstrappers/LogTenancyBootstrapper.php`:
- Around line 60-61: The bootstrap() method computes $channels via getChannels()
but doesn't persist that exact set; change LogTenancyBootstrapper to store the
computed channels (e.g. a new private property $bootstrappedChannels) when
bootstrap() runs and then have revert() use $this->bootstrappedChannels (with a
safe null/empty check fallback) instead of recomputing getChannels() or reading
logging.default/storagePathChannels/channelOverrides again; update any code
paths that reference the prior temporary $channels (including lines around
74-78) to read from $this->bootstrappedChannels so tenant-specific logger
bindings are reliably forgotten on revert.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 17d62ba3-c4db-44b3-876b-2777b37122c3
📒 Files selected for processing (2)
src/Bootstrappers/LogTenancyBootstrapper.phptests/Bootstrappers/LogTenancyBootstrapperTest.php
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Bootstrappers/LogTenancyBootstrapper.php`:
- Around line 138-163: The throw in overrideChannelConfig uses a fully-qualified
\InvalidArgumentException; to match file import style, add an import for
InvalidArgumentException at the top of the file and update the throw expression
in overrideChannelConfig to throw new InvalidArgumentException("Channel override
closure for '{$channel}' must return an array."); so the class is referenced by
its short name instead of the prefixed FQN.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3262507a-2e82-4c22-b68d-0cbbca21aa33
📒 Files selected for processing (1)
src/Bootstrappers/LogTenancyBootstrapper.php
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Bootstrappers/LogTenancyBootstrapper.php`:
- Around line 43-53: The docblock for $channelOverrides is misleading: clarify
that overrides may be either an array mapping or a Closure, and that only the
Closure's return value must be an array (not the Closure itself). Update the
comment near $channelOverrides in LogTenancyBootstrapper.php to state: supported
override types are an array mapping (e.g. ['slack' => ['url' => 'webhookUrl']])
or a Closure (e.g. fn(Tenant $tenant, array $channel): array =>
array_merge($channel, ['url' => $tenant->slackUrl'])), and in the Closure case
the Closure must return the channel configuration as an array. Ensure the
wording removes the contradiction that "the override should be an array" and
instead specifies the Closure’s return value requirement.
- Around line 131-135: The code in LogTenancyBootstrapper that handles channels
listed in static::$storagePathChannels currently overwrites any existing
logging.channels.{channel}.path with storage_path('logs/laravel.log'), which
destroys custom filenames and causes different tenant channels to collide;
instead read the existing configured path (e.g.
$this->config->get("logging.channels.{$channel}.path")), compute its relative
subpath (preserving directories and basename) and then set
logging.channels.{channel}.path to the tenant-scoped storage path by joining the
tenant directory (tenant key) with that relative subpath (use storage_path for
absolute base), ensuring you preserve the original subpath if it’s already
outside storage/logs by rebasing rather than replacing in the $this->config->set
call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9658fcee-45fd-4728-86ed-1158a5a4f0b4
📒 Files selected for processing (1)
src/Bootstrappers/LogTenancyBootstrapper.php
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Bootstrappers/LogTenancyBootstrapper.php (1)
136-138:⚠️ Potential issue | 🟠 MajorPreserve the relative log subpath, not just the filename.
basename($path)keeps only the last segment. If two tenantized channels are configured asstorage/logs/api/app.logandstorage/logs/web/app.log, they both collapse tostorage/.../logs/app.logand their output mixes. Rebase the path relative tostorage_path()when the original path already lives under storage, and only fall back tobasename()for non-storage paths.💡 Suggested fix
- $path = $this->config->get("logging.channels.{$channel}.path"); - - $this->config->set("logging.channels.{$channel}.path", storage_path('logs/' . ($path ? basename($path) : 'laravel.log'))); + $path = (string) $this->config->get( + "logging.channels.{$channel}.path", + storage_path('logs/laravel.log') + ); + $storageRoot = rtrim(storage_path(), DIRECTORY_SEPARATOR) . DIRECTORY_SEPARATOR; + + $relativePath = str_starts_with($path, $storageRoot) + ? ltrim(substr($path, strlen($storageRoot)), DIRECTORY_SEPARATOR) + : 'logs/' . basename($path); + + $this->config->set("logging.channels.{$channel}.path", storage_path($relativePath));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Bootstrappers/LogTenancyBootstrapper.php` around lines 136 - 138, The current logic uses basename($path) which loses intermediate directories and causes different tenant channels (e.g., storage/logs/api/app.log vs storage/logs/web/app.log) to collide; update the code in LogTenancyBootstrapper where you fetch and set "logging.channels.{$channel}.path" to preserve a relative subpath when the original path resides under storage_path(): if $path starts with storage_path() compute the relative path by stripping storage_path() and trimming leading slashes and join that under storage_path('logs/' . $relative), otherwise fall back to using basename($path) (and default to 'laravel.log' when $path is empty); use existing symbols $this->config->get, $this->config->set, storage_path() and basename() to locate and change the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Bootstrappers/LogTenancyBootstrapperTest.php`:
- Around line 32-34: In beforeEach capture the current static values of
LogTenancyBootstrapper::$channelOverrides and
LogTenancyBootstrapper::$storagePathChannels into local variables (e.g.
$originalChannelOverrides, $originalStoragePathChannels) and then in afterEach
restore those exact captured values back to
LogTenancyBootstrapper::$channelOverrides and
LogTenancyBootstrapper::$storagePathChannels instead of hard-coding arrays so
the tests revert to the true originals; apply the same pattern for the other
reset points that currently hard-code values.
---
Duplicate comments:
In `@src/Bootstrappers/LogTenancyBootstrapper.php`:
- Around line 136-138: The current logic uses basename($path) which loses
intermediate directories and causes different tenant channels (e.g.,
storage/logs/api/app.log vs storage/logs/web/app.log) to collide; update the
code in LogTenancyBootstrapper where you fetch and set
"logging.channels.{$channel}.path" to preserve a relative subpath when the
original path resides under storage_path(): if $path starts with storage_path()
compute the relative path by stripping storage_path() and trimming leading
slashes and join that under storage_path('logs/' . $relative), otherwise fall
back to using basename($path) (and default to 'laravel.log' when $path is
empty); use existing symbols $this->config->get, $this->config->set,
storage_path() and basename() to locate and change the logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8006ceab-0634-4428-a3b7-6ac00a203f75
📒 Files selected for processing (2)
src/Bootstrappers/LogTenancyBootstrapper.phptests/Bootstrappers/LogTenancyBootstrapperTest.php
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Bootstrappers/LogTenancyBootstrapper.php`:
- Around line 133-141: The code currently resolves and bakes an absolute central
storage path into tenant channels; add a runtime guard in LogTenancyBootstrapper
(the branch handling static::$storagePathChannels where you call
$this->config->get("logging.channels.{$channel}.path") and set
storage_path(...)) that detects if the existing channel path is already an
absolute/central storage path (e.g., starts with storage_path() or is an
absolute filesystem path) and immediately throw a clear RuntimeException
instructing the user to enable/position FilesystemTenancyBootstrapper earlier in
the bootstrappers list; do this check before calling storage_path(...) and avoid
mutating the config if the guard triggers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6db5d9fe-f993-4381-b44e-a9709a300325
📒 Files selected for processing (1)
src/Bootstrappers/LogTenancyBootstrapper.php
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Bootstrappers/LogTenancyBootstrapper.php (1)
145-155:⚠️ Potential issue | 🟠 MajorSkip storage-path rewrites when storage suffixing is disabled.
This branch still rewrites
logging.channels.{channel}.patheven whentenancy.filesystem.suffix_storage_pathisfalse. In that modestorage_path()stays central, so these channels keep writing to shared storage instead of being tenant-isolated. Please gate the default path-rewrite logic on that flag and silently skip it when suffixing is off.💡 Suggested fix
foreach ($channels as $channel) { if (isset(static::$channelOverrides[$channel])) { $this->overrideChannelConfig($channel, static::$channelOverrides[$channel], $tenant); - } elseif (in_array($channel, static::$storagePathChannels)) { + } elseif (in_array($channel, static::$storagePathChannels, true)) { + if ($this->config->get('tenancy.filesystem.suffix_storage_path') === false) { + continue; + } + // 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" // (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());Based on learnings, in
archtechx/tenancyLogTenancyBootstrapper, the$storagePathChannelslogic depends on bothFilesystemTenancyBootstrapperrunning first andtenancy.filesystem.suffix_storage_pathbeingtrue; whensuffix_storage_pathisfalse, storage-path channels should be silently skipped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Bootstrappers/LogTenancyBootstrapper.php` around lines 145 - 155, The branch in LogTenancyBootstrapper that rewrites logging.channels.{channel}.path for entries in $storagePathChannels must be skipped when tenancy.filesystem.suffix_storage_path is false; update the conditional around the in_array($channel, static::$storagePathChannels) block to also check config('tenancy.filesystem.suffix_storage_path') (or $this->config->get(...)) and return/continue silently when suffixing is disabled so no storage_path() rewrite occurs and channels remain on central storage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Bootstrappers/LogTenancyBootstrapperTest.php`:
- Around line 23-26: The cleanup only globs one-level *.log files into $logFiles
so nested files like storage/logs/single/*.log, storage/logs/daily/*.log and
tenantized storage/tenant*/logs/*/*.log are missed; update the globbing used
where $logFiles is constructed (and the similar block at the other occurrence)
to include nested directories (e.g. add patterns for logs/*/*.log and
tenant*/logs/*/*.log or switch to a recursive iterator/RegexIterator using
storage_path()) so all nested .log files are collected and removed during
teardown.
---
Duplicate comments:
In `@src/Bootstrappers/LogTenancyBootstrapper.php`:
- Around line 145-155: The branch in LogTenancyBootstrapper that rewrites
logging.channels.{channel}.path for entries in $storagePathChannels must be
skipped when tenancy.filesystem.suffix_storage_path is false; update the
conditional around the in_array($channel, static::$storagePathChannels) block to
also check config('tenancy.filesystem.suffix_storage_path') (or
$this->config->get(...)) and return/continue silently when suffixing is disabled
so no storage_path() rewrite occurs and channels remain on central storage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fd32f838-0fe9-4557-bf08-fe6b4594b276
📒 Files selected for processing (2)
src/Bootstrappers/LogTenancyBootstrapper.phptests/Bootstrappers/LogTenancyBootstrapperTest.php
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
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).
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, 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).Summary by CodeRabbit
New Features
Tests
Chores