Scope cache activation#48
Open
jdreesen wants to merge 8 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)
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughCacheScope replaces direct CacheActivator state management, with HTTP and console listeners controlling scope activation. Configuration, decorators, integration tests, unit tests, documentation, and compatibility deprecations are updated for the new cache-scope model. ChangesCacheScope activation and compatibility
Documentation and release metadata
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Request
participant HttpCacheScopeListener
participant CacheScope
participant DocumentResolver
participant ResponseTagger
Request->>HttpCacheScopeListener: kernel request/controller event
HttpCacheScopeListener->>CacheScope: enable() when request is cacheable
HttpCacheScopeListener->>DocumentResolver: resolve fallback document when configured
DocumentResolver-->>HttpCacheScopeListener: document or null
HttpCacheScopeListener->>ResponseTagger: tag route element and fallback document
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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
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.
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:
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.,
Summary by CodeRabbit
New Features
Bug Fixes
Documentation