Skip to content

Implement log method #4

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 19, 2025
Merged

Implement log method #4

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8fa9dbf
feat: implement log method in RpcLogger with level handling
hungthai1401 Aug 11, 2025
2e7bebc
test: expand RpcLoggerTest with comprehensive logging tests
hungthai1401 Aug 15, 2025
28a1bb3
refactor: replace TestRPC with RpcSpy for improved RPC call testing
hungthai1401 Aug 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 43 additions & 5 deletions src/RpcLogger.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,55 @@

use Psr\Log\LoggerInterface;
use Psr\Log\LoggerTrait;
use Psr\Log\LogLevel as PsrLogLevel;
use Psr\Log\InvalidArgumentException as PsrInvalidArgumentException;
use RoadRunner\Logger\Logger as AppLogger;

class RpcLogger implements LoggerInterface
{
use LoggerTrait;

private readonly AppLogger $logger;

public function __construct(AppLogger $logger)
{
$this->logger = $logger;
}

/**
* @param mixed $level
* @param array<array-key, mixed> $context
Copy link
Member

@roxblnfk roxblnfk Aug 13, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@hungthai1401 hungthai1401 Aug 14, 2025

Choose a reason for hiding this comment

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

@roxblnfk Do you have any suggestions for me?

Copy link
Member

@roxblnfk roxblnfk Aug 14, 2025

Choose a reason for hiding this comment

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

First, write tests.
With tests we can merge the PR and then add other features like exceptions formatting

Copy link
Contributor Author

@hungthai1401 hungthai1401 Aug 15, 2025

Choose a reason for hiding this comment

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

@roxblnfk I have added unit test for this PR, plz confirm them. tysm

*
* @link https://www.php-fig.org/psr/psr-3/#5-psrlogloglevel
*/
public function log($level, \Stringable|string $message, array $context = []): void
{
/**
* TODO: to be implemented
*
* @link https://github.com/roadrunner-php/psr-logger/issues/3
*/
$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;

Comment on lines +32 to +55
Copy link

@coderabbitai coderabbitai bot Aug 11, 2025

Choose a reason for hiding this comment

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

💡 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:

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.

Suggested change
$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).

default:
throw new PsrInvalidArgumentException('Invalid log level: ' . $normalizedLevel);
}
}
}
Loading
Loading