Skip to content

Introduce handler resolver - #327

Open
vjik wants to merge 10 commits into
masterfrom
message-handler-resolver
Open

Introduce handler resolver#327
vjik wants to merge 10 commits into
masterfrom
message-handler-resolver

Conversation

@vjik

@vjik vjik commented Aug 14, 2026

Copy link
Copy Markdown
Member
Q A
Is bugfix?
New feature?
Breaks BC? ✔️
Tests pass? ✔️

@codecov

codecov Bot commented Aug 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.50746% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 92.42%. Comparing base (82dd022) to head (1ef1709).

Files with missing lines Patch % Lines
src/Message/Handler/HandlerResolver.php 97.61% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #327      +/-   ##
============================================
- Coverage     92.51%   92.42%   -0.09%     
- Complexity      379      382       +3     
============================================
  Files            55       59       +4     
  Lines           988     1017      +29     
============================================
+ Hits            914      940      +26     
- Misses           74       77       +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@vjik
vjik requested a review from a team August 14, 2026 09:00
@vjik vjik added the status:code review The pull request needs review. label Aug 14, 2026
Comment thread src/Message/Handler/CallableHandler.php
@vjik
vjik requested a review from samdark August 18, 2026 07:13
Comment on lines 74 to 77

if (is_callable($definition)) {
return $definition;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change

Duplicates the new check on line 40

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

CallableFactory is used in MiddlewareFactory also. Left for future refactoring.

return $definition;
}

if (is_string($definition)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Previously it was possible to use callables. Now it always looks into container in case of strings. Worth keeping previous behavior?

Suggested change
if (is_string($definition)) {
if (is_string($definition) && $this->container->has($definition)) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fixed


namespace Yiisoft\Queue\Message;

interface MessageHandlerInterface

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Still used in README and guide.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done

Comment on lines +88 to +90
if (is_string($definition)) {
return $this->getHandlerFromContainer($messageType, $definition);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Needs fallback to callable resolver

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done

@vjik
vjik requested review from samdark and viktorprogger August 31, 2026 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status:code review The pull request needs review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants