Disable AutoDataSource caching in debug mode #1424
Conversation
WalkthroughA protected helper method Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
modules/cms/classes/AutoDatasource.php (1)
127-138: Consider adding a type-hint and minor simplification tofetchPathCacheTo align with the rest of the class and make intent clearer, you could:
- Type-hint the parameter as the interface this method actually expects (it calls
getAvailablePaths()andgetPathsCacheKey()), e.g.:protected function fetchPathCache(DatasourceInterface $datasource): array
- Optionally simplify the body to avoid the temporary variable:
protected function fetchPathCache(DatasourceInterface $datasource): array { if (Config::get('app.debug', false)) { return $datasource->getAvailablePaths(); } return Cache::rememberForever($datasource->getPathsCacheKey(), function () use ($datasource) { return $datasource->getAvailablePaths(); }); }This keeps behaviour identical while tightening up the contract and readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/cms/classes/AutoDatasource.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/cms/classes/AutoDatasource.php (1)
modules/system/helpers/Cache.php (1)
Cache(8-84)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: windows-latest / PHP 8.1
- GitHub Check: windows-latest / PHP 8.4
- GitHub Check: ubuntu-latest / PHP 8.4
- GitHub Check: windows-latest / PHP 8.2
- GitHub Check: windows-latest / PHP 8.3
- GitHub Check: ubuntu-latest / PHP 8.3
- GitHub Check: ubuntu-latest / PHP 8.1
- GitHub Check: ubuntu-latest / PHP 8.2
- GitHub Check: windows-latest / JavaScript
🔇 Additional comments (2)
modules/cms/classes/AutoDatasource.php (2)
13-13: Debug-mode cache bypass and helper refactor look correctUsing
Config::get('app.debug', false)insidefetchPathCache()and routing all call sites (appendDatasource,prependDatasource,populateCache) through that helper cleanly disables the persistent cache in debug while keeping the existing in‑memory$this->pathCachebehaviour intact. This matches the stated goal and should preserve existing semantics whenapp.debugisfalse.I’d just recommend exercising both
app.debug = trueandfalsepaths in tests (or manual checks) to confirm that:
- Path resolution behaves the same as before in non‑debug.
- Changes to underlying datasources are immediately visible in debug without needing manual cache clears.
Also applies to: 77-81, 86-90, 105-125
77-81: <function_calls>
Search for actual usage of prependDatasource and appendDatasource in the entire codebase
echo "=== prependDatasource usage ==="
grep -r "prependDatasource" --include=".php" .
echo ""
echo "=== appendDatasource usage ==="
grep -r "appendDatasource" --include=".php" .
Now let me create a comprehensive test to verify the actual behavior:
<function_calls>
Let's understand the structure better by examining what array_prepend does
and trace through a concrete scenario
python3 << 'PYTHON_EOF'
Simulate the scenario described in the review
Initial state after populateCache with 2 datasources
pathCache_initial = [
{'path1': True, 'path2': True}, # index 0
{'path3': True} # index 1
]
datasources_initial = {
'db': 'DbDatasource',
'fs': 'FileDatasource'
}print("INITIAL STATE (after populateCache)")
print(f"pathCache keys: {list(pathCache_initial.keys() if isinstance(pathCache_initial, dict) else range(len(pathCache_initial)))}")
print(f"datasources keys: {list(datasources_initial.keys())}")
print()Simulate appendDatasource('newfs', datasource)
PHP: $this->datasources[$key] = $datasource;
PHP: $this->pathCache[] = $this->fetchPathCache($datasource);
pathCache_after_append = [
{'path1': True, 'path2': True}, # index 0
{'path3': True}, # index 1
{'path4': True} # index 2
]
datasources_after_append = {
'db': 'DbDatasource',
'fs': 'FileDatasource',
'newfs': 'NewFileDatasource'
}print("AFTER appendDatasource('newfs', ds)")
print(f"pathCache indices: {list(range(len(pathCache_after_append)))}")
print(f"datasources keys: {list(datasources_after_append.keys())}")
print(f"array_keys(datasources): {list(datasources_after_append.keys())}")
print()Problem in getDatasourceForPath at line 282:
$datasourceIndex = array_keys($this->datasources)[$datasourceIndex];
If we found path at numeric index 2, we do:
array_keys(datasources)[2] = 'newfs' ✓ Works!
print("getDatasourceForPath scenario:")
print(f"Found path at numeric index: 2")
print(f"array_keys(datasources)[2] = '{list(datasources_after_append.keys())[2]}' ✓")
print()Now simulate prependDatasource using array_prepend
PHP: $this->datasources = array_prepend($this->datasources, $datasource, $key);
PHP: $this->pathCache = array_prepend($this->pathCache, $this->fetchPathCache($datasource), $key);
This should put string key at the beginning
pathCache_after_prepend = {}
pathCache_after_prepend['prependfs'] = {'path0': True} # string key from prepend
pathCache_after_prepend.update({k: v for k, v in enumerate(pathCache_after_append)}) # numeric keys 0,1,2datasources_after_prepend = {}
datasources_after_prepend['prependfs'] = 'PrependFileDatasource'
datasources_after_prepend.update(datasources_after_append)print("AFTER prependDatasource('prependfs', ds)")
print(f"pathCache keys (mixed): {list(pathCache_after_prepend.keys())}")
print(f"datasources keys: {list(datasources_after_prepend.keys())}")
print()Now the problem:
In getDatasourceForPath, line 266: foreach ($this->pathCache as $i => $paths)
This iterates with BOTH numeric keys (0,1,2) and string keys ('prependfs')
print("getDatasourceForPath after prepend:")
print("Iteration through pathCache:")
for i, (k, v) in enumerate(pathCache_after_prepend.items()):
print(f" $i = {repr(k)}, $paths = {v}")
print()Line 282: $datasourceIndex = array_keys($this->datasources)[$datasourceIndex];
If we found path at key 'prependfs', we try:
array_keys(datasources)['prependfs'] - ERROR! Can't use string as array index
If we found path at numeric index 0 (second iteration), we get:
array_keys(datasources)[0] = 'prependfs' - but that's the PREPENDED one, not the original!
print("The bug:")
print(f"If found at key='prependfs': array_keys(datasources)['prependfs'] - INVALID (string index)")
print(f"If found at numeric 0: array_keys(datasources)[0] = '{list(datasources_after_prepend.keys())[0]}' - WRONG order!")
print()print("In getValidPaths, line 307 after array_reverse:")
print(f"After array_reverse(pathCache), keys persist: {list(reversed(list(pathCache_after_prepend.keys())))}")
print(f"datasources keys: {list(datasources_after_prepend.keys())}")
print()
print("Line 309: if ($this->singleDatasourceMode && $datasourceKey !== $this->activeDatasourceKey)")
print(f"If activeDatasourceKey='db' and datasourceKey=0 (numeric), comparison '0' !== 'db' works but semantically wrong")PYTHON_EOF
Now let me check if there's any actual usage in the codebase:
More comprehensive search for these methods
echo "Searching for any calls to these methods:"
rg "prependDatasource|appendDatasource" --type php
LukeTowers
added
the
maintenance
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
modules/cms/classes/AutoDatasource.php (1)
127-138: Add PHPDoc and simplify the method.The method lacks documentation and can be simplified by removing the unnecessary intermediate variable and using early returns.
Apply this diff to add documentation and simplify:
+ /** + * Fetch path cache for a datasource, bypassing cache in debug mode. + * + * When app.debug is enabled, this returns live paths without caching + * to ensure developers see up-to-date file system changes immediately. + * + * @param DatasourceInterface $datasource + * @return array + */ protected function fetchPathCache(DatasourceInterface $datasource): array { - $pathCache = []; if (Config::get('app.debug', false)) { - $pathCache = $datasource->getAvailablePaths(); - } else { - $pathCache = Cache::rememberForever($datasource->getPathsCacheKey(), function () use ($datasource) { - return $datasource->getAvailablePaths(); - }); + return $datasource->getAvailablePaths(); } - return $pathCache; + + return Cache::rememberForever($datasource->getPathsCacheKey(), function () use ($datasource) { + return $datasource->getAvailablePaths(); + }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/cms/classes/AutoDatasource.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/cms/classes/AutoDatasource.php (1)
modules/system/helpers/Cache.php (1)
Cache(8-84)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: windows-latest / PHP 8.4
- GitHub Check: windows-latest / PHP 8.3
- GitHub Check: windows-latest / PHP 8.1
- GitHub Check: ubuntu-latest / PHP 8.3
- GitHub Check: windows-latest / PHP 8.2
- GitHub Check: ubuntu-latest / PHP 8.4
- GitHub Check: ubuntu-latest / PHP 8.2
- GitHub Check: ubuntu-latest / PHP 8.1
- GitHub Check: windows-latest / JavaScript
🔇 Additional comments (3)
modules/cms/classes/AutoDatasource.php (3)
13-13: LGTM!The Config facade import is necessary for the new debug-aware caching functionality.
80-80: LGTM! Good refactoring.Centralizing the cache retrieval logic in
fetchPathCacheimproves maintainability and makes the debug mode behavior consistent across all datasource operations.Also applies to: 89-89
122-125: LGTM!The refactoring to use
fetchPathCachemaintains the existing behavior while centralizing the caching decision logic.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.