Fix: Gracefully handle cache drivers that do not support tagging - #75
Conversation
The default config enables cache tags, but drivers like "file" or "database" do not support tagging. This change: - Types the Cache constructor $tags parameter as array and normalises the config value in GeoIP so null/missing values become an empty array - Replaces the hardcoded driver list in Clear command with a dynamic supportsTags() check that covers all current and future drivers - Updates config comment to explain auto-fallback behaviour - Adds a test covering tagged-cache construction on a non-tagging driver Addresses the same issue as Torann#255.
…sage - Fix test to use file cache driver (which genuinely lacks tag support) instead of array driver, so the fallback path is actually exercised - Log a warning in GeoIP constructor when cache tags are configured but the active driver does not support tagging - Improve geoip:clear error message to explain why selective clearing is unavailable
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the handling of cache tags by allowing them to be automatically ignored if the active cache driver does not support tagging. Key changes include updating the Cache constructor type-hints, refining the Clear command's support check to use the supportsTags() method, and adding a warning log when tags are configured but unsupported. A concern was raised regarding the warning log in GeoIP.php, as it may cause log flooding if the service is instantiated frequently in environments without tag support.
| if ($cacheTags !== [] && !$cache->supportsTags()) { | ||
| $this->logger->warning('GeoIP: cache tags are configured but the active cache driver does not support tagging. Tags will be ignored.'); | ||
| } |
There was a problem hiding this comment.
The warning message is logged every time the GeoIP service is instantiated if the cache driver does not support tags. This could lead to log flooding in production environments. Consider using a more rate-limited approach or logging this only once per request/process lifecycle.
composer.json already requires ^8.3, so 8.2 builds are unnecessary.
The Docker image (oskarstark/php-cs-fixer-ga) does not have access to the project's composer dependencies, causing a fatal error because the custom IxDFCodingStandard\PhpCsFixer\Config class is not found. Use the locally installed php-cs-fixer from vendor/bin instead.
- Replace Docker-based php-cs-fixer with composer scripts - Add Rector auto-fix step alongside php-cs-fixer - Add fork-safe dual-job setup: auto-fix for internal PRs, dry-run check for fork PRs - Add rector:dry and cs:check composer scripts for CI dry-run jobs
2 participants
Context
When the default cache driver does not support tagging (e.g.
file,database), butcache_tagsis configured (which is the default), thegeoip:clearcommand fails with a misleading error. Additionally, there is no indication to the developer that their tag configuration is being silently ignored at runtime.This is the same issue reported in Torann#255, but addressed more robustly at the code level rather than with a config-level workaround.
Solution
Cacheconstructor$tagsparameter asarraywith a@param list<string>annotation, replacing the untyped parametercache_tagsconfig inGeoIPconstructor usingarray_values((array) ...)sonullor missing values safely become an empty array instead of causing errors['file', 'database']) inClear::isSupported()with a dynamicsupportsTags()check that covers all current and future non-tagging driversgeoip:clearerror message to explain why selective clearing is unavailablefilecache driver to genuinely exercise the fallback pathcache_tagstonull