Fix FilesystemTenancyBootstrapper::scopeCache() discarding configured cache paths - #1473
Fix FilesystemTenancyBootstrapper::scopeCache() discarding configured cache paths#1473lukinovec wants to merge 6 commits into
FilesystemTenancyBootstrapper::scopeCache() discarding configured cache paths#1473Conversation
The tests cover the current (mostly incorrect) scopeCache() behavior (= hardcoding the /framework/cache/data path regardless of what was configured). The 'file cache stores are separated per tenant' is not a regression test -- it covers the default path, which already worked correctly, there were just no tests for it. The rest are regression tests (see the "NOTE ABOUT REGRESSION" comments -- these are temporary, added them just so that it's clear what's currently wrong or broken) that should be fixed by the FS bootstrapper fix in the next commit.
scopeCache() rewrote path and lock_path for every file-driver store to a hardcoded '<storage>/framework/cache/data' path, completely ignoring the store's config. Now, scopeCache() remembers each store's original path and lock_path, scopes these paths for the tenant, and restores them to the stored originals on revert. The store's lock_path was always overwritten by the same hardcoded path. But lock_path is configurable too, AND it's actually optional (unlike path). If it's not configured at all (= it's null or just unset), Laravel automatically falls back to the store's path. So in that case, leave lock_path null instead of assigning the path to it. This is not a *huge* change, assigning path to lock_path would essentially achieve the same thing, BUT if someone explicitly sets lock_path to null in the config, we should just respect that and let Laravel fall back to the path instead of setting the lock_path ourselves. Also, on revert(), the same hardcoded path was used in scopeCache(). So if someone used a custom file driver-based store, cached something in central context, initialized and ended tenancy, the central cache got corrupt (see the 'central cache is not lost when tenancy ends' test).
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughFile cache stores now retain their original paths, apply tenant-specific ChangesFilesystem cache scoping
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1473 +/- ##
============================================
+ Coverage 86.63% 86.65% +0.02%
- Complexity 1219 1227 +8
============================================
Files 186 186
Lines 3583 3605 +22
============================================
+ Hits 3104 3124 +20
- Misses 479 481 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Bootstrappers/FilesystemTenancyBootstrapper.php`:
- Around line 220-244: Validate the original cache path in the cache-scoping
flow before passing it to scopeCachePath(); when a file-driver store omits path,
fail with a clear configuration error or skip the store consistently during
bootstrap and revert. Preserve the existing optional lock_path handling and
ensure scopeCachePath() is never called with null.
🪄 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 Plus
Run ID: 31b4c918-c01e-4d38-bdde-b69db2e32054
📒 Files selected for processing (2)
src/Bootstrappers/FilesystemTenancyBootstrapper.phptests/Bootstrappers/FilesystemTenancyBootstrapperTest.php
scopeCache() didn't feel right since it 1) stored thee original paths, 2) actually scoped things. Separate the concerns so that scopeCache() just does that -- scopes cache.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Making it protected could be a minor bc, and it'd be inconsistent with scopeSessions (which is public).
Using a different directory for the
filecache store by settingcache.stores.file.path(either usingconfig([...]), or directly inconfig/cache.php-- doesn't matter) has no effect --FilesystemTenancyBootstrapper::scopeCache()ignorespath/lock_pathentirely and rewrites both to a hardcoded<storage>/framework/cache/datapath on everytenancy()->initialize()/tenancy()->end():Specific issues with hardcoding the path like this:
lock_pathis always overwritten withpath, so a store with a separate lock directory loses that separationrevert()runs the same code, so it doesn't restore what the store was configured with before tenancy initialized -- it just re-applies the same hardcoded default. Central cache ends up using the wrong path after ending tenancy.The fix
scopeCache()now captures each store's originalpath/lock_pathonbootstrap()and scopes those instead of a hardcoded default.scopeCachePath()handles the actual scoping:storage/framework/cache/databecomesstorage/tenant1/framework/cache/data).lock_pathstaysnullwhen a store doesn't configure it, rather than us making it default to the scoped path --FileStorealready falls back topathfor locks in that case, so no need for us to do that, we can just respect the store's original config.Possible further improvement
There's another thing we could explore -- rather than
scopeCachePath()hardcoding how a store's path gets scoped, that could go through config instead, similar to howdiskRoot()resolvesroot_overridetemplates with the%storage_path%/%tenant%placeholders. Something like atenancy.cache.path_override.{$name}key with a%configured_path%/%suffix%-style template.That would make cache path scoping configurable instead of hardcoded, and it'd let people do things
this fix doesn't support -- e.g. keep the cache directory central, with tenant subdirectories under it, instead of moving the whole store under the tenant's storage path. I think that'd help specifically when people set
suffix_storage_path => false, since with that setting,storage_path()never gets a tenant prefix anywhere, butscopeCache()doesn't check it, so it still creates astorage/tenant1/framework/cache/datadirectory (the only tenant-prefixed thing anywhere understorage/for those users).Looked into that for a bit, but not sure if that's worth pursuing right now. This fix already swaps the central path prefix for the tenant's when a store's path is under
storage_path(), and appends the suffix otherwise, without needing any config. Apath_overridekey would only be useful for less cases like the "central with subdirectory" example mentioned above -- stores that don't set one would just keep using this fix's existing behavior directly (no override or template involved).Leaving this as a draft for now, until this alternative is explored and discussed thoroughly.
More issues found by looking into the possible improvements
'suffix_storage_path' => falseis not respected at all by scopeCache and scopeSessions.(will edit this, for now, see https://3.basecamp.com/5170965/buckets/23651018/todos/10144092860#__recording_10154592928)
path_overridethingSummary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Tests