-
Notifications
You must be signed in to change notification settings - Fork 1
Enhanced context format #8
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.
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
Changes from all commits
a2aff82
93024f0
3ca8bb6
00ac89b
af98022
4cbf8ac
0027fd2
0406f7f
50ba224
33f5a80
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace RoadRunner\PsrLogger\Context; | ||
|
|
||
| use RoadRunner\PsrLogger\Context\ObjectProcessor\DateTimeProcessor; | ||
| use RoadRunner\PsrLogger\Context\ObjectProcessor\FallbackProcessor; | ||
| use RoadRunner\PsrLogger\Context\ObjectProcessor\StringableProcessor; | ||
| use RoadRunner\PsrLogger\Context\ObjectProcessor\ThrowableProcessor; | ||
|
|
||
| /** | ||
| * Default context processor. | ||
| * | ||
| * @api | ||
| */ | ||
| final class DefaultProcessor | ||
| { | ||
| /** @var list<ObjectProcessor> */ | ||
| private array $processors = []; | ||
|
|
||
| private function __construct() {} | ||
|
|
||
| /** | ||
| * Create empty instance. | ||
| */ | ||
| public static function create(): self | ||
| { | ||
| return new self(); | ||
| } | ||
|
|
||
| /** | ||
| * Create with default set of {@see ObjectProcessor}. | ||
| */ | ||
| public static function createDefault(): self | ||
| { | ||
| $self = new self(); | ||
| $self->processors = [ | ||
| new DateTimeProcessor(), | ||
| new StringableProcessor(), | ||
| new ThrowableProcessor(), | ||
| new FallbackProcessor(), | ||
| ]; | ||
| return $self; | ||
| } | ||
|
|
||
| /** | ||
| * Copy the current object and add Object Processors before existing ones. | ||
| */ | ||
| public function withObjectProcessors(ObjectProcessor ...$processors): self | ||
| { | ||
| $clone = clone $this; | ||
| $clone->processors = \array_merge(\array_values($processors), $clone->processors); | ||
| return $clone; | ||
| } | ||
|
|
||
| public function __invoke(mixed $value): mixed | ||
| { | ||
| if (\is_resource($value)) { | ||
| return \get_resource_type($value) . ' resource'; | ||
| } | ||
|
|
||
| if (\is_array($value)) { | ||
| foreach ($value as &$v) { | ||
| $v = $this($v); | ||
| } | ||
| } | ||
|
|
||
| if (\is_object($value)) { | ||
| foreach ($this->processors as $processor) { | ||
| if ($processor->canProcess($value)) { | ||
| return $processor->process($value, $this); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return $value; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace RoadRunner\PsrLogger\Context; | ||
|
|
||
| /** | ||
| * Converts an object into a scalar or an array for serializable logger context. | ||
| * | ||
| * @template T | ||
| * @api | ||
| */ | ||
| interface ObjectProcessor | ||
| { | ||
| /** | ||
| * Check if this processor can handle the given value. | ||
| */ | ||
| public function canProcess(object $value): bool; | ||
|
|
||
| /** | ||
| * @param T $value | ||
| * @param callable(mixed): mixed $processor Function to process nested object values | ||
| */ | ||
| public function process(object $value, callable $processor): mixed; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace RoadRunner\PsrLogger\Context\ObjectProcessor; | ||
|
|
||
| use RoadRunner\PsrLogger\Context\ObjectProcessor; | ||
|
|
||
| /** | ||
| * Processor for DateTime objects. | ||
| * | ||
| * Converts DateTime and DateTimeImmutable objects to ISO 8601 format | ||
| * for consistent structured logging. | ||
| * | ||
| * @implements ObjectProcessor<\DateTimeInterface> | ||
| * @api | ||
| */ | ||
| final class DateTimeProcessor implements ObjectProcessor | ||
| { | ||
| #[\Override] | ||
| public function canProcess(object $value): bool | ||
| { | ||
| return $value instanceof \DateTimeInterface; | ||
| } | ||
|
|
||
| #[\Override] | ||
| public function process(object $value, callable $processor): mixed | ||
| { | ||
| return $value->format(\DateTimeInterface::ATOM); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,37 @@ | ||||||||||||||||||||||||||||||||||||||||
| <?php | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| declare(strict_types=1); | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| namespace RoadRunner\PsrLogger\Context\ObjectProcessor; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| use RoadRunner\PsrLogger\Context\ObjectProcessor; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||
| * Fallback processor for unknown objects. | ||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||
| * @implements ObjectProcessor<object> | ||||||||||||||||||||||||||||||||||||||||
| * @api | ||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||
| final class FallbackProcessor implements ObjectProcessor | ||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||
| #[\Override] | ||||||||||||||||||||||||||||||||||||||||
| public function canProcess(object $value): bool | ||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||
| return true; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| #[\Override] | ||||||||||||||||||||||||||||||||||||||||
| public function process(object $value, callable $processor): array | ||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||
| $result = ['@class' => $value::class] + \get_object_vars($value); | ||||||||||||||||||||||||||||||||||||||||
| foreach ($result as $k => &$v) { | ||||||||||||||||||||||||||||||||||||||||
| if ($v === $value) { | ||||||||||||||||||||||||||||||||||||||||
| unset($result[$k]); | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| $v = $processor($v); | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+26
to
+33
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Skip processing self-references after unsetting them. Line 27 tries to drop self-referential properties, but because the loop lacks a Apply this diff: foreach ($result as $k => &$v) {
if ($v === $value) {
unset($result[$k]);
+ continue;
}
$v = $processor($v);
}📝 Committable suggestion
Suggested change
🧰 Tools🪛 GitHub Actions: static analysis[error] 26-26: MixedAssignment: Unable to determine the type that $v is being assigned to [error] 31-31: MixedAssignment: Unable to determine the type that $v is being assigned to 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| return $result; | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+27
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major Prevent re-processing after unsetting self-references; avoid by-ref pitfalls. After Apply: - foreach ($result as $k => &$v) {
- if ($v === $value) {
- unset($result[$k]);
- }
-
- $v = $processor($v);
- }
+ foreach (\array_keys($result) as $k) {
+ $v = $result[$k];
+ if ($v === $value) {
+ unset($result[$k]);
+ continue;
+ }
+ $result[$k] = $processor($v);
+ }This removes the reference, skips removed entries, and satisfies static analyzers. 📝 Committable suggestion
Suggested change
🧰 Tools🪛 GitHub Actions: static analysis[error] 26-26: MixedAssignment: Unable to determine the type that $v is being assigned to [error] 31-31: MixedAssignment: Unable to determine the type that $v is being assigned to 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace RoadRunner\PsrLogger\Context\ObjectProcessor; | ||
|
|
||
| use RoadRunner\PsrLogger\Context\ObjectProcessor; | ||
|
|
||
| /** | ||
| * Converts Stringable objects to their string representation. | ||
| * | ||
| * @implements ObjectProcessor<\Stringable> | ||
| * @api | ||
| */ | ||
| final class StringableProcessor implements ObjectProcessor | ||
| { | ||
| #[\Override] | ||
| public function canProcess(object $value): bool | ||
| { | ||
| return $value instanceof \Stringable; | ||
| } | ||
|
|
||
| #[\Override] | ||
| public function process(object $value, callable $processor): mixed | ||
| { | ||
| return (string) $value; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| namespace RoadRunner\PsrLogger\Context\ObjectProcessor; | ||
|
|
||
| use RoadRunner\PsrLogger\Context\ObjectProcessor; | ||
|
|
||
| /** | ||
| * Converts exceptions to structured data containing class, message, | ||
| * code, file, line, and stack trace information. | ||
| * | ||
| * @implements ObjectProcessor<\Throwable> | ||
| * @api | ||
| */ | ||
| final class ThrowableProcessor implements ObjectProcessor | ||
| { | ||
| #[\Override] | ||
| public function canProcess(object $value): bool | ||
| { | ||
| return $value instanceof \Throwable; | ||
| } | ||
|
|
||
| #[\Override] | ||
| public function process(object $value, callable $processor): array | ||
| { | ||
| return [ | ||
| 'class' => \get_class($value), | ||
| 'message' => $value->getMessage(), | ||
| 'code' => $value->getCode(), | ||
| 'file' => $value->getFile(), | ||
| 'line' => $value->getLine(), | ||
| 'trace' => $value->getTraceAsString(), | ||
| ]; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -10,16 +10,22 @@ | |||||||||||||||||||||||
| use Psr\Log\InvalidArgumentException as PsrInvalidArgumentException; | ||||||||||||||||||||||||
| use RoadRunner\Logger\Logger as AppLogger; | ||||||||||||||||||||||||
| use RoadRunner\Logger\LogLevel; | ||||||||||||||||||||||||
| use RoadRunner\PsrLogger\Context\DefaultProcessor; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||
| * @api | ||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||
| class RpcLogger implements LoggerInterface | ||||||||||||||||||||||||
| { | ||||||||||||||||||||||||
| use LoggerTrait; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| private readonly AppLogger $logger; | ||||||||||||||||||||||||
| private readonly \Closure $objectProcessor; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| public function __construct(AppLogger $logger) | ||||||||||||||||||||||||
| public function __construct(AppLogger $logger, ?callable $processor = null) | ||||||||||||||||||||||||
| { | ||||||||||||||||||||||||
| $this->logger = $logger; | ||||||||||||||||||||||||
| $this->objectProcessor = ($processor ?? DefaultProcessor::createDefault())(...); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||
|
|
@@ -29,26 +35,29 @@ public function __construct(AppLogger $logger) | |||||||||||||||||||||||
| * | ||||||||||||||||||||||||
| * @link https://www.php-fig.org/psr/psr-3/#5-psrlogloglevel | ||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||
| #[\Override] | ||||||||||||||||||||||||
| public function log($level, \Stringable|string $message, array $context = []): void | ||||||||||||||||||||||||
| { | ||||||||||||||||||||||||
| $normalizedLevel = \strtolower(match (true) { | ||||||||||||||||||||||||
| \is_string($level), | ||||||||||||||||||||||||
| \is_string($level) => $level, | ||||||||||||||||||||||||
| $level instanceof \Stringable => (string) $level, | ||||||||||||||||||||||||
| $level instanceof \BackedEnum => (string) $level->value, | ||||||||||||||||||||||||
| $level instanceof LogLevel => $level->name, | ||||||||||||||||||||||||
| default => throw new PsrInvalidArgumentException('Invalid log level type provided.'), | ||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /** @var array<string, mixed> $context */ | ||||||||||||||||||||||||
| // Process context data for structured logging using the processor manager | ||||||||||||||||||||||||
| $processedContext = ($this->objectProcessor)($context); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
|
Comment on lines
+49
to
+51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Normalize context keys to strings and cast message once. Fix Psalm MixedAssignment/MixedArgument and ensure AppLogger gets array<string,mixed>. - // Process context data for structured logging using the processor manager
- $processedContext = ($this->objectProcessor)($context);
+ // Process context data for structured logging
+ /** @var array<array-key, mixed> $tmpContext */
+ $tmpContext = ($this->objectProcessor)($context);
+ /** @var array<string, mixed> $processedContext */
+ $processedContext = [];
+ foreach ($tmpContext as $k => $v) {
+ $processedContext[(string) $k] = $v;
+ }
+ $message = (string) $message;📝 Committable suggestion
Suggested change
🧰 Tools🪛 GitHub Actions: static analysis[error] 46-46: Psalm: MixedAssignment: Unable to determine the type that $processedContext is being assigned to 🤖 Prompt for AI Agents |
||||||||||||||||||||||||
| match ($normalizedLevel) { | ||||||||||||||||||||||||
| PsrLogLevel::EMERGENCY, | ||||||||||||||||||||||||
| PsrLogLevel::ALERT, | ||||||||||||||||||||||||
| PsrLogLevel::CRITICAL, | ||||||||||||||||||||||||
| PsrLogLevel::ERROR => $this->logger->error($message, $context), | ||||||||||||||||||||||||
| PsrLogLevel::WARNING => $this->logger->warning($message, $context), | ||||||||||||||||||||||||
| PsrLogLevel::NOTICE, PsrLogLevel::INFO => $this->logger->info((string) $message, $context), | ||||||||||||||||||||||||
| 'log' => $this->logger->log((string) $message, $context), | ||||||||||||||||||||||||
| PsrLogLevel::DEBUG => $this->logger->debug($message, $context), | ||||||||||||||||||||||||
| PsrLogLevel::ERROR => $this->logger->error($message, $processedContext), | ||||||||||||||||||||||||
| PsrLogLevel::WARNING => $this->logger->warning($message, $processedContext), | ||||||||||||||||||||||||
| PsrLogLevel::NOTICE, PsrLogLevel::INFO => $this->logger->info((string) $message, $processedContext), | ||||||||||||||||||||||||
| 'log' => $this->logger->log((string) $message, $processedContext), | ||||||||||||||||||||||||
| PsrLogLevel::DEBUG => $this->logger->debug($message, $processedContext), | ||||||||||||||||||||||||
|
Comment on lines
43
to
+60
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainVerify: is custom level 'log' intentionally supported as an input level? PSR-3 levels don’t include 'log'. Keeping this branch is fine if RoadRunner’s Logger explicitly accepts a 'log' level alias; otherwise consider removing it to avoid accepting invalid levels. 🌐 Web query: 💡 Result: RoadRunner\Logger\Logger does not accept a 'log' level as a distinct log level; 'log' is only the generic method name defined by PSR-3 for submitting logs at any severity. Only the standard PSR-3 levels (emergency, alert, critical, error, warning, notice, info, debug) are accepted; custom or unknown levels, such as a literal 'log' level, are not supported[6][4][10]. Supporting details:
Summary of best practice: Additional context:
Citations:
Remove unsupported ‘log’ level and enforce PSR-3 compliance • Remove the default => throw new \Psr\Log\InvalidArgumentException(
sprintf('Unsupported log level "%s". Valid levels: emergency, alert, critical, error, warning, notice, info, debug.', $normalizedLevel)
),• Update or add tests to assert that providing an invalid level (including “log”) triggers the exception. This change ensures that only valid PSR-3 levels are accepted and that any unknown level is explicitly rejected, in line with PSR-3 guidelines. 🤖 Prompt for AI Agents |
||||||||||||||||||||||||
| default => throw new PsrInvalidArgumentException("Invalid log level `$normalizedLevel` provided."), | ||||||||||||||||||||||||
|
Comment on lines
+56
to
61
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential TypeError: cast Stringable messages to string for all logger calls AppLogger methods likely accept string messages. Passing a \Stringable without casting can throw a TypeError with strict types. Some branches already cast; others don’t. Apply this diff to consistently cast: - PsrLogLevel::ERROR => $this->logger->error($message, $processedContext),
- PsrLogLevel::WARNING => $this->logger->warning($message, $processedContext),
- PsrLogLevel::NOTICE, PsrLogLevel::INFO => $this->logger->info((string) $message, $processedContext),
- 'log' => $this->logger->log((string) $message, $processedContext),
- PsrLogLevel::DEBUG => $this->logger->debug($message, $processedContext),
+ PsrLogLevel::ERROR => $this->logger->error((string) $message, $processedContext),
+ PsrLogLevel::WARNING => $this->logger->warning((string) $message, $processedContext),
+ PsrLogLevel::NOTICE, PsrLogLevel::INFO => $this->logger->info((string) $message, $processedContext),
+ 'log' => $this->logger->log((string) $message, $processedContext),
+ PsrLogLevel::DEBUG => $this->logger->debug((string) $message, $processedContext),Optional follow-up: cast once after processing context to reduce repetition. // Process context data for structured logging using the processor manager
$processedContext = $this->contextProcessor->processContext($context);
+ $message = (string) $message;Then remove per-branch casts. 🤖 Prompt for AI Agents |
||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Add explicit return after array processing.
Array processing modifies
$valueby reference but doesn't explicitly return. Execution falls through to line 51 (object check fails) and eventually returns on line 59. While this works, it's confusing and wastes cycles checking if an array is an object.Apply this diff:
if (\is_array($value)) { foreach ($value as &$v) { $v = $this($v); } + return $value; }📝 Committable suggestion
🧰 Tools
🪛 GitHub Actions: static analysis
[error] 46-46: Psalm: MixedAssignment: Unable to determine the type that $v is being assigned to
[error] 47-47: Psalm: MixedAssignment: Unable to determine the type that $v is being assigned to
🤖 Prompt for AI Agents