Skip to content

Invalidation event type proposal#44

Open
jan888adams wants to merge 3 commits into
invalidation-event-typefrom
invalidation-event-type-proposal
Open

Invalidation event type proposal#44
jan888adams wants to merge 3 commits into
invalidation-event-typefrom
invalidation-event-type-proposal

Conversation

@jan888adams
Copy link
Copy Markdown
Contributor

@jan888adams jan888adams commented Mar 1, 2026

Summary by CodeRabbit

  • Tests

    • Enhanced test coverage for cache invalidation event handling with explicit event type validation.
  • Refactor

    • Improved code organization and test infrastructure with property name clarification.
  • Style

    • Minor code formatting adjustments.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 1, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch invalidation-event-type-proposal

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

- 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>
@jan888adams jan888adams force-pushed the invalidation-event-type-proposal branch from 0b13633 to 9ed6752 Compare March 1, 2026 09:02
@jan888adams jan888adams changed the base branch from main to invalidation-event-type March 1, 2026 09:06
@jan888adams jan888adams marked this pull request as ready for review March 1, 2026 09:09
Copilot AI review requested due to automatic review settings March 1, 2026 09:09
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Introduces an explicit EventType for element invalidation events and updates unit tests to validate the dispatched event type for update vs delete operations.

Changes:

  • Update ElementInvalidationEvent::fromElement(...) usages in tests to pass the new EventType (Update/Delete).
  • Add new unit tests asserting that InvalidateElementListener dispatches invalidation events with the correct type.
  • Rename a test property in TraceableResponseTaggerTest (currently inconsistent with remaining usages).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
tests/Unit/Element/InvalidateElementListenerTest.php Passes EventType to ElementInvalidationEvent::fromElement and adds assertions for dispatched event type.
tests/Unit/Cache/ResponseTagger/TraceableResponseTaggerTest.php Renames the tagger property, but the rename is incomplete and breaks the test.
src/Element/EventType.php Adjusts file header formatting while defining the EventType enum (Update/Delete).
Comments suppressed due to low confidence (1)

tests/Unit/Cache/ResponseTagger/TraceableResponseTaggerTest.php:26

  • The property was renamed to $traceableResponseTagger, but the rest of the test still assigns to and uses $collectTagsResponseTagger. This will result in an undefined property / failing tests. Please update setUp() and all usages to consistently use the new property name (or keep the old name everywhere).
    private TraceableResponseTagger $traceableResponseTagger;

    /** @var ObjectProphecy<ResponseTagger> */
    private ObjectProphecy $innerTagger;

    protected function setUp(): void
    {
        $this->innerTagger = $this->prophesize(ResponseTagger::class);
        $this->collectTagsResponseTagger = new TraceableResponseTagger($this->innerTagger->reveal());
    }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@tests/Unit/Cache/ResponseTagger/TraceableResponseTaggerTest.php`:
- Line 17: The typed property TraceableResponseTagger $traceableResponseTagger
is declared but the tests use the wrong runtime property name
$collectTagsResponseTagger; update all usages to match the declared property by
replacing every $collectTagsResponseTagger with $traceableResponseTagger in
setUp() and the tests tag_should_collect_tags,
tag_should_forward_tags_to_inner_tagger, and reset_should_reset_collected_tags
so the typed property is actually used and no runtime property access issues
occur.

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0b13633 and 9ed6752.

📒 Files selected for processing (3)
  • src/Element/EventType.php
  • tests/Unit/Cache/ResponseTagger/TraceableResponseTaggerTest.php
  • tests/Unit/Element/InvalidateElementListenerTest.php

Comment thread tests/Unit/Cache/ResponseTagger/TraceableResponseTaggerTest.php
Jan Adams and others added 2 commits March 1, 2026 11:41
… to traceableResponseTagger

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants