Uh oh!
There was an error while loading. Please reload this page.
Fix #272: Add hasStatusSupport() to AdapterInterface - #299
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## master #299 +/- ##
========================================
Coverage 0.00% 0.00% Complexity 330 330 ========================================
Files 49 49 Lines 910 910 ========================================
Misses 910 910 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
WarLikeLaux
commented
Jun 5, 2026
@coderabbitai full review |
✅ Action performedFull review finished. |
📝 WalkthroughWalkthroughThis PR adds a ChangesStatus tracking capability reporting
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/Benchmark/Support/VoidAdapter.php (1)
29-32:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix
VoidAdapter::status()to respecthasStatusSupport()contract.tests/Benchmark/Support/VoidAdapter.phpreturnsfalsefromhasStatusSupport(), butstatus()currently throwsInvalidArgumentException, contradictingAdapterInterface’s contract thatstatus()must always returnMessageStatus::NOT_FOUNDwhen status tracking isn’t supported. Benchmarks don’t call->status(...)(andVoidAdapteris only used byQueueBench), so this likely doesn’t affect current benchmark runs, but it should still be corrected for contract compliance.🤖 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/Benchmark/Support/VoidAdapter.php` around lines 29 - 32, VoidAdapter::status() currently throws InvalidArgumentException even though hasStatusSupport() returns false; update the status(int|string $id): MessageStatus implementation to return MessageStatus::NOT_FOUND when status tracking is not supported (i.e., when hasStatusSupport() is false) instead of throwing. Locate the VoidAdapter class and modify the status method to respect hasStatusSupport(), returning MessageStatus::NOT_FOUND for unsupported adapters in accordance with AdapterInterface, while still allowing a real status lookup if hasStatusSupport() were true.tests/App/FakeAdapter.php (1)
31-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix
FakeAdapter::status()to satisfy theAdapterInterfacecontract when status isn’t supported
AdapterInterfaceexpectsstatus()to returnMessageStatus::NOT_FOUNDwheneverhasStatusSupport()isfalse, buttests/App/FakeAdapter.phpcurrently throws an exception instead:publicfunctionstatus(string|int$id): MessageStatus { thrownewException('`status()` method is not implemented yet.'); }Even for a test double, this violates the interface contract/LSP. Implement
status()to returnMessageStatus::NOT_FOUNDwhenhasStatusSupport()isfalse(and keep throwing only if you intentionally want to fail callers in supported scenarios). The test suite contains->status()calls, but the grep results don’t show anyFakeAdapter::status()usage.🤖 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/App/FakeAdapter.php` around lines 31 - 34, FakeAdapter::status() currently throws an exception which violates AdapterInterface; change it so that it checks $this->hasStatusSupport() and returns MessageStatus::NOT_FOUND when hasStatusSupport() is false, and only throw or perform real status logic when status support is enabled—this ensures FakeAdapter::status() follows the AdapterInterface contract and LSP.
🤖 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.
Outside diff comments:
In `@tests/App/FakeAdapter.php`:
- Around line 31-34: FakeAdapter::status() currently throws an exception which
violates AdapterInterface; change it so that it checks $this->hasStatusSupport()
and returns MessageStatus::NOT_FOUND when hasStatusSupport() is false, and only
throw or perform real status logic when status support is enabled—this ensures
FakeAdapter::status() follows the AdapterInterface contract and LSP.
In `@tests/Benchmark/Support/VoidAdapter.php`:
- Around line 29-32: VoidAdapter::status() currently throws
InvalidArgumentException even though hasStatusSupport() returns false; update
the status(int|string $id): MessageStatus implementation to return
MessageStatus::NOT_FOUND when status tracking is not supported (i.e., when
hasStatusSupport() is false) instead of throwing. Locate the VoidAdapter class
and modify the status method to respect hasStatusSupport(), returning
MessageStatus::NOT_FOUND for unsupported adapters in accordance with
AdapterInterface, while still allowing a real status lookup if
hasStatusSupport() were true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cb9429ca-b294-4dbf-b85b-fc3574b0aeda
📒 Files selected for processing (6)
CHANGELOG.mdsrc/Adapter/AdapterInterface.phpstubs/InMemoryAdapter.phptests/App/FakeAdapter.phptests/Benchmark/Support/VoidAdapter.phptests/Unit/Stubs/InMemoryAdapterTest.php
samdark
left a comment
There was a problem hiding this comment.
Needs pull requests for yiisoft/queue-redis and yiisoft/queue-amqp before it can be merged.
Uh oh!
There was an error while loading. Please reload this page.
vjik
commented
Jun 7, 2026
What is new method planned to be used for? |
samdark
commented
Jun 7, 2026
@vjik Very good question. Usually that is meant for something like progress indicators and admin panels for a queue to look after tasks, etc. but it seems to be not enough for any functionality that is meaningful to the end user. See the summarizer demo where I had to mark processing steps and save statuses in DB:
So since that's usually not enough anyway, we may get rid of the concept of message status overall. |
vjik
commented
Jun 7, 2026
What about adding new status "UNKNOWN" that means adapter don't track status? |
I mean that likely we don't need status tracking at the adapter level at all. |
…atusSupport # Conflicts: # tests/App/FakeAdapter.php
What does this PR do?
Adds
AdapterInterface::hasStatusSupport(): boolso callers can tell an adapter that does not track status apart from a missing message ID, since both cases returnMessageStatus::NOT_FOUNDfromstatus().InMemoryAdapterreturnstrue; the test stubsFakeAdapterandVoidAdapterreturnfalse. Since those stubs declare no status support, theirstatus()now returnsMessageStatus::NOT_FOUNDinstead of throwing, which is whatAdapterInterface::status()documents for adapters without status tracking.BC
The new method on
AdapterInterfacerequires every implementation to declare it. The package is1.0.0 under developmentand the signature was proposed by @samdark in the issue.Summary by CodeRabbit
New Features
Documentation
Tests
Driver adaptation
hasStatusSupport()toAdapterqueue-redis#13hasStatusSupport()toAdapterqueue-amqp#129