Scope cache activation - #48
Open
jdreesen wants to merge 11 commits into
Open
Conversation
Until now, the cache tagging/invalidation was active by default, and you could disable and enable it again. From now on, the cache tagging/invalidation is inactive by default. It will be enabled for certain scopes automatically: - in console mode it will be activated via a `ConsoleEvents::COMMAND` listener, so basically nothing changes here - in HTTP mode, users can switch between "request" and "controller" modes - the "request" mode is basically the previous behavior - the "controller" mode scopes the cache to be only active once the controller gets called, so it ignores everything "generic" that happens before The "controller" mode is the new default, as it skips tagging of elements that are loaded *before* the request-specific stuff happens, which means that e.g., - the parent documents don't get tagged anymore if you call a document route (as Pimcore may load all of them to determine e.g., the document's language) - the root document doesn't get tagged anymore if you call a non-document route (as Pimcore loads it as a "fallback" document automatically)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (24)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe PR introduces ChangesCacheScope state and decorators
HTTP and console activation
Validation and documentation
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to The PR changes when cache tagging and invalidation run. A legacy activation path can clear an explicit disable and unexpectedly resume cache operations when both APIs are used together, creating a bounded correctness risk; the change is mergeable with explicit owner follow-up. Sequence Diagram(s)sequenceDiagram
participant Kernel
participant HttpCacheScopeListener
participant CacheScope
participant ResponseTagger
participant DocumentResolver
Kernel->>HttpCacheScopeListener: Dispatch request or controller event
HttpCacheScopeListener->>CacheScope: Enable scope for eligible HTTP flow
HttpCacheScopeListener->>ResponseTagger: Tag route document or data object
HttpCacheScopeListener->>DocumentResolver: Resolve fallback document when enabled
DocumentResolver-->>HttpCacheScopeListener: Return document or null
HttpCacheScopeListener->>ResponseTagger: Tag fallback document
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 70.83% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 96 functions across 26 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches 💡 1📝 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 |
jdreesen
force-pushed
the
scope-cache-activation
branch
from
July 6, 2026 16:54
9a5671b to
e4ccc29
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Integration/Invalidation/CancelInvalidationTest.php (1)
44-136: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEnable the cache scope before each mutation/delete.
InvalidateElementListeneris wired throughOnlyWhenActiveCacheInvalidator, so withCacheScopeleft inactive these assertions never reachinvalidateTags()and the cancel path is not exercised.🤖 Prompt for 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. In `@tests/Integration/Invalidation/CancelInvalidationTest.php` around lines 44 - 136, Enable the cache scope in each of the CancelInvalidationTest mutation/delete cases before calling the relevant factory save, setContent/setKey/setData, or delete operations. The assertions currently depend on InvalidateElementListener being triggered through OnlyWhenActiveCacheInvalidator, so activate CacheScope within each test (or in shared setup for these scenarios) to ensure invalidateTags() is actually reached and the cancel path is exercised.
🧹 Nitpick comments (1)
src/EventListener/HttpCacheScopeListener.php (1)
35-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider documenting the priority value.
priority: 1024is a fairly unusual/high value with no inline explanation of why this listener must run before others onkernel.request. A short comment would help future maintainers avoid accidentally lowering it.🤖 Prompt for 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. In `@src/EventListener/HttpCacheScopeListener.php` around lines 35 - 43, The HttpCacheScopeListener::onKernelRequest event listener uses a high AsEventListener priority value without explaining why it must run early. Add a short inline comment near the priority: 1024 declaration to document that this listener needs to execute before other kernel.request listeners, so future changes to the priority do not break cache scope enabling behavior.
🤖 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 `@config/services.php`:
- Around line 36-43: The deprecated cache_activator service is currently
instantiated separately from cache_scope, so its deactivateCaching state does
not affect the shared cache state used elsewhere. Update the service wiring in
services.php so CacheActivator resolves to the same CacheScope instance as
neusta_pimcore_http_cache.cache_scope, keeping the deprecated alias while
delegating behavior to the shared service. Focus on the service definitions for
CacheActivator and CacheScope so both names point to one underlying object.
In `@src/CacheActivator.php`:
- Around line 5-33: `CacheActivator` currently extends `CacheScope` but is wired
as a separate service, so its `activateCaching()` and `deactivateCaching()`
calls mutate only an isolated instance. Update `CacheActivator` to delegate to
the shared `CacheScope` service instance used elsewhere (for example via
composition or service aliasing), and keep the legacy methods
(`isCachingActive()`, `activateCaching()`, `deactivateCaching()`) forwarding to
that shared scope so they control the same cache state as the
listeners/decorators.
---
Outside diff comments:
In `@tests/Integration/Invalidation/CancelInvalidationTest.php`:
- Around line 44-136: Enable the cache scope in each of the
CancelInvalidationTest mutation/delete cases before calling the relevant factory
save, setContent/setKey/setData, or delete operations. The assertions currently
depend on InvalidateElementListener being triggered through
OnlyWhenActiveCacheInvalidator, so activate CacheScope within each test (or in
shared setup for these scenarios) to ensure invalidateTags() is actually reached
and the cancel path is exercised.
---
Nitpick comments:
In `@src/EventListener/HttpCacheScopeListener.php`:
- Around line 35-43: The HttpCacheScopeListener::onKernelRequest event listener
uses a high AsEventListener priority value without explaining why it must run
early. Add a short inline comment near the priority: 1024 declaration to
document that this listener needs to execute before other kernel.request
listeners, so future changes to the priority do not break cache scope enabling
behavior.
🪄 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: CHILL
Plan: Pro
Run ID: f4ac6839-32a0-4b33-be86-bce938c3bf93
📒 Files selected for processing (28)
config/services.phpdoc/8-disable-caching-behavior.mdsrc/Cache/CacheInvalidator/OnlyWhenActiveCacheInvalidator.phpsrc/Cache/ResponseTagger/OnlyWhenActiveResponseTagger.phpsrc/CacheActivator.phpsrc/CacheScope.phpsrc/DependencyInjection/Configuration.phpsrc/DependencyInjection/NeustaPimcoreHttpCacheExtension.phpsrc/EventListener/ConsoleCacheScopeListener.phpsrc/EventListener/HttpCacheScopeListener.phptests/Integration/Configuration/CollectConfigurationDataTest.phptests/Integration/Helpers/ArrangeCacheTest.phptests/Integration/Invalidation/CancelInvalidationTest.phptests/Integration/Invalidation/InvalidateAdditionalTagTest.phptests/Integration/Invalidation/InvalidateAssetTest.phptests/Integration/Invalidation/InvalidateDocumentTest.phptests/Integration/Invalidation/InvalidateObjectTest.phptests/Integration/Tagging/CollectTagsDataTest.phptests/Integration/Tagging/TagAdditionalTagTest.phptests/Integration/Tagging/TagAssetTest.phptests/Integration/Tagging/TagDocumentTest.phptests/Integration/Tagging/TagObjectTest.phptests/Unit/Cache/CacheInvalidator/OnlyWhenActiveCacheInvalidatorTest.phptests/Unit/Cache/ResponseTagger/OnlyWhenActiveResponseTaggerTest.phptests/Unit/CacheActivatorTest.phptests/Unit/CacheScopeTest.phptests/Unit/EventListener/HttpCacheScopeListenerTest.phptests/app/config/services.php
💤 Files with no reviewable changes (2)
- tests/Integration/Helpers/ArrangeCacheTest.php
- tests/app/config/services.php
…_cache.cache_activator` point to the same service
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
jdreesen
force-pushed
the
scope-cache-activation
branch
3 times, most recently
from
July 8, 2026 15:14
1f9ee1f to
3d924c8
Compare
jdreesen
force-pushed
the
scope-cache-activation
branch
from
July 8, 2026 15:15
3d924c8 to
9fd3192
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/CacheScope.php (1)
21-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider guarding
startCollecting()againstdisabledForRequestfor docblock consistency.The docblock states "Has no effect if
disableForRequest()was already called," but the method unconditionally sets$enabled = true. WhileisActive()correctly returnsfalsein that case, adding an early return would make the internal state match the documented contract.♻️ Optional refactor
public function startCollecting(): void { + if ($this->disabledForRequest) { + return; + } $this->enabled = true; }🤖 Prompt for 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. In `@src/CacheScope.php` around lines 21 - 24, Update startCollecting() to return immediately when disabledForRequest indicates collection was disabled for the request; otherwise set enabled to true, keeping the internal state consistent with the method’s documented no-op behavior.
🤖 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/CacheScope.php`:
- Around line 16-24: Fix the malformed PHPDoc `@see` annotation in
CacheScope::startCollecting by replacing the closing ] after disableForRequest()
with the required }, producing a valid {`@see` ...} reference.
---
Nitpick comments:
In `@src/CacheScope.php`:
- Around line 21-24: Update startCollecting() to return immediately when
disabledForRequest indicates collection was disabled for the request; otherwise
set enabled to true, keeping the internal state consistent with the method’s
documented no-op behavior.
🪄 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: CHILL
Plan: Pro
Run ID: 98afd819-3fc3-4410-a714-df879ee263dc
📒 Files selected for processing (15)
CHANGELOG.mdREADME.mddoc/10-error-handling.mddoc/11-profiling.mddoc/12-contribution.mddoc/2-configuration.mddoc/8-cache-scope.mddoc/9-disable-caching-behavior.mdsrc/CacheActivator.phpsrc/CacheScope.phpsrc/EventListener/ConsoleCacheScopeListener.phpsrc/EventListener/HttpCacheScopeListener.phptests/Integration/Invalidation/InvalidateAdditionalTagTest.phptests/Unit/CacheScopeTest.phptests/Unit/EventListener/HttpCacheScopeListenerTest.php
✅ Files skipped from review due to trivial changes (8)
- doc/10-error-handling.md
- doc/2-configuration.md
- doc/12-contribution.md
- doc/11-profiling.md
- README.md
- doc/8-cache-scope.md
- CHANGELOG.md
- doc/9-disable-caching-behavior.md
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/Unit/CacheScopeTest.php
- src/EventListener/ConsoleCacheScopeListener.php
- src/CacheActivator.php
- src/EventListener/HttpCacheScopeListener.php
- tests/Integration/Invalidation/InvalidateAdditionalTagTest.php
- tests/Unit/EventListener/HttpCacheScopeListenerTest.php
Rename `Active` references to `Enabled` throughout codebase Rename `disableForRequest` method to `disable` again Update the docs Refactor `CacheScopeTest` to improve readability and ensure consistent spacing Replace `enable` and `disable` methods with `startCollecting` and `disableForRequest`
jdreesen
force-pushed
the
scope-cache-activation
branch
from
July 10, 2026 18:38
0f42eac to
884148d
Compare
isCachingActive() pointed to a nonexistent CacheScope::isActive(), the trigger_deprecation() package name was missing the "team" prefix, and the deprecation version was stamped 0.7 even though 0.7.0/0.7.1 already shipped without it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CacheScope::isEnabled() gated both tag collection and invalidation, but HttpCacheScopeListener only enables it for cacheable, non-admin GET/HEAD requests. That meant invalidation was silently skipped for exactly the requests that trigger it most (an admin-UI save, or any non-GET endpoint). Add CacheScope::isInvalidating(), which is active by default everywhere and only stops once disable() is called, independently of whether tag collection is enabled for the current request. Wire the invalidator chain to it (OnlyWhenEnabledCacheInvalidator -> OnlyWhenInvalidatingCacheInvalidator) and map the deprecated CacheActivator::isCachingActive() to it, restoring the pre-0.7 CacheActivator default. Revive the pre-rewrite ArrangeCacheTest test helper: now that invalidation runs unconditionally, fixture creation in integration tests needs to suppress it explicitly, the same way it did before CacheScope existed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
reset() runs before enable() to preserve the legacy CacheActivator semantics (deactivate then reactivate always works, per the BC test), but it also clears a prior CacheScope::disable() on the same instance since "disabled" carries no information about which API set it. This can't be fixed without breaking one of the two behaviors, so it's called out instead: don't mix this deprecated API with CacheScope on the same instance. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduces
CacheScope, a runtime switch that controls when cache tagging and invalidation happen.Tagging requires the scope to be explicitly activated for the current request or console command:
scopeconfig option controls when activation happens:request: activated from the very beginning of the request (the previous, pre-CacheScopebehavior).controller(new default): activated only once the controller is invoked, so it ignores everything "generic" that happens before. This means, e.g.:Invalidation is active by default everywhere — including non-cacheable and admin-context requests, such as saving an element via the Pimcore admin UI or through a custom
POSTendpoint — independent of whether tagging is currently activated (seeCacheScope::isInvalidating()).Both tagging and invalidation stop once
CacheScope::disable()is called, for the current request or command.CacheActivatoris deprecated in favor ofCacheScope.Summary by CodeRabbit
New Features
Deprecations
Documentation