Allow distinguishing update from delete invalidations - #40
Merged
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: WalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Element as Element (Pimcore)
participant Listener as InvalidateElementListener
participant Event as ElementInvalidationEvent
participant Dispatcher as EventDispatcher
alt Update Operation
Element->>Listener: onUpdate(event)
Listener->>Listener: invalidateElement(element, EventType::Update)
else Delete Operation
Element->>Listener: onDelete(event)
Listener->>Listener: invalidateElement(element, EventType::Delete)
end
Listener->>Event: ElementInvalidationEvent::fromElement(element, type)
Event->>Event: __construct(type, element, elementType, cacheTags)
Listener->>Dispatcher: dispatch(event)
Dispatcher->>Dispatcher: Event contains type information
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Unit/Cache/ResponseTagger/TraceableResponseTaggerTest.php (1)
17-26:⚠️ Potential issue | 🔴 CriticalIncomplete property rename causes test failures.
The property was renamed to
$traceableResponseTaggeron line 17, but all usages throughout the file still reference the old name$collectTagsResponseTagger. This includes:
- Line 25:
setUp()assigns to the old property name- Lines 33, 41, 55, 65, 71, 74: Test methods use the old property name
The tests will fail or behave unexpectedly because
$traceableResponseTaggeris never assigned.🐛 Proposed fix: complete the rename
protected function setUp(): void { $this->innerTagger = $this->prophesize(ResponseTagger::class); - $this->collectTagsResponseTagger = new TraceableResponseTagger($this->innerTagger->reveal()); + $this->traceableResponseTagger = new TraceableResponseTagger($this->innerTagger->reveal()); }And update all test method usages from
$this->collectTagsResponseTaggerto$this->traceableResponseTagger:public function tag_should_collect_tags(): void { - $this->collectTagsResponseTagger->tag( + $this->traceableResponseTagger->tag( new CacheTags( CacheTag::fromString('tag1'), CacheTag::fromString('tag2'), )); self::assertSame( 'tag1,tag2', - $this->collectTagsResponseTagger->recordedTags->toString(), + $this->traceableResponseTagger->recordedTags->toString(), ); }Apply the same change to
tag_should_forward_tags_to_inner_tagger(line 55) andreset_should_reset_collected_tags(lines 65, 71, 74).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Cache/ResponseTagger/TraceableResponseTaggerTest.php` around lines 17 - 26, The property was partially renamed to $traceableResponseTagger but setUp() and all test methods still reference $collectTagsResponseTagger; update setUp() to assign $this->traceableResponseTagger = new TraceableResponseTagger($this->innerTagger->reveal()) and replace every occurrence of $this->collectTagsResponseTagger in test methods (including tag_should_forward_tags_to_inner_tagger and reset_should_reset_collected_tags) with $this->traceableResponseTagger so the instantiated object is used consistently.
🧹 Nitpick comments (8)
.claude/skills/brainstorm/SKILL.md (1)
52-52: Use “multiple-choice” for consistency and grammar polish.At Line 52, hyphenating the compound adjective improves readability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/brainstorm/SKILL.md at line 52, Replace the compound adjective "multiple choice" with the hyphenated form "multiple-choice" in the SKILL.md text where the phrase "multiple choice questions" appears; update the sentence to read "multiple-choice questions" to improve consistency and grammar..claude/skills/php-best-practices/rules/modern-enums.md (1)
82-87: Clarify the$tag->type->typeproperty access.The expression
$tag->type->typeaccesses a nestedtypeproperty twice, which appears confusing or potentially erroneous. If$tag->typealready returns anElementType, it should be used directly.📝 Suggested fix
-return match ($tag->type->type) { +return match ($tag->type) { ElementType::Asset => $this->asset->isEnabled($tag), ElementType::Document => $this->document->isEnabled($tag), ElementType::Object => $this->object->isEnabled($tag), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/php-best-practices/rules/modern-enums.md around lines 82 - 87, The match expression is using a confusing double property access `$tag->type->type`; update the code to use the actual ElementType value directly (e.g. `match ($tag->type)`) or otherwise dereference the correct property so the match operates on an ElementType enum instance; also add a defensive check or typehint where `type` is assigned to ensure `$tag->type` is an ElementType before calling methods like `isEnabled` on `$this->asset`, `$this->document`, and `$this->object`..claude/skills/php-best-practices/rules/solid-interface-segregation.md (2)
68-71: Same undefined$tagsissue in the "Good" example.For consistency and clarity, show where
$tagsoriginates.📝 Suggested fix
public function onUpdate(): void { + $tags = $this->collectTags(); $this->invalidator->invalidate($tags); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/php-best-practices/rules/solid-interface-segregation.md around lines 68 - 71, The onUpdate() method in the "Good" example uses an undefined $tags variable; update onUpdate() so $tags is defined or passed in — e.g., add a parameter to onUpdate(array $tags): void or obtain $tags from a known source (like $this->getTags() or a method call) before calling $this->invalidator->invalidate($tags); ensure the change is applied to the onUpdate() declaration and any call sites so $tags is clearly sourced.
33-37: Undefined variable$tagsin example.The
$tagsvariable is used but not defined within theonUpdatemethod scope, which could confuse readers. Consider showing where$tagscomes from.📝 Suggested fix
public function onUpdate(): void { - $this->cache->invalidate($tags); // Only uses 1 of 5 methods + $tags = $this->collectTags(); // Example: fetch tags from somewhere + $this->cache->invalidate($tags); // Only uses 1 of 5 methods }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/php-best-practices/rules/solid-interface-segregation.md around lines 33 - 37, The example's onUpdate method uses an undefined local variable $tags when calling $this->cache->invalidate($tags); — fix by showing where $tags comes from: either add a method parameter (public function onUpdate(array $tags): void) and use that, reference an existing class property (e.g. $this->tags) initialized elsewhere, or construct/derive $tags inside onUpdate before calling $this->cache->invalidate; update the example to include one of these approaches so $tags is clearly defined for the onUpdate method and matches the CacheInterface::invalidate usage..claude/skills/debug/SKILL.md (1)
81-81: Add language specifier to fenced code block.The fenced code block at line 81 is missing a language identifier, which affects syntax highlighting and rendering.
📝 Suggested fix
-``` +```plaintext For EACH component boundary:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/debug/SKILL.md at line 81, The fenced code block containing the line "For EACH component boundary:" is missing a language specifier; update the opening fence for that block (the triple backticks before "For EACH component boundary:") to include a language identifier such as plaintext (e.g., change ``` to ```plaintext) so the block is properly labeled for syntax highlighting and rendering..claude/skills/tdd/SKILL.md (1)
49-58: Add language specifiers to fenced code blocks.Multiple fenced code blocks are missing language identifiers, which affects rendering and syntax highlighting:
- Line 49: Process flow diagram
- Line 251: Test output
- Line 269: Test output
📝 Suggested fixes
-``` +```text RED (Write failing test)-``` +```text $ composer tests -- --filter it_rejects_empty_cache_tag-``` +```text $ composer tests -- --filter it_rejects_empty_cache_tagAlso applies to: 251-254, 269-272
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/tdd/SKILL.md around lines 49 - 58, Several fenced code blocks in .claude/skills/tdd/SKILL.md are missing language specifiers (e.g., the flow block starting with "RED (Write failing test)" and the test output blocks containing "$ composer tests -- --filter it_rejects_empty_cache_tag"); update each triple-backtick fence to include an appropriate language tag such as text (e.g., ```text) so the process flow and test output blocks render with correct syntax highlighting and formatting for the blocks containing "RED (Write failing test)" and the test output lines.doc/11-claude-code.md (1)
15-38: Consider adding language identifiers to fenced code blocks.Markdownlint flags several code blocks without language specifiers. For conversation/workflow examples, you can use
textor leave them as-is since they represent human-AI dialog rather than executable code. The directory tree at line 15 could benefit fromtextorplaintextfor consistency.Example fix for the directory structure block
-``` +```text CLAUDE.md # Project overview, quick reference, conventions .claude/ ├── rules/ # Always loaded — project conventionsAlso applies to: 52-59, 76-82, 88-95, 101-107, 164-167, 171-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@doc/11-claude-code.md` around lines 15 - 38, Add language identifiers to the fenced code blocks in the CLAUDE.md document—at minimum label the directory tree block that starts with "CLAUDE.md" and ".claude/" as ```text (or ```plaintext) and similarly tag other example/conversation/workflow blocks (e.g., the conversation/workflow examples and other unlabeled snippets) so markdownlint stops flagging them; update the fenced code delimiters throughout the file (including the directory tree and other example blocks referenced in the review) to include the chosen language token..claude/skills/writing-skills/SKILL.md (1)
209-214: Consider using consistent skill name format in cross-references.The examples reference skills as "tdd skill" and "debug skill" (lowercase, no hyphens), but the naming convention (line 91) requires "letters, numbers, and hyphens only", and other examples show hyphenated names like "condition-based-waiting" and "root-cause-tracing".
For clarity and consistency, consider using the actual skill names (e.g., if the TDD skill is named
test-driven-development, reference it that way).📝 Example of consistent formatting
-**REQUIRED SUB-SKILL:** Use tdd skill -**REQUIRED BACKGROUND:** You MUST understand debug skill +**REQUIRED SUB-SKILL:** Use test-driven-development skill +**REQUIRED BACKGROUND:** You MUST understand debugging skillOr if the skill names are simply
tddanddebug:-**REQUIRED SUB-SKILL:** Use tdd skill -**REQUIRED BACKGROUND:** You MUST understand debug skill +**REQUIRED SUB-SKILL:** Use tdd +**REQUIRED BACKGROUND:** You MUST understand debug🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/writing-skills/SKILL.md around lines 209 - 214, Update the inconsistent cross-reference texts "tdd skill" and "debug skill" to match the repository naming convention (letters, numbers, and hyphens only) used elsewhere (see examples "condition-based-waiting" and "root-cause-tracing") — replace "tdd skill" with the canonical skill name (e.g., "test-driven-development" or "tdd" as appropriate) and "debug skill" with the canonical "debug" name, and ensure the "REQUIRED SUB-SKILL" and "REQUIRED BACKGROUND" lines use the exact hyphenated identifiers instead of plain phrases so cross-references are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/brainstorm/SKILL.md:
- Around line 33-42: The fenced code block that begins with "Explore project
context" is missing a language tag (triggers MD040); fix it by adding a language
identifier (e.g., "text") to the opening triple backticks so the block becomes
```text ... ```; update the block in SKILL.md around the "Explore project
context" section to include this tag to satisfy markdownlint.
In @.claude/skills/debug/condition-based-waiting.md:
- Around line 20-27: The "good" example loop using $maxAttempts, $interval,
$result and $cache->get('key') silently exits on timeout; update it to
explicitly handle the timeout after the loop (e.g., check if $result is still
null and then throw an exception, return an error, or log a clear timeout
message) so callers see the failure; apply the same explicit post-loop timeout
handling to the other example using the same variables.
In @.claude/skills/review/SKILL.md:
- Around line 14-19: The two untyped fenced code blocks that start with the
lines "1. SCOPE: Understand what changed and why" and the later block beginning
with "## Review Summary" should include language identifiers to satisfy MD040;
update the first fence to use a language like ```text (or ```markdown) and
update the second fenced block to use ```markdown (or ```text) so both fences
become typed, leaving the inner content unchanged and preserving indentation and
content in the blocks.
---
Outside diff comments:
In `@tests/Unit/Cache/ResponseTagger/TraceableResponseTaggerTest.php`:
- Around line 17-26: The property was partially renamed to
$traceableResponseTagger but setUp() and all test methods still reference
$collectTagsResponseTagger; update setUp() to assign
$this->traceableResponseTagger = new
TraceableResponseTagger($this->innerTagger->reveal()) and replace every
occurrence of $this->collectTagsResponseTagger in test methods (including
tag_should_forward_tags_to_inner_tagger and reset_should_reset_collected_tags)
with $this->traceableResponseTagger so the instantiated object is used
consistently.
---
Nitpick comments:
In @.claude/skills/brainstorm/SKILL.md:
- Line 52: Replace the compound adjective "multiple choice" with the hyphenated
form "multiple-choice" in the SKILL.md text where the phrase "multiple choice
questions" appears; update the sentence to read "multiple-choice questions" to
improve consistency and grammar.
In @.claude/skills/debug/SKILL.md:
- Line 81: The fenced code block containing the line "For EACH component
boundary:" is missing a language specifier; update the opening fence for that
block (the triple backticks before "For EACH component boundary:") to include a
language identifier such as plaintext (e.g., change ``` to ```plaintext) so the
block is properly labeled for syntax highlighting and rendering.
In @.claude/skills/php-best-practices/rules/modern-enums.md:
- Around line 82-87: The match expression is using a confusing double property
access `$tag->type->type`; update the code to use the actual ElementType value
directly (e.g. `match ($tag->type)`) or otherwise dereference the correct
property so the match operates on an ElementType enum instance; also add a
defensive check or typehint where `type` is assigned to ensure `$tag->type` is
an ElementType before calling methods like `isEnabled` on `$this->asset`,
`$this->document`, and `$this->object`.
In @.claude/skills/php-best-practices/rules/solid-interface-segregation.md:
- Around line 68-71: The onUpdate() method in the "Good" example uses an
undefined $tags variable; update onUpdate() so $tags is defined or passed in —
e.g., add a parameter to onUpdate(array $tags): void or obtain $tags from a
known source (like $this->getTags() or a method call) before calling
$this->invalidator->invalidate($tags); ensure the change is applied to the
onUpdate() declaration and any call sites so $tags is clearly sourced.
- Around line 33-37: The example's onUpdate method uses an undefined local
variable $tags when calling $this->cache->invalidate($tags); — fix by showing
where $tags comes from: either add a method parameter (public function
onUpdate(array $tags): void) and use that, reference an existing class property
(e.g. $this->tags) initialized elsewhere, or construct/derive $tags inside
onUpdate before calling $this->cache->invalidate; update the example to include
one of these approaches so $tags is clearly defined for the onUpdate method and
matches the CacheInterface::invalidate usage.
In @.claude/skills/tdd/SKILL.md:
- Around line 49-58: Several fenced code blocks in .claude/skills/tdd/SKILL.md
are missing language specifiers (e.g., the flow block starting with "RED (Write
failing test)" and the test output blocks containing "$ composer tests --
--filter it_rejects_empty_cache_tag"); update each triple-backtick fence to
include an appropriate language tag such as text (e.g., ```text) so the process
flow and test output blocks render with correct syntax highlighting and
formatting for the blocks containing "RED (Write failing test)" and the test
output lines.
In @.claude/skills/writing-skills/SKILL.md:
- Around line 209-214: Update the inconsistent cross-reference texts "tdd skill"
and "debug skill" to match the repository naming convention (letters, numbers,
and hyphens only) used elsewhere (see examples "condition-based-waiting" and
"root-cause-tracing") — replace "tdd skill" with the canonical skill name (e.g.,
"test-driven-development" or "tdd" as appropriate) and "debug skill" with the
canonical "debug" name, and ensure the "REQUIRED SUB-SKILL" and "REQUIRED
BACKGROUND" lines use the exact hyphenated identifiers instead of plain phrases
so cross-references are consistent.
In `@doc/11-claude-code.md`:
- Around line 15-38: Add language identifiers to the fenced code blocks in the
CLAUDE.md document—at minimum label the directory tree block that starts with
"CLAUDE.md" and ".claude/" as ```text (or ```plaintext) and similarly tag other
example/conversation/workflow blocks (e.g., the conversation/workflow examples
and other unlabeled snippets) so markdownlint stops flagging them; update the
fenced code delimiters throughout the file (including the directory tree and
other example blocks referenced in the review) to include the chosen language
token.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
.claude/rules/architecture.md.claude/rules/bundle-usage.md.claude/rules/code-style.md.claude/rules/roles.md.claude/rules/static-analysis.md.claude/rules/testing.md.claude/skills/brainstorm/SKILL.md.claude/skills/code-review/SKILL.md.claude/skills/context7/SKILL.md.claude/skills/debug/SKILL.md.claude/skills/debug/condition-based-waiting.md.claude/skills/debug/defense-in-depth.md.claude/skills/debug/root-cause-tracing.md.claude/skills/humanizer/SKILL.md.claude/skills/php-best-practices/SKILL.md.claude/skills/php-best-practices/rules/error-custom-exceptions.md.claude/skills/php-best-practices/rules/modern-constructor-promotion.md.claude/skills/php-best-practices/rules/modern-enums.md.claude/skills/php-best-practices/rules/modern-first-class-callables.md.claude/skills/php-best-practices/rules/modern-match-expression.md.claude/skills/php-best-practices/rules/modern-readonly-properties.md.claude/skills/php-best-practices/rules/solid-dependency-inversion.md.claude/skills/php-best-practices/rules/solid-interface-segregation.md.claude/skills/php-best-practices/rules/solid-single-responsibility.md.claude/skills/php-best-practices/rules/type-strict-mode.md.claude/skills/php-pro/SKILL.md.claude/skills/php-pro/references/async-patterns.md.claude/skills/php-pro/references/laravel-patterns.md.claude/skills/php-pro/references/modern-php-features.md.claude/skills/php-pro/references/symfony-patterns.md.claude/skills/php-pro/references/testing-quality.md.claude/skills/review/SKILL.md.claude/skills/review/checklist.md.claude/skills/tdd/SKILL.md.claude/skills/web-search/SKILL.md.claude/skills/writing-skills/SKILL.mdCLAUDE.mdREADME.mddoc/11-claude-code.mdsrc/Element/EventType.phptests/Unit/Cache/ResponseTagger/TraceableResponseTaggerTest.phptests/Unit/Element/InvalidateElementListenerTest.php
✅ Files skipped from review due to trivial changes (7)
- .claude/skills/debug/defense-in-depth.md
- .claude/skills/debug/root-cause-tracing.md
- README.md
- .claude/skills/php-pro/references/laravel-patterns.md
- .claude/rules/testing.md
- .claude/skills/php-pro/references/testing-quality.md
- .claude/skills/php-best-practices/rules/error-custom-exceptions.md
Contributor
|
I’m not a fan of the MR title. It explains the changes, but it doesn’t clearly describe the feature. |
Contributor
|
I created a brach, with testcases and documentation: for this MR: |
- Fix blank line after <?php in EventType.php - Rename $collectTagsResponseTagger to $traceableResponseTagger in TraceableResponseTaggerTest - Fix broken fromElement() calls in InvalidateElementListenerTest (missing EventType arg) - Add tests verifying EventType::Update and EventType::Delete are dispatched correctly Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Jan Adams <j.adams@ecodar.de>
jdreesen
force-pushed
the
invalidation-event-type
branch
from
August 31, 2026 09:02
ab795c9 to
91f8412
Compare
Addresses code review feedback: InvalidationType stays unbacked (unlike ElementType, its value never ends up in a cache tag), and fromElement()/invalidateElement() now take the type before the element, consistent with the constructor's parameter order.
jdreesen
changed the title
EventType info to ElementInvalidationEvent
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
2 participants
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.
ElementInvalidationEventlisteners previously had no way to tell whether the invalidation was triggered by a save or a delete. This adds that information so listeners can react differently to each case.Changes
ElementInvalidationEventnow exposes atypeproperty of the newInvalidationTypeenum (UpdateorDelete).InvalidateElementListenerpasses the correct type when dispatching the event on save/delete.doc/4-events.md) and tests updated accordingly.Usage