Implement log method #4
Conversation
WalkthroughInjects a RoadRunner AppLogger into RpcLogger and implements PSR-3 level normalization and dispatch in log(); adds a comprehensive unit test suite and a test RPC spy to validate level-to-RPC mappings, message/context handling, and invalid-level behavior. Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant RpcLogger
participant AppLogger as RoadRunner AppLogger
Caller->>RpcLogger: log(level, message, context)
RpcLogger->>RpcLogger: normalize level (string/lowecase)
alt level in {emergency, alert, critical, error}
RpcLogger->>AppLogger: error(message, context)
else level == warning
RpcLogger->>AppLogger: warning(message, context)
else level in {notice, info}
RpcLogger->>AppLogger: info((string) message, context)
else level == debug
RpcLogger->>AppLogger: debug(message, context)
else invalid level
RpcLogger-->>Caller: throw PsrInvalidArgumentException
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code Graph Analysis (1)tests/Unit/RpcLoggerTest.php (2)
🪛 PHPMD (2.15.0)tests/Unit/RpcSpy.php27-27: Avoid unused parameters such as '$codec'. (Unused Code Rules) (UnusedFormalParameter) 32-32: Avoid unused parameters such as '$service'. (Unused Code Rules) (UnusedFormalParameter) 🔇 Additional comments (9)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/RpcLogger.php (4)
17-22: Constructor injection with a readonly dependency looks good; consider property promotion to reduce boilerplate.You can remove the explicit property and assignment by promoting it in the constructor.
- private readonly AppLogger $logger; - - public function __construct(AppLogger $logger) - { - $this->logger = $logger; - } + public function __construct(private readonly AppLogger $logger) + { + }
24-29: Docblock improvements: add @throws and align context key type.
- PSR-3 reserves the right to throw InvalidArgumentException for invalid levels; document it.
- Align the context annotation with the assertion below (string keys).
/** * @param mixed $level - * @param array<array-key, mixed> $context + * @param array<string, mixed> $context + * + * @throws \Psr\Log\InvalidArgumentException * * @link https://www.php-fig.org/psr/psr-3/#5-psrlogloglevel */
36-41: Preserve the original PSR severity when collapsing multiple levels to a single backend level.Since EMERGENCY/ALERT/CRITICAL/ERROR are all mapped to
error(), consider forwarding the original PSR level in context to avoid loss of fidelity.- $this->logger->error((string) $message, $context); + $this->logger->error((string) $message, $context + ['psr_level' => $normalizedLevel]);
56-58: Improve the exception message for diagnostics.Including the original type/value helps debugging invalid inputs.
- throw new PsrInvalidArgumentException('Invalid log level: ' . $normalizedLevel); + throw new PsrInvalidArgumentException( + sprintf( + 'Invalid log level: %s (normalized: %s)', + \is_scalar($level) ? (string) $level : \get_debug_type($level), + $normalizedLevel + ) + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/RpcLogger.php(1 hunks)
🔇 Additional comments (1)
src/RpcLogger.php (1)
9-11: Imports/aliases are appropriate and consistent with PSR-3 usage.No issues spotted.
| $normalizedLevel = \is_string($level) ? \strtolower($level) : (string) $level; | ||
|
|
||
| /** @var array<string, mixed> $context */ | ||
| switch ($normalizedLevel) { | ||
| case PsrLogLevel::EMERGENCY: | ||
| case PsrLogLevel::ALERT: | ||
| case PsrLogLevel::CRITICAL: | ||
| case PsrLogLevel::ERROR: | ||
| $this->logger->error($message, $context); | ||
| return; | ||
|
|
||
| case PsrLogLevel::WARNING: | ||
| $this->logger->warning($message, $context); | ||
| return; | ||
|
|
||
| case PsrLogLevel::NOTICE: | ||
| case PsrLogLevel::INFO: | ||
| $this->logger->info((string) $message, $context); | ||
| return; | ||
|
|
||
| case PsrLogLevel::DEBUG: | ||
| $this->logger->debug($message, $context); | ||
| return; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure message is always a string when calling AppLogger to avoid potential TypeError.
If AppLogger methods are typed as string $message (common), passing a Stringable directly can trigger a TypeError. Cast consistently across all branches (INFO already does).
- $this->logger->error($message, $context);
+ $this->logger->error((string) $message, $context);
return;
case PsrLogLevel::WARNING:
- $this->logger->warning($message, $context);
+ $this->logger->warning((string) $message, $context);
return;
case PsrLogLevel::NOTICE:
case PsrLogLevel::INFO:
$this->logger->info((string) $message, $context);
return;
case PsrLogLevel::DEBUG:
- $this->logger->debug($message, $context);
+ $this->logger->debug((string) $message, $context);
return;To confirm AppLogger’s parameter types, please verify the method signatures:
🌐 Web query:
What are the PHP method signatures (parameter types) of error(), warning(), info(), and debug() on the RoadRunner Logger class (RoadRunner\Logger\Logger or Spiral\RoadRunner\Logger)? Do they accept only string $message or also \Stringable?
💡 Result:
Short answer: the RoadRunner PHP logger methods take string, not \Stringable.
Details / signatures (as published in the package docs):
- debug(string): void
- error(string): void
- info(string): void
- warning(string): void
(there is also log(string): void for stderr.)
The RoadRunner app-logger docs and the package README show these methods typed/documented as string, and the RPC API used by the plugin uses plain string parameters. (docs.roadrunner.dev, github.com, packagist.org)
If you want, I can fetch the exact Logger.php source file for the release you care about and paste the function signatures (to show the exact type hints used in code). Which version/branch should I check?
Citations:
- 1: https://docs.roadrunner.dev/docs/logging-and-observability/applogger?utm_source=chatgpt.com
- 2: https://github.com/roadrunner-php/app-logger
- 3: https://packagist.org/packages/roadrunner-php/app-logger?utm_source=chatgpt.com
Ensure message is cast to string for all AppLogger calls
RoadRunner’s AppLogger methods (debug(), error(), info(), warning()) are strictly typed to accept string only (per docs.roadrunner.dev, app-logger README). Passing a \Stringable directly will trigger a TypeError. You should consistently cast $message to string in every branch:
• File: src/RpcLogger.php (around lines 32–55)
• Update the calls to error(), warning(), and debug() as follows:
--- a/src/RpcLogger.php
+++ b/src/RpcLogger.php
@@ switch ($normalizedLevel) {
case PsrLogLevel::EMERGENCY:
case PsrLogLevel::ALERT:
case PsrLogLevel::CRITICAL:
case PsrLogLevel::ERROR:
- $this->logger->error($message, $context);
+ $this->logger->error((string) $message, $context);
return;
case PsrLogLevel::WARNING:
- $this->logger->warning($message, $context);
+ $this->logger->warning((string) $message, $context);
return;
case PsrLogLevel::NOTICE:
case PsrLogLevel::INFO:
$this->logger->info((string) $message, $context);
return;
case PsrLogLevel::DEBUG:
- $this->logger->debug($message, $context);
+ $this->logger->debug((string) $message, $context);
return;
}📝 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.
| $normalizedLevel = \is_string($level) ? \strtolower($level) : (string) $level; | |
| /** @var array<string, mixed> $context */ | |
| switch ($normalizedLevel) { | |
| case PsrLogLevel::EMERGENCY: | |
| case PsrLogLevel::ALERT: | |
| case PsrLogLevel::CRITICAL: | |
| case PsrLogLevel::ERROR: | |
| $this->logger->error($message, $context); | |
| return; | |
| case PsrLogLevel::WARNING: | |
| $this->logger->warning($message, $context); | |
| return; | |
| case PsrLogLevel::NOTICE: | |
| case PsrLogLevel::INFO: | |
| $this->logger->info((string) $message, $context); | |
| return; | |
| case PsrLogLevel::DEBUG: | |
| $this->logger->debug($message, $context); | |
| return; | |
| $normalizedLevel = \is_string($level) ? \strtolower($level) : (string) $level; | |
| /** @var array<string, mixed> $context */ | |
| switch ($normalizedLevel) { | |
| case PsrLogLevel::EMERGENCY: | |
| case PsrLogLevel::ALERT: | |
| case PsrLogLevel::CRITICAL: | |
| case PsrLogLevel::ERROR: | |
| $this->logger->error((string) $message, $context); | |
| return; | |
| case PsrLogLevel::WARNING: | |
| $this->logger->warning((string) $message, $context); | |
| return; | |
| case PsrLogLevel::NOTICE: | |
| case PsrLogLevel::INFO: | |
| $this->logger->info((string) $message, $context); | |
| return; | |
| case PsrLogLevel::DEBUG: | |
| $this->logger->debug((string) $message, $context); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/RpcLogger.php around lines 32 to 55, AppLogger methods require a string
and currently some branches pass $message without casting; update the calls to
$this->logger->error, $this->logger->warning and $this->logger->debug to cast
the message to string (e.g. (string) $message) so every logger call receives a
string (info already does this).
|
|
||
| /** | ||
| * @param mixed $level | ||
| * @param array<array-key, mixed> $context |
There was a problem hiding this comment.
Looks like we have to convert this $context for roadrunner logger format.
For example an Exception, DateTime, or DateInterval object can be here. We need to format it in a good way.
There was a problem hiding this comment.
@roxblnfk Do you have any suggestions for me?
There was a problem hiding this comment.
First, write tests.
With tests we can merge the PR and then add other features like exceptions formatting
There was a problem hiding this comment.
@roxblnfk I have added unit test for this PR, plz confirm them. tysm
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
tests/Unit/RpcLoggerTest.php (5)
31-36: Constructor test is redundant but harmlessSince instantiation is already exercised in setUp() and all subsequent tests, this dedicated constructor test adds minimal extra value. Fine to keep for explicitness; okay to drop to reduce test noise.
147-177: LGTM: Invalid level scenarios; consider a couple more edge casesThe assertions for int, null, and bool(true) are sharp and aligned with normalized string casting. Optional: add tests for bool(false) → 'Invalid log level: ' (empty string) and numeric string '0' to fully cover corner cases.
212-307: Reduce repetition with small assertion helper or a data provider for trait methodsEight trait-method tests repeat the same pattern. Consider a helper to DRY:
private function assertLastCall(string $expectedMethod): void { $this->assertSame(1, $this->rpc->getCallCount()); $lastCall = $this->rpc->getLastCall(); $this->assertSame($expectedMethod, $lastCall['method']); }Then each test can shrink to the act + assertLastCall('...'). Alternatively, a data provider mapping method names to expected RPC methods can collapse these into a single parametrized test.
361-369: Silence static analysis warnings for unused parameters in TestRPCPHPMD flags unused parameters in withCodec/withServicePrefix. Prefix them with an underscore to signal intentional non-use.
- public function withCodec(CodecInterface $codec): RPCInterface + public function withCodec(CodecInterface $_codec): RPCInterface { return $this; } - public function withServicePrefix(string $service): RPCInterface + public function withServicePrefix(string $_service): RPCInterface { return $this; }
371-375: Avoid mutating internal array pointer in getLastCall()Using end() mutates the internal pointer. Favor array_key_last for purity.
- public function getLastCall(): ?array - { - return end($this->calls) ?: null; - } + public function getLastCall(): ?array + { + if ($this->calls === []) { + return null; + } + return $this->calls[array_key_last($this->calls)]; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
tests/Unit/RpcLoggerTest.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/Unit/RpcLoggerTest.php (1)
src/RpcLogger.php (2)
RpcLogger(13-60)log(30-59)
🪛 PHPMD (2.15.0)
tests/Unit/RpcLoggerTest.php
361-361: Avoid unused parameters such as '$codec'. (Unused Code Rules)
(UnusedFormalParameter)
366-366: Avoid unused parameters such as '$service'. (Unused Code Rules)
(UnusedFormalParameter)
🔇 Additional comments (10)
tests/Unit/RpcLoggerTest.php (10)
24-29: LGTM: Clean dependency wiring in setUp()Good composition: TestRPC → AppLogger → RpcLogger. This isolates the unit under test and gives precise observability of RPC interactions.
38-50: LGTM: Correct mapping for emergency/alert/critical/error to ErrorWithContextAssertions on method name and DTO payload type are on point.
52-63: LGTM: Warning → WarningWithContext and notice/info → InfoWithContextSolid coverage and correct assertions for context-bearing calls.
Also applies to: 65-77
79-90: LGTM: Debug → DebugWithContextCorrect mapping and payload type verification.
109-119: LGTM: Correctly asserting no-context path (Info + raw payload)Both tests validate the “no context → non-with-context RPC method” behavior. This aligns well with AppLogger’s split between WithContext and non-context methods.
Also applies to: 199-210
121-145: LGTM: Case-insensitive level handlingGood coverage for uppercase and mixed-case inputs; matches the normalization strategy.
179-197: LGTM: Message passthrough cases (empty string and numeric string)Good checks for raw-message passthrough when no context is supplied.
309-327: LGTM: Complex context sanity checkGood to see nested arrays and objects accepted without exploding the serializer path. Method routing remains correct.
329-346: LGTM: Data providers are clear and explicitNamed datasets improve readability of failures.
376-385: LGTM: Utility methods for the spy are straightforwardgetCallCount() and reset() are simple and effective for test assertions.
| public function testLogWithStringableMessage(): void | ||
| { | ||
| $stringableMessage = new class implements \Stringable { | ||
| public function __toString(): string | ||
| { | ||
| return 'Stringable message'; | ||
| } | ||
| }; | ||
|
|
||
| $this->rpcLogger->log(PsrLogLevel::INFO, $stringableMessage); | ||
|
|
||
| $this->assertSame(1, $this->rpc->getCallCount()); | ||
| $lastCall = $this->rpc->getLastCall(); | ||
| $this->assertSame('Info', $lastCall['method']); | ||
| $this->assertSame('Stringable message', $lastCall['payload']); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider asserting stringable handling across all levels; align impl to cast consistently
Great check for stringable messages (INFO with no context). To harden behavior and prevent future regressions:
- Add analogous tests for stringable messages at ERROR/WARNING/DEBUG (with/without context).
- Ensure RpcLogger casts to string before delegating at all levels (DEBUG currently passes $message without a cast; INFO/NOTICE cast).
Follow-up: If you want to lock this in at the implementation level, consider this change in RpcLogger::log:
public function log($level, \Stringable|string $message, array $context = []): void
{
$normalizedLevel = \is_string($level) ? \strtolower($level) : (string) $level;
$msg = (string) $message;
switch ($normalizedLevel) {
case Psr\Log\LogLevel::EMERGENCY:
case Psr\Log\LogLevel::ALERT:
case Psr\Log\LogLevel::CRITICAL:
case Psr\Log\LogLevel::ERROR:
$this->logger->error($msg, $context);
return;
case Psr\Log\LogLevel::WARNING:
$this->logger->warning($msg, $context);
return;
case Psr\Log\LogLevel::NOTICE:
case Psr\Log\LogLevel::INFO:
$this->logger->info($msg, $context);
return;
case Psr\Log\LogLevel::DEBUG:
$this->logger->debug($msg, $context);
return;
default:
throw new Psr\Log\InvalidArgumentException('Invalid log level: ' . $normalizedLevel);
}
}Run this quick check to confirm current casting patterns in RpcLogger:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate RpcLogger and show how info/debug are invoked
fd -i --full-path 'RpcLogger.php' -t f | while read -r file; do
echo ">>> $file"
rg -n -C2 '\$this->logger->info\(' "$file"
rg -n -C2 '\$this->logger->debug\(' "$file"
doneLength of output: 568
Cast stringable messages for DEBUG too; add tests for other levels
Verified: src/RpcLogger.php casts (string) $message for INFO/NOTICE (around line 49) but passes $message uncast to DEBUG (around line 53). This is inconsistent and can break Stringable handling.
Actionable items:
- Fix implementation
- File: src/RpcLogger.php — replace:
with:
case PsrLogLevel::DEBUG: $this->logger->debug($message, $context); return;case PsrLogLevel::DEBUG: $this->logger->debug((string) $message, $context); return;
- File: src/RpcLogger.php — replace:
- Add tests
- File: tests/Unit/RpcLoggerTest.php — add analogous stringable-message tests for DEBUG, WARNING and ERROR (with/without context), mirroring the existing INFO test.
- Optional improvement
- Normalize/cast once at the start of RpcLogger::log (accept \Stringable|string $message and do $msg = (string)$message) and use $msg for all logger calls to avoid repetition.
🤖 Prompt for AI Agents
In tests/Unit/RpcLoggerTest.php around lines 92-107, add missing tests and fix
the implementation: update src/RpcLogger.php so the DEBUG branch casts
stringable messages like the others (replace the DEBUG call to pass (string)
$message or better, cast once at the start of RpcLogger::log into $msg and use
$msg for all levels), then add new unit tests in tests/Unit/RpcLoggerTest.php
mirroring the existing INFO test for stringable messages at DEBUG, WARNING and
ERROR (include variants that pass context where applicable) to assert the RPC
call count, method name and that the payload equals the stringified message.
2 participants
Sorry for my late.
This PR implement log method to satisfy PSR-3.
Summary by CodeRabbit
New Features
Bug Fixes
Tests