Conveniently disable auto tagging proposal#43
Conveniently disable auto tagging proposal#43jan888adams wants to merge 16 commits intoconveniently-disable-auto-taggingfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
…internal - Fix 0-based index in error message (was 1-based due to pre-increment) - Use separate accumulator so partial tags are never applied on invalid yield - Guard tag() call in finally with !isEmpty() to avoid unnecessary FOSHttpCache calls - Fix PHPDoc Generator key type from int to array-key - Add @internal to CacheType interface to signal it is not for external implementation - Update CacheActivatorTest: fix broken constructor, add ProphecyTrait, add 10 tests covering all withoutAutomaticTagging() code paths Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Make TraceableResponseTagger::$recordedTags private, expose via getRecordedTags() - Fix missing declare(strict_types=1) inline style (blank line after <?php) - Rename $collectTagsResponseTagger to $traceableResponseTagger in test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
CacheTags is immutable and TraceableResponseTagger is final, so a getter adds ceremony with no encapsulation benefit. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
f3f5a7d to
823ab45
Compare
…sage The generator key already identifies the problematic yield sufficiently. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR refines the CacheActivator::withoutAutomaticTagging() behavior to avoid partial tag application on generator failure, fixes a PHPDoc generator key type, aligns file header formatting with project conventions, and expands unit test coverage for withoutAutomaticTagging().
Changes:
- Prevent partial tag application by accumulating yielded tags and applying them only after successful generator iteration.
- Fix PHPDoc generator key type from
inttoarray-keyand simplify the invalid-yield error message. - Add/extend unit tests for
withoutAutomaticTagging()and adjust test naming.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/CacheActivator.php |
Accumulates generator yields before applying tags; updates PHPDoc and error message. |
tests/Unit/CacheActivatorTest.php |
Adds unit coverage for withoutAutomaticTagging() including invalid-yield scenarios. |
tests/Unit/Cache/ResponseTagger/TraceableResponseTaggerTest.php |
Renames the test subject property (but currently inconsistently applied). |
src/Cache/ResponseTagger/TraceableResponseTagger.php |
Adjusts header formatting to <?php declare(strict_types=1); on one line. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…tentional Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… to traceableResponseTagger Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ield The generator key alone is ambiguous — a key of 1 could mean the first or second yield depending on whether keys are explicit. The position counter (0-based iteration count) provides unambiguous location information independent of the generator key. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Tests cover: - Asset loaded inside the block is not automatically tagged - Tag manually yielded inside the block does appear in the response Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…, int cast Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…gging Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/8-disable-caching-behavior.md`:
- Line 55: The sentence describing state restoration uses "afterwards" but
should use US English "afterward"; update the wording in the doc sentence that
mentions the caching state being restored after the block completes (the line
referencing withoutAutomaticTagging()) to replace "afterwards" with "afterward"
so the sentence reads "...it remains disabled afterward."
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
doc/8-disable-caching-behavior.mdsrc/Cache/ResponseTagger/TraceableResponseTagger.phpsrc/CacheActivator.phptests/Integration/Fixtures/without_automatic_tagging_route.phptests/Integration/Tagging/WithoutAutomaticTaggingTest.phptests/Unit/Cache/ResponseTagger/TraceableResponseTaggerTest.phptests/Unit/CacheActivatorTest.phptests/app/src/Controller/WithoutAutomaticTaggingController.php
|
|
||
| ### State restoration | ||
|
|
||
| The previous caching state is always restored after the block completes, even if an exception is thrown. If caching was already disabled before calling `withoutAutomaticTagging()`, it remains disabled afterwards. |
There was a problem hiding this comment.
Use “afterward” for US English consistency.
Replace “afterwards” with “afterward”.
✏️ Proposed wording fix
-...it remains disabled afterwards.
+...it remains disabled afterward.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The previous caching state is always restored after the block completes, even if an exception is thrown. If caching was already disabled before calling `withoutAutomaticTagging()`, it remains disabled afterwards. | |
| The previous caching state is always restored after the block completes, even if an exception is thrown. If caching was already disabled before calling `withoutAutomaticTagging()`, it remains disabled afterward. |
🧰 Tools
🪛 LanguageTool
[locale-violation] ~55-~55: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ...utomaticTagging()`, it remains disabled afterwards.
(AFTERWARDS_US)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@doc/8-disable-caching-behavior.md` at line 55, The sentence describing state
restoration uses "afterwards" but should use US English "afterward"; update the
wording in the doc sentence that mentions the caching state being restored after
the block completes (the line referencing withoutAutomaticTagging()) to replace
"afterwards" with "afterward" so the sentence reads "...it remains disabled
afterward."
|
|
||
| ## Suppress automatic tagging for a specific code block | ||
|
|
||
| Sometimes you need to load Pimcore elements for business logic without tagging the current response with those elements. For example, loading related products to calculate a price should not cause the response to be tagged with all those products. |
There was a problem hiding this comment.
I guess this isn’t the best example..
Summary by CodeRabbit
Documentation
New Features