Enhanced context format #8
Conversation
- Added ContextProcessorManager to manage multiple context processors. - Introduced various processors: ArrayProcessor, DateTimeProcessor, FallbackProcessor, NullProcessor, ObjectProcessor, ResourceProcessor, ScalarProcessor, StringableProcessor, and ThrowableProcessor. - Enhanced RpcLogger to utilize ContextProcessorManager for processing log context data.
- Implemented unit tests for various context processors including ArrayProcessor, DateTimeProcessor, FallbackProcessor, NullProcessor, ObjectProcessor, ResourceProcessor, ScalarProcessor, StringableProcessor, and ThrowableProcessor. - Ensured coverage for processing different data types and edge cases, validating the functionality of each processor. - Enhanced the RpcLoggerTest with additional tests for logging with complex contexts and custom processors.
WalkthroughIntroduces a context processing pipeline for log contexts and integrates it into RpcLogger. Adds ContextProcessorManager and multiple processors (built-in types, DateTime, Throwable, Stringable, Resource, Object, Fallback). Updates RpcLogger to use processed context and adjusts constructor. Expands unit tests for processors and RpcLogger. Enables coverage in composer test script. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App as Caller
participant RL as RpcLogger
participant CPM as ContextProcessorManager
participant AL as AppLogger
App->>RL: log(level, message, context)
RL->>CPM: processContext(context)
CPM-->>RL: processedContext
RL->>AL: log(level, message, processedContext)
AL-->>RL: ack
RL-->>App: return
sequenceDiagram
autonumber
participant CPM as ContextProcessorManager
participant P1 as BuiltInTypeProcessor
participant P2 as DateTimeProcessor
participant P3 as ThrowableProcessor
participant P4 as StringableProcessor
participant P5 as ResourceProcessor
participant P6 as ObjectProcessor
participant P7 as FallbackProcessor
note over CPM: processValue(value)
CPM->>P1: canProcess?
alt P1 handles
P1->>P1: process (recurse for arrays)
P1-->>CPM: result
else not handled
CPM->>P2: canProcess? ➜ process?
CPM->>P3: canProcess? ➜ process?
CPM->>P4: canProcess? ➜ process?
CPM->>P5: canProcess? ➜ process?
CPM->>P6: canProcess? ➜ process?
CPM->>P7: canProcess (always)
P7-->>CPM: gettype(value)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 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. ✨ 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: 7
🧹 Nitpick comments (26)
src/ContextProcessor/ThrowableProcessor.php (2)
27-37: Leverage the recursiveProcessor to serialize chained exceptions (and silence PHPMD).Using the callback to process the previous throwable makes output richer and resolves the unused-parameter warning.
Apply:
- public function process(mixed $value, callable $recursiveProcessor): mixed - { - return [ + public function process(mixed $value, callable $recursiveProcessor): mixed + { + $previous = $value->getPrevious(); + return [ 'class' => \get_class($value), 'message' => $value->getMessage(), 'code' => $value->getCode(), 'file' => $value->getFile(), 'line' => $value->getLine(), - 'trace' => $value->getTraceAsString(), + 'trace' => $value->getTraceAsString(), + 'previous' => $previous !== null ? $recursiveProcessor($previous) : null, ]; }If you prefer not to change the payload shape right now, minimally suppress PHPMD on this method:
/** * @param \Throwable $value * @param callable(mixed): mixed $recursiveProcessor * @return array<string, mixed> + * @SuppressWarnings(PHPMD.UnusedFormalParameter) */
35-35: Consider emitting structured stack frames (optional).Arrays from getTrace() are more machine-friendly than a single string. If backward compatibility allows, expose frames (e.g., as traceFrames) alongside trace.
src/ContextProcessor/DateTimeProcessor.php (2)
22-30: Silence PHPMD for the intentionally unused recursiveProcessor.This processor doesn’t recurse; suppress the warning locally.
/** * @param \DateTimeInterface $value * @param callable(mixed): mixed $recursiveProcessor * @return string + * @SuppressWarnings(PHPMD.UnusedFormalParameter) */ public function process(mixed $value, callable $recursiveProcessor): mixed { return $value->format(\DateTimeInterface::ATOM); }
29-29: Optional: Use RFC3339_EXTENDED to retain sub-second precision.If higher precision timestamps are useful for log correlation, prefer RFC3339_EXTENDED.
- return $value->format(\DateTimeInterface::ATOM); + return $value->format(\DateTimeInterface::RFC3339_EXTENDED);src/ContextProcessor/ResourceProcessor.php (2)
21-29: Suppress PHPMD for the unused recursiveProcessor (no recursion needed here)./** * @param resource $value * @param callable(mixed): mixed $recursiveProcessor * @return string + * @SuppressWarnings(PHPMD.UnusedFormalParameter) */ public function process(mixed $value, callable $recursiveProcessor): mixed { return \get_resource_type($value) . ' resource'; }
26-29: Confirming PHP >= 8.1: get_resource_id() is availablecomposer.json specifies PHP >= 8.1, so get_resource_id (added in PHP 7.2) can be used safely. Including the resource ID can improve debugability.
Locations to update:
- src/ContextProcessor/ResourceProcessor.php (process method)
Proposed diff:
--- a/src/ContextProcessor/ResourceProcessor.php +++ b/src/ContextProcessor/ResourceProcessor.php @@ public function process(mixed $value, callable $recursiveProcessor): mixed - return \get_resource_type($value) . ' resource'; + $type = \get_resource_type($value); + $id = \get_resource_id($value); + return \sprintf('%s resource (%d)', $type, $id);src/ContextProcessor/NullProcessor.php (1)
27-31: Return the given value and suppress the unused callback warning.Returning $value expresses pass-through and eliminates one unused-parameter warning; suppress the other.
/** * @param null $value * @param callable(mixed): mixed $recursiveProcessor * @return null + * @SuppressWarnings(PHPMD.UnusedFormalParameter) */ public function process(mixed $value, callable $recursiveProcessor): mixed { - // Null values are already suitable for logging - return null; + // Null values are already suitable for logging + return $value; }src/ContextProcessor/FallbackProcessor.php (2)
17-21: Suppress PHPMD for intentionally unused parameter in canProcess
$valueis intentionally unused because this processor is the last-resort handler. Suppress the warning to keep static analysis clean.Apply this diff:
- public function canProcess(mixed $value): bool + /** + * @SuppressWarnings("UnusedFormalParameter") + */ + public function canProcess(mixed $value): bool { // This processor can handle anything as a last resort return true; }
23-30: Prefer get_debug_type and suppress PHPMD for unused$recursiveProcessor
- Suppress the unused-parameter warning for
$recursiveProcessor.- Consider
\get_debug_type($value)for more informative type names (e.g., class names for objects, “float” instead of “double”). Not functionally required given upstream processors, but nicer as a final fallback.Apply this diff:
- /** - * @param callable(mixed): mixed $recursiveProcessor - * @return string - */ + /** + * @param callable(mixed): mixed $recursiveProcessor + * @return string + * @SuppressWarnings("UnusedFormalParameter") + */ public function process(mixed $value, callable $recursiveProcessor): mixed { - return \gettype($value); + return \get_debug_type($value); }src/ContextProcessor/ScalarProcessor.php (1)
23-33: Suppress PHPMD for intentionally unused$recursiveProcessor
processreturns the scalar as-is and doesn’t need the recursive callback. Suppress the warning to avoid noise from static analysis.Apply this diff:
/** * @param scalar $value * @param callable(mixed): mixed $recursiveProcessor * @return scalar */ - public function process(mixed $value, callable $recursiveProcessor): mixed + /** + * @SuppressWarnings("UnusedFormalParameter") + */ + public function process(mixed $value, callable $recursiveProcessor): mixed { // Scalar values are already suitable for logging return $value; }src/ContextProcessor/StringableProcessor.php (1)
21-29: Suppress PHPMD for intentionally unused$recursiveProcessorThe recursive callback isn’t needed here since we directly cast to string. Suppress the warning.
Apply this diff:
/** * @param \Stringable $value * @param callable(mixed): mixed $recursiveProcessor * @return string */ - public function process(mixed $value, callable $recursiveProcessor): mixed + /** + * @SuppressWarnings("UnusedFormalParameter") + */ + public function process(mixed $value, callable $recursiveProcessor): mixed { return (string) $value; }tests/Unit/ContextProcessor/ArrayProcessorTest.php (2)
17-28: Avoid leaving open resources in data providersUsing
fopen('php://memory', 'r')creates a resource that isn’t closed. Prefertmpfile(), which is cleaned up automatically by PHP.Apply this diff:
return [ 'string' => ['not an array'], 'integer' => [42], 'float' => [3.14], 'boolean' => [true], 'null' => [null], 'object' => [new \stdClass()], - 'resource' => [\fopen('php://memory', 'r')], + 'resource' => [\tmpfile()], ];
75-99: Strengthen the nested-array test and remove the unused$expectedvariable warningCurrently,
$expectedis unused and the assertion only checks for a key. Make the recursive processor transform arrays so we can assert the exact expected result.Apply this diff:
// Mock recursive processor that adds a prefix to strings - $recursiveProcessor = static function ($value) { - return \is_string($value) ? 'processed:' . $value : $value; - }; + $recursiveProcessor = static function ($value) { + if (\is_array($value)) { + return 'processed:' . \serialize($value); + } + return \is_string($value) ? 'processed:' . $value : $value; + }; $result = $this->processor->process($array, $recursiveProcessor); $expected = [ 'level1' => 'processed:' . \serialize(['level2' => ['value' => 'deep']]), ]; - // Since the recursive processor is called on the nested array, - // we need to test that it was called correctly - $this->assertArrayHasKey('level1', $result); + // The nested array should be transformed by the recursive processor + $this->assertSame($expected, $result);tests/Unit/ContextProcessor/ObjectProcessorTest.php (1)
17-28: Avoid leaking resources in data providersThe
nonObjectProvideropens a stream resource that is never closed. Data providers are executed before the test and you don’t get a chance tofclosethere, which can lead to handle leaks across the test run.Apply this minimal change inside the provider and move the resource case to a dedicated test where you can open/close explicitly:
return [ 'string' => ['not an object'], 'integer' => [42], 'float' => [3.14], 'boolean' => [true], 'null' => [null], 'array' => [[]], - 'resource' => [\fopen('php://memory', 'r')], + // resource case covered in a dedicated test to avoid leaking open handles ];Then add this method elsewhere in the class to cover the resource case:
public function testCannotProcessResource(): void { $h = \fopen('php://memory', 'r'); try { $this->assertFalse($this->processor->canProcess($h)); } finally { \fclose($h); } }tests/Unit/ContextProcessor/NullProcessorTest.php (1)
17-29: Prevent resource leaks in providerAs with the other test, the resource created in the data provider is never closed.
Remove the resource from the provider:
return [ 'string' => ['test'], 'integer' => [42], 'float' => [3.14], 'boolean true' => [true], 'boolean false' => [false], 'array' => [[]], 'object' => [new \stdClass()], - 'resource' => [\fopen('php://memory', 'r')], + // resource case is covered by a dedicated test to ensure proper closing ];Then add a dedicated test method where you can open and close:
public function testCannotProcessResource(): void { $h = \fopen('php://memory', 'r'); try { $this->assertFalse($this->processor->canProcess($h)); } finally { \fclose($h); } }tests/Unit/ContextProcessor/StringableProcessorTest.php (1)
17-29: Close the opened resource from the data providerThe provider opens a memory stream (Line 27) that is never closed in the test, leaking a resource. Close it in the test via a finally block.
#[DataProvider('nonStringableProvider')] public function testCannotProcessNonStringable(mixed $value): void { - $this->assertFalse($this->processor->canProcess($value)); + try { + $this->assertFalse($this->processor->canProcess($value)); + } finally { + if (\is_resource($value)) { + \fclose($value); + } + } }Also applies to: 43-47
tests/Unit/ContextProcessor/ScalarProcessorTest.php (2)
17-30: Close resource from data provider in canProcess testThe data provider opens a resource (Line 28) that isn’t closed, causing a leak when used in this test. Close it in a finally block.
#[DataProvider('scalarValuesProvider')] public function testCanProcessScalarValues(mixed $value, bool $expected): void { - $this->assertSame($expected, $this->processor->canProcess($value)); + try { + $this->assertSame($expected, $this->processor->canProcess($value)); + } finally { + if (\is_resource($value)) { + \fclose($value); + } + } }Also applies to: 32-36
17-30: Also close resource in process testSame leak exists when the dataset is used in testProcessScalarValues. Close in a finally block.
#[DataProvider('scalarValuesProvider')] public function testProcessScalarValues(mixed $value, bool $canProcess): void { - if ($canProcess) { - $recursiveProcessor = static fn($v) => $v; - $result = $this->processor->process($value, $recursiveProcessor); - $this->assertSame($value, $result); - } else { - $this->addToAssertionCount(1); // Skip non-scalar values - } + try { + if ($canProcess) { + $recursiveProcessor = static fn($v) => $v; + $result = $this->processor->process($value, $recursiveProcessor); + $this->assertSame($value, $result); + } else { + $this->addToAssertionCount(1); // Skip non-scalar values + } + } finally { + if (\is_resource($value)) { + \fclose($value); + } + } }Also applies to: 38-48
tests/Unit/ContextProcessor/ResourceProcessorTest.php (1)
69-88: Rename and simplify: not actually testing cURL resourceOn PHP 8+, curl_init returns CurlHandle (object), and this test ultimately processes a stream resource, not cURL. The extension gate and curl handle are unnecessary. Rename the test and drop cURL code.
- public function testProcessCurlResource(): void - { - if (!\extension_loaded('curl')) { - $this->markTestSkipped('cURL extension not available'); - } - - $curlHandle = \curl_init(); - - // In PHP 8+, curl_init returns CurlHandle object, not resource - // Let's test with a different resource type that's consistently a resource - $resource = \fopen('data://text/plain;base64,SGVsbG8gV29ybGQ=', 'r'); - $recursiveProcessor = static fn($v) => $v; - - $result = $this->processor->process($resource, $recursiveProcessor); - - \fclose($resource); - \curl_close($curlHandle); - - $this->assertSame('stream resource', $result); - } + public function testProcessDataStreamResource(): void + { + $resource = \fopen('data://text/plain;base64,SGVsbG8gV29ybGQ=', 'r'); + $recursiveProcessor = static fn($v) => $v; + $result = $this->processor->process($resource, $recursiveProcessor); + \fclose($resource); + $this->assertSame('stream resource', $result); + }src/ContextProcessor/ContextProcessorManager.php (1)
65-75: Optional: add recursion depth guard to prevent runaway processingSelf-referential arrays/objects can cause infinite recursion. Adding a max-depth cap is a pragmatic safety net.
- public function processValue(mixed $value): mixed + private int $maxDepth = 10; + + public function processValue(mixed $value, int $depth = 0): mixed { + if ($depth >= $this->maxDepth) { + return '*max-depth*'; + } foreach ($this->processors as $processor) { if ($processor->canProcess($value)) { - return $processor->process($value, [$this, 'processValue']); + $recursive = function (mixed $v) use ($depth) { + return $this->processValue($v, $depth + 1); + }; + return $processor->process($value, $recursive); } } // This should never happen due to FallbackProcessor, but just in case return \gettype($value); }Note: No changes needed where processValue is called—existing calls will start at depth 0.
tests/Unit/ContextProcessor/FallbackProcessorTest.php (2)
17-41: Prefer deterministic cleanup over register_shutdown_function for test-created resources.Using register_shutdown_function to close the data-provider resource works, but it’s global and can hide leaks. Consider closing the handle in a class-level tearDownAfterClass or creating/closing the resource within individual tests that need it to keep lifecycle explicit.
Example approach using tearDownAfterClass:
private static $resource; public static function allTypesProvider(): array { self::$resource = \fopen('php://memory', 'r'); return [ // ... 'resource' => [self::$resource, 'resource'], ]; } public static function tearDownAfterClass(): void { if (\is_resource(self::$resource)) { \fclose(self::$resource); } }
59-70: Assert that the closed resource is actually closed to avoid false confidence.Minor robustness: after fclose($resource), add an assertion that is_resource($resource) is false (where applicable) before proceeding, to make the test intent explicit. It won’t harm environments where PHP still reports it as a resource, since you’re only asserting the result type is a string.
- \fclose($resource); + \fclose($resource); + // Depending on PHP version, this may be false or still report as resource. + $this->assertIsString($this->processor->process($resource, static fn($v) => $v));tests/Unit/RpcLoggerTest.php (4)
546-559: Silence PHPMD warnings for unused $recursiveProcessor in test stubs.These stubs don’t use the recursive processor; make that explicit to satisfy static analysis.
Apply this diff:
public function process(mixed $value, callable $recursiveProcessor): mixed { + unset($recursiveProcessor); // unused in this test stub // Mask email for privacy $parts = \explode('@', $value); return \substr($parts[0], 0, 2) . '***@' . $parts[1]; }
586-601: Silence PHPMD warnings for unused $recursiveProcessor in URL processor stub.public function process(mixed $value, callable $recursiveProcessor): mixed { + unset($recursiveProcessor); // unused in this test stub $parsed = \parse_url($value); return [ 'scheme' => $parsed['scheme'] ?? null, 'host' => $parsed['host'] ?? null, 'path' => $parsed['path'] ?? null, ]; }
604-614: Silence PHPMD warnings for unused $recursiveProcessor in CC processor stub.public function process(mixed $value, callable $recursiveProcessor): mixed { + unset($recursiveProcessor); // unused in this test stub return '****-****-****-' . \substr($value, -4); }
485-503: Ensure resource creation succeeds to avoid undefined behavior on fopen failure.fopen() may return false; guard the test to fail fast or skip if the resource can’t be created.
- $resource = \fopen('php://memory', 'r'); + $resource = \fopen('php://memory', 'r'); + $this->assertIsResource($resource, 'Failed to create test resource');
📜 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 (23)
src/ContextProcessor/ArrayProcessor.php(1 hunks)src/ContextProcessor/ContextProcessorInterface.php(1 hunks)src/ContextProcessor/ContextProcessorManager.php(1 hunks)src/ContextProcessor/DateTimeProcessor.php(1 hunks)src/ContextProcessor/FallbackProcessor.php(1 hunks)src/ContextProcessor/NullProcessor.php(1 hunks)src/ContextProcessor/ObjectProcessor.php(1 hunks)src/ContextProcessor/ResourceProcessor.php(1 hunks)src/ContextProcessor/ScalarProcessor.php(1 hunks)src/ContextProcessor/StringableProcessor.php(1 hunks)src/ContextProcessor/ThrowableProcessor.php(1 hunks)src/RpcLogger.php(2 hunks)tests/Unit/ContextProcessor/ArrayProcessorTest.php(1 hunks)tests/Unit/ContextProcessor/ContextProcessorManagerTest.php(1 hunks)tests/Unit/ContextProcessor/DateTimeProcessorTest.php(1 hunks)tests/Unit/ContextProcessor/FallbackProcessorTest.php(1 hunks)tests/Unit/ContextProcessor/NullProcessorTest.php(1 hunks)tests/Unit/ContextProcessor/ObjectProcessorTest.php(1 hunks)tests/Unit/ContextProcessor/ResourceProcessorTest.php(1 hunks)tests/Unit/ContextProcessor/ScalarProcessorTest.php(1 hunks)tests/Unit/ContextProcessor/StringableProcessorTest.php(1 hunks)tests/Unit/ContextProcessor/ThrowableProcessorTest.php(1 hunks)tests/Unit/RpcLoggerTest.php(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (23)
src/ContextProcessor/FallbackProcessor.php (1)
src/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(21-21)process(30-30)
src/ContextProcessor/ResourceProcessor.php (5)
src/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(21-21)process(30-30)src/ContextProcessor/ArrayProcessor.php (2)
canProcess(16-19)process(26-41)src/ContextProcessor/FallbackProcessor.php (2)
canProcess(17-21)process(27-30)src/ContextProcessor/ObjectProcessor.php (2)
canProcess(17-20)process(27-48)src/ContextProcessor/ScalarProcessor.php (2)
canProcess(18-21)process(28-32)
tests/Unit/RpcLoggerTest.php (4)
src/ContextProcessor/ContextProcessorManager.php (2)
ContextProcessorManager(13-104)addProcessor(27-30)src/RpcLogger.php (2)
log(35-59)RpcLogger(15-60)tests/Unit/RpcSpy.php (4)
getCallCount(42-45)getLastCall(37-40)RpcSpy(17-51)reset(47-50)tests/Unit/ContextProcessor/ContextProcessorManagerTest.php (6)
__toString(69-72)__toString(151-154)__toString(170-173)canProcess(125-128)process(130-133)testProcessorOrdering(147-163)
src/ContextProcessor/ObjectProcessor.php (6)
src/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(21-21)process(30-30)src/ContextProcessor/ArrayProcessor.php (2)
canProcess(16-19)process(26-41)src/ContextProcessor/FallbackProcessor.php (2)
canProcess(17-21)process(27-30)src/ContextProcessor/ThrowableProcessor.php (2)
canProcess(17-20)process(27-37)src/ContextProcessor/StringableProcessor.php (2)
canProcess(16-19)process(26-29)src/ContextProcessor/DateTimeProcessor.php (2)
canProcess(17-20)process(27-30)
src/ContextProcessor/ScalarProcessor.php (2)
src/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(21-21)process(30-30)src/ContextProcessor/NullProcessor.php (2)
canProcess(17-20)process(27-31)
tests/Unit/ContextProcessor/ArrayProcessorTest.php (1)
src/ContextProcessor/ArrayProcessor.php (1)
ArrayProcessor(14-42)
src/ContextProcessor/DateTimeProcessor.php (1)
src/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(21-21)process(30-30)
src/ContextProcessor/ArrayProcessor.php (2)
src/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(21-21)process(30-30)src/ContextProcessor/ObjectProcessor.php (2)
canProcess(17-20)process(27-48)
tests/Unit/ContextProcessor/ObjectProcessorTest.php (1)
src/ContextProcessor/ObjectProcessor.php (1)
ObjectProcessor(15-49)
tests/Unit/ContextProcessor/ThrowableProcessorTest.php (1)
src/ContextProcessor/ThrowableProcessor.php (1)
ThrowableProcessor(15-38)
src/ContextProcessor/StringableProcessor.php (2)
src/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(21-21)process(30-30)src/ContextProcessor/ObjectProcessor.php (2)
canProcess(17-20)process(27-48)
tests/Unit/ContextProcessor/ContextProcessorManagerTest.php (1)
src/ContextProcessor/ContextProcessorManager.php (3)
ContextProcessorManager(13-104)processContext(40-60)addProcessor(27-30)
src/ContextProcessor/ContextProcessorInterface.php (9)
src/ContextProcessor/ArrayProcessor.php (2)
canProcess(16-19)process(26-41)src/ContextProcessor/FallbackProcessor.php (2)
canProcess(17-21)process(27-30)src/ContextProcessor/NullProcessor.php (2)
canProcess(17-20)process(27-31)src/ContextProcessor/ResourceProcessor.php (2)
canProcess(16-19)process(26-29)src/ContextProcessor/ObjectProcessor.php (2)
canProcess(17-20)process(27-48)src/ContextProcessor/ScalarProcessor.php (2)
canProcess(18-21)process(28-32)src/ContextProcessor/ThrowableProcessor.php (2)
canProcess(17-20)process(27-37)src/ContextProcessor/StringableProcessor.php (2)
canProcess(16-19)process(26-29)src/ContextProcessor/DateTimeProcessor.php (2)
canProcess(17-20)process(27-30)
src/ContextProcessor/ContextProcessorManager.php (11)
src/RpcLogger.php (1)
__construct(22-26)src/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(21-21)process(30-30)src/ContextProcessor/ArrayProcessor.php (3)
canProcess(16-19)process(26-41)ArrayProcessor(14-42)src/ContextProcessor/FallbackProcessor.php (3)
canProcess(17-21)process(27-30)FallbackProcessor(15-31)src/ContextProcessor/NullProcessor.php (3)
canProcess(17-20)process(27-31)NullProcessor(15-32)src/ContextProcessor/ResourceProcessor.php (3)
canProcess(16-19)process(26-29)ResourceProcessor(14-30)src/ContextProcessor/ObjectProcessor.php (3)
canProcess(17-20)process(27-48)ObjectProcessor(15-49)src/ContextProcessor/ScalarProcessor.php (3)
canProcess(18-21)process(28-32)ScalarProcessor(16-33)src/ContextProcessor/ThrowableProcessor.php (3)
canProcess(17-20)process(27-37)ThrowableProcessor(15-38)src/ContextProcessor/StringableProcessor.php (3)
canProcess(16-19)process(26-29)StringableProcessor(14-30)src/ContextProcessor/DateTimeProcessor.php (3)
canProcess(17-20)process(27-30)DateTimeProcessor(15-31)
tests/Unit/ContextProcessor/DateTimeProcessorTest.php (2)
tests/Unit/ContextProcessor/ContextProcessorManagerTest.php (3)
canProcess(125-128)testProcessDateTime(40-48)process(130-133)src/ContextProcessor/DateTimeProcessor.php (1)
DateTimeProcessor(15-31)
tests/Unit/ContextProcessor/ScalarProcessorTest.php (3)
tests/Unit/ContextProcessor/ContextProcessorManagerTest.php (5)
CoversClass(12-208)canProcess(125-128)testProcessScalarValues(25-38)process(130-133)setUp(204-207)tests/Unit/ContextProcessor/NullProcessorTest.php (3)
CoversClass(12-54)DataProvider(36-40)setUp(50-53)src/ContextProcessor/ScalarProcessor.php (1)
ScalarProcessor(16-33)
tests/Unit/ContextProcessor/StringableProcessorTest.php (1)
src/ContextProcessor/StringableProcessor.php (1)
StringableProcessor(14-30)
tests/Unit/ContextProcessor/NullProcessorTest.php (2)
tests/Unit/ContextProcessor/ScalarProcessorTest.php (4)
CoversClass(12-54)DataProvider(32-36)DataProvider(38-48)setUp(50-53)src/ContextProcessor/NullProcessor.php (1)
NullProcessor(15-32)
tests/Unit/ContextProcessor/FallbackProcessorTest.php (1)
src/ContextProcessor/FallbackProcessor.php (1)
FallbackProcessor(15-31)
tests/Unit/ContextProcessor/ResourceProcessorTest.php (1)
src/ContextProcessor/ResourceProcessor.php (1)
ResourceProcessor(14-30)
src/ContextProcessor/ThrowableProcessor.php (2)
src/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(21-21)process(30-30)src/ContextProcessor/ObjectProcessor.php (2)
canProcess(17-20)process(27-48)
src/RpcLogger.php (1)
src/ContextProcessor/ContextProcessorManager.php (3)
ContextProcessorManager(13-104)__construct(18-21)processContext(40-60)
src/ContextProcessor/NullProcessor.php (3)
src/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(21-21)process(30-30)src/ContextProcessor/ArrayProcessor.php (2)
canProcess(16-19)process(26-41)src/ContextProcessor/ScalarProcessor.php (2)
canProcess(18-21)process(28-32)
🪛 PHPMD (2.15.0)
src/ContextProcessor/FallbackProcessor.php
17-17: Avoid unused parameters such as '$value'. (Unused Code Rules)
(UnusedFormalParameter)
27-27: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
src/ContextProcessor/ResourceProcessor.php
26-26: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
tests/Unit/RpcLoggerTest.php
468-468: Avoid unused private fields such as '$privateProp'. (Unused Code Rules)
(UnusedPrivateField)
553-553: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
610-610: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
677-677: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
src/ContextProcessor/ScalarProcessor.php
28-28: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
tests/Unit/ContextProcessor/ArrayProcessorTest.php
92-92: Avoid unused local variables such as '$expected'. (Unused Code Rules)
(UnusedLocalVariable)
src/ContextProcessor/DateTimeProcessor.php
27-27: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
tests/Unit/ContextProcessor/ObjectProcessorTest.php
48-48: Avoid unused private fields such as '$secret'. (Unused Code Rules)
(UnusedPrivateField)
64-64: Avoid unused private fields such as '$private'. (Unused Code Rules)
(UnusedPrivateField)
src/ContextProcessor/StringableProcessor.php
26-26: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
tests/Unit/ContextProcessor/ContextProcessorManagerTest.php
113-113: Avoid unused private fields such as '$privateProp'. (Unused Code Rules)
(UnusedPrivateField)
130-130: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
src/ContextProcessor/ThrowableProcessor.php
27-27: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
src/ContextProcessor/NullProcessor.php
27-27: Avoid unused parameters such as '$value'. (Unused Code Rules)
(UnusedFormalParameter)
27-27: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
🔇 Additional comments (24)
src/ContextProcessor/ThrowableProcessor.php (1)
17-20: Ordering Verified: ThrowableProcessor precedes ObjectProcessor
- In src/ContextProcessor/ContextProcessorManager.php,
addProcessor(new ThrowableProcessor())is called on line 91, and
addProcessor(new ObjectProcessor())on line 99.- This ensures throwables are handled before generic objects.
No changes needed.
src/ContextProcessor/ContextProcessorInterface.php (1)
16-31: Interface design looks solid.Clear contract, generic-friendly PHPDoc, and callable signature are appropriate for recursive processing.
src/RpcLogger.php (1)
45-47: Sanity check: processing empty contexts is cheap and safeGood use of ContextProcessorManager; it returns an empty array for empty input, so this call is safe on the hot path.
tests/Unit/ContextProcessor/ObjectProcessorTest.php (1)
42-61: Thorough coverage and correct expectations for public-vs-nonpublic properties and recursionTests accurately validate:
- Only public properties are serialized.
- Private/protected are excluded.
- stdClass handling (both non-empty and empty).
- Nested array/object values are delegated to the provided recursive processor.
- Mixed falsy values are preserved.
Also applies to: 76-161
tests/Unit/ContextProcessor/ContextProcessorManagerTest.php (1)
17-203: LGTM: solid end-to-end coverage of the default pipeline and processor orderingThe suite exercises null/scalars, DateTime, Throwable, Stringable, arrays, resources, objects, ordering, and a complex mixed context. Assertions align with the implementations and the default registration order in ContextProcessorManager.
tests/Unit/ContextProcessor/NullProcessorTest.php (1)
31-48: LGTM: behavior and boundaries are correct
- canProcess(null) returns true; non-nulls are rejected.
- process returns null and does not touch the recursive processor.
tests/Unit/ContextProcessor/DateTimeProcessorTest.php (1)
16-54: LGTM: correct coverage for DateTimeInterface handlingTests verify both DateTime and DateTimeImmutable, negative cases, and stable formatting with concrete offsets. Expectations match the production implementation.
src/ContextProcessor/ObjectProcessor.php (1)
17-21: Appropriate type checkHandling all objects here is correct; specialized processors (Throwable, DateTime, Stringable) precede this one in the manager’s registration order.
tests/Unit/ContextProcessor/StringableProcessorTest.php (4)
12-16: Good coverage and setupCoversClass is correctly targeted and the setup is clear.
49-61: LGTM: Conversion behavior aligns with implementationThe test validates that process returns (string)$value and matches StringableProcessor’s contract.
64-79: LGTM: Complex stringable logic coveredCovers non-trivial __toString logic and expected uppercase result.
81-94: LGTM: Empty string case coveredEdge case for empty string is appropriately asserted.
tests/Unit/ContextProcessor/ScalarProcessorTest.php (1)
12-16: Solid structure and coverage targetCoversClass annotation and setup look good.
tests/Unit/ContextProcessor/ResourceProcessorTest.php (4)
30-35: LGTM: canProcess for resourceOpens and closes the resource correctly; assertion is appropriate.
43-53: LGTM: Stream resource processingAsserts the formatted string precisely and handles cleanup.
55-67: LGTM: File resource processingUses temp file safely, cleans up, and validates expected output.
90-105: LGTM: Closed resource handlingGood defensive branching across PHP versions—only processes when still recognized as resource.
src/ContextProcessor/ArrayProcessor.php (1)
14-41: Array processing is correct and preserves keysImplementation cleanly delegates recursion and maintains keys. Psalm annotations are appropriate.
src/ContextProcessor/ContextProcessorManager.php (2)
53-57: Potential key collision when casting keys to stringCasting context keys to string may overwrite entries if both 1 and '1' exist, causing silent data loss. Consider detecting collisions or documenting this behavior.
Would you like a small guard that appends suffixes on collision (e.g., "1#2") or a debug log to flag it?
81-103: Default processor order is sensiblePriority given to Null, Scalar, specific object types, then collections/resources, then generic object and fallback. This ordering will produce predictable results.
tests/Unit/ContextProcessor/FallbackProcessorTest.php (1)
12-16: Good coverage and intent are clear.Tests validate both canProcess() and process() paths and include edge cases (e.g., closed resource). Nicely scoped.
tests/Unit/ContextProcessor/ThrowableProcessorTest.php (1)
52-69: Solid assertions for Throwable normalization.Ensures class, message, code, and basic location/trace information are present with correct types. Looks good.
tests/Unit/RpcLoggerTest.php (2)
1-58: Core RpcLogger behavior tests look solid.Level normalization, method routing, and empty-context behavior are consistently validated. Using RpcSpy to assert single-call behavior and payload shape is pragmatic.
Also applies to: 64-102, 121-157, 168-208, 209-240, 242-337, 339-357
159-166: Enum import is unnecessary
TheLogLevelEnumis declared inRoadRunner\PsrLogger\Tests\Unit, the same namespace asRpcLoggerTest, so referencingLogLevelEnum::Warningwithout an import is valid.
| public function process(mixed $value, callable $recursiveProcessor): mixed | ||
| { | ||
| // Try to convert to array (for objects with public properties) | ||
| $objectVars = \get_object_vars($value); | ||
|
|
||
| if (!empty($objectVars)) { | ||
| /** @var array<string, mixed> $processed */ | ||
| $processed = []; | ||
| /** | ||
| * @var string $property | ||
| * @var mixed $propertyValue | ||
| */ | ||
| foreach ($objectVars as $property => $propertyValue) { | ||
| /** @psalm-suppress MixedAssignment - Intentionally processing mixed types */ | ||
| $processed[$property] = $recursiveProcessor($propertyValue); | ||
| } | ||
| return $processed; | ||
| } | ||
|
|
||
| // Fallback to class name if no public properties | ||
| return \get_class($value); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Robust and predictable object-to-array conversion with recursive delegation
- get_object_vars usage correctly restricts to public properties.
- Delegation to the provided $recursiveProcessor enables deep processing without duplicating logic.
- Fallback to class name for objects without public properties is reasonable and matches tests.
One caveat: with the current manager recursion strategy, cyclic graphs (object↔object, array↔array, or mixed) will cause infinite recursion. The guard belongs in ContextProcessorManager, but calling out here for visibility.
Below is a minimal, backward-compatible approach you can apply to ContextProcessorManager to prevent infinite recursion and bound traversal depth:
// src/ContextProcessor/ContextProcessorManager.php (illustrative patch)
final class ContextProcessorManager
{
/** @var array<ContextProcessorInterface> */
private array $processors = [];
/** @var \SplObjectStorage<object, null> */
private \SplObjectStorage $seenObjects;
private int $maxDepth = 20;
public function __construct()
{
$this->seenObjects = new \SplObjectStorage();
$this->registerDefaultProcessors();
}
/**
* @template TKey of array-key
* @template TValue
* @param array<TKey, TValue> $context
* @return array<string, mixed>
*/
public function processContext(array $context): array
{
if ($context === []) {
return [];
}
$processed = [];
foreach ($context as $key => $value) {
$processed[(string) $key] = $this->processValueDepth($value, 0);
}
return $processed;
}
// Preserve existing public API
public function processValue(mixed $value): mixed
{
return $this->processValueDepth($value, 0);
}
private function processValueDepth(mixed $value, int $depth): mixed
{
if ($depth > $this->maxDepth) {
return \is_object($value)
? \get_class($value) . ' (max-depth)'
: (\is_array($value) ? 'array (max-depth)' : \gettype($value));
}
$maybeDetach = false;
if (\is_object($value)) {
if ($this->seenObjects->contains($value)) {
return \get_class($value) . ' (recursive)';
}
$this->seenObjects->attach($value);
$maybeDetach = true;
}
try {
foreach ($this->processors as $processor) {
if ($processor->canProcess($value)) {
// Important: recurse with depth+1
return $processor->process($value, fn ($v) => $this->processValueDepth($v, $depth + 1));
}
}
return \gettype($value); // defensive fallback
} finally {
if ($maybeDetach) {
$this->seenObjects->detach($value);
}
}
}
}Optionally, add regression tests to assert graceful handling of:
- An array that references itself by reference.
- Two objects referencing each other.
I can contribute those tests if you want.
🤖 Prompt for AI Agents
In src/ContextProcessor/ObjectProcessor.php around lines 27 to 48 the processor
delegates recursion to the manager but does not guard against cyclic structures,
which allows infinite recursion; fix this in ContextProcessorManager by
implementing a depth-limited recursive entry (e.g. private
processValueDepth(mixed $value, int $depth)) that returns a max-depth marker
after a configurable maxDepth, track visited objects with a \SplObjectStorage to
detect and short-circuit cycles (returning a "recursive" marker for seen
objects), ensure you attach/detach objects around processing, and change public
processValue/processContext to call processValueDepth and pass a closure that
increments depth (fn($v) => $this->processValueDepth($v, $depth + 1)) to all
processors.
| 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), | ||
| default => throw new PsrInvalidArgumentException("Invalid log level `$normalizedLevel` provided."), |
There was a problem hiding this comment.
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
In src/RpcLogger.php around lines 52 to 57, some logger calls pass a \Stringable
$message without casting which can cause a TypeError under strict types; ensure
the message is always a string by casting before use — either replace each
logger call to use (string) $message consistently in every branch or better:
after processing the context, assign $msg = (string) $message and use $msg in
all switch branches (then remove any per-branch casts).
| public function testProcessNestedArray(): void | ||
| { | ||
| $context = [ | ||
| 'level1' => [ | ||
| 'level2' => [ | ||
| 'value' => 'deep', | ||
| 'number' => 123, | ||
| ], | ||
| ], | ||
| ]; | ||
|
|
||
| $result = $this->manager->processContext($context); | ||
|
|
||
| $this->assertSame($context, $result); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding a regression test for cyclic structures
Nested arrays are covered, but cyclic references (arrays referencing themselves or objects referencing each other) will currently cause unbounded recursion in the manager’s recursion callback, leading to a fatal error/stack overflow.
If you’d like, I can open a follow-up to implement a recursion guard (seen-object tracking + max depth) in ContextProcessorManager and contribute a couple of tests that assert graceful handling (e.g., “ClassName (recursive)” / “array (max-depth)”) rather than recursing infinitely.
🤖 Prompt for AI Agents
In tests/Unit/ContextProcessor/ContextProcessorManagerTest.php around lines
81-95, add regression tests for cyclic data structures and ensure
ContextProcessorManager handles them without infinite recursion: create tests
that build a self-referencing array and mutually-referencing objects, call
processContext, and assert the result contains sentinel strings (e.g., "array
(recursive)" or "ClassName (recursive)") or "array (max-depth)" when depth limit
is reached; then implement a recursion guard in ContextProcessorManager by
tracking seen values (use object identity via spl_object_hash/spl_object_id for
objects and track array spl hashes or references) and short-circuiting when a
value is revisited, and enforce a configurable max depth fallback that returns a
max-depth marker instead of recursing further.
| public function testProcessCustomException(): void | ||
| { | ||
| $customException = new class('Custom message', 999) extends \Exception {}; | ||
| $recursiveProcessor = static fn($v) => $v; | ||
|
|
||
| $result = $this->processor->process($customException, $recursiveProcessor); | ||
|
|
||
| $this->assertIsArray($result); | ||
| $this->assertTrue(\str_contains($result['class'], 'Exception@anonymous')); | ||
| $this->assertSame('Custom message', $result['message']); | ||
| $this->assertSame(999, $result['code']); | ||
| } |
There was a problem hiding this comment.
Fix assertion for anonymous exception class naming.
get_class() for an anonymous class extending Exception returns something like "class@anonymous..."; it does not contain "Exception@anonymous". This assertion will be brittle or fail across PHP versions.
Apply this change:
- $this->assertTrue(\str_contains($result['class'], 'Exception@anonymous'));
+ $this->assertStringContainsString('@anonymous', $result['class']);🤖 Prompt for AI Agents
In tests/Unit/ContextProcessor/ThrowableProcessorTest.php around lines 84 to 95,
the assertion expecting 'Exception@anonymous' is brittle because get_class() for
an anonymous class returns a name like "class@anonymous..." (not necessarily
containing the parent class name). Update the assertion to check for the generic
'@anonymous' marker (e.g., assert that $result['class'] contains '@anonymous' or
use a regex to match /@anonymous/), leaving other assertions unchanged.
| public function testCustomProcessorIntegration(): void | ||
| { | ||
| // Create a custom processor for email addresses | ||
| $emailProcessor = new class implements ContextProcessorInterface { | ||
| public function canProcess(mixed $value): bool | ||
| { | ||
| return \is_string($value) && \filter_var($value, FILTER_VALIDATE_EMAIL) !== false; | ||
| } | ||
|
|
||
| public function process(mixed $value, callable $recursiveProcessor): mixed | ||
| { | ||
| // Mask email for privacy | ||
| $parts = \explode('@', $value); | ||
| return \substr($parts[0], 0, 2) . '***@' . $parts[1]; | ||
| } | ||
| }; | ||
|
|
||
| // Create processor manager with custom processor added first | ||
| $processorManager = new ContextProcessorManager(); | ||
| $processorManager->addProcessor($emailProcessor); | ||
|
|
||
| // Create logger with custom processor manager | ||
| $logger = new RpcLogger($this->appLogger, $processorManager); | ||
|
|
||
| $context = [ | ||
| 'user_email' => '[email protected]', | ||
| 'admin_email' => '[email protected]', | ||
| 'regular_string' => 'not an email', | ||
| 'user_id' => 123, | ||
| ]; | ||
|
|
||
| $logger->info('User action performed', $context); | ||
|
|
||
| $this->assertSame(1, $this->rpc->getCallCount()); | ||
| $lastCall = $this->rpc->getLastCall(); | ||
| $this->assertSame('InfoWithContext', $lastCall['method']); | ||
| $this->assertInstanceOf(\RoadRunner\AppLogger\DTO\V1\LogEntry::class, $lastCall['payload']); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Custom processor is appended after defaults; won’t run before ScalarProcessor for strings.
The comment says “added first,” but ContextProcessorManager registers defaults in its constructor and addProcessor() appends. For string values, the default ScalarProcessor will always short-circuit before your email processor.
Recommended:
- Add a “prepend” API to ContextProcessorManager so custom processors can take precedence:
- public function addProcessorFirst(ContextProcessorInterface $p): void { array_unshift($this->processors, $p); }
- Or allow injecting the processor list via constructor and controlling whether defaults are registered.
Once available, update this test to actually verify masking occurred (e.g., by inspecting the processed context, or by stubbing AppLogger to capture the processed context pre-serialization).
🧰 Tools
🪛 PHPMD (2.15.0)
553-553: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
| public function testMultipleCustomProcessors(): void | ||
| { | ||
| // Custom processor for URLs | ||
| $urlProcessor = new class implements ContextProcessorInterface { | ||
| public function canProcess(mixed $value): bool | ||
| { | ||
| return \is_string($value) && \filter_var($value, FILTER_VALIDATE_URL) !== false; | ||
| } | ||
|
|
||
| public function process(mixed $value, callable $recursiveProcessor): mixed | ||
| { | ||
| $parsed = \parse_url($value); | ||
| return [ | ||
| 'scheme' => $parsed['scheme'] ?? null, | ||
| 'host' => $parsed['host'] ?? null, | ||
| 'path' => $parsed['path'] ?? null, | ||
| ]; | ||
| } | ||
| }; | ||
|
|
||
| // Custom processor for credit card numbers (mock) | ||
| $ccProcessor = new class implements ContextProcessorInterface { | ||
| public function canProcess(mixed $value): bool | ||
| { | ||
| return \is_string($value) && \preg_match('/^\d{4}-?\d{4}-?\d{4}-?\d{4}$/', $value); | ||
| } | ||
|
|
||
| public function process(mixed $value, callable $recursiveProcessor): mixed | ||
| { | ||
| return '****-****-****-' . \substr($value, -4); | ||
| } | ||
| }; | ||
|
|
||
| $processorManager = new ContextProcessorManager(); | ||
| $processorManager->addProcessor($urlProcessor); | ||
| $processorManager->addProcessor($ccProcessor); | ||
|
|
||
| $logger = new RpcLogger($this->appLogger, $processorManager); | ||
|
|
||
| $context = [ | ||
| 'website' => 'https://example.com/path/to/resource', | ||
| 'payment_card' => '1234-5678-9012-3456', | ||
| 'regular_data' => 'normal string', | ||
| 'amount' => 99.99, | ||
| ]; | ||
|
|
||
| $logger->warning('Payment processed', $context); | ||
|
|
||
| $this->assertSame(1, $this->rpc->getCallCount()); | ||
| $lastCall = $this->rpc->getLastCall(); | ||
| $this->assertSame('WarningWithContext', $lastCall['method']); | ||
| $this->assertInstanceOf(\RoadRunner\AppLogger\DTO\V1\LogEntry::class, $lastCall['payload']); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same precedence issue for multiple custom processors; also not asserting they were applied.
Both custom processors are appended after defaults, so strings will be handled by ScalarProcessor first. The test only checks that a call happened, not that processors actually transformed the context.
Proposed steps:
- Introduce addProcessorFirst() and use it here for $urlProcessor and $ccProcessor so they precede ScalarProcessor.
- Strengthen the test by asserting the processed shapes (e.g., that website becomes an array with scheme/host/path and payment_card is masked). If inspecting LogEntry is not feasible, consider a thin test double of AppLogger to capture the context arguments.
🧰 Tools
🪛 PHPMD (2.15.0)
610-610: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
| public function testProcessorOrdering(): void | ||
| { | ||
| // Create processors for the same type to test ordering | ||
| $firstProcessor = new class implements ContextProcessorInterface { | ||
| public function canProcess(mixed $value): bool | ||
| { | ||
| return \is_int($value); | ||
| } | ||
|
|
||
| public function process(mixed $value, callable $recursiveProcessor): mixed | ||
| { | ||
| return 'first:' . $value; | ||
| } | ||
| }; | ||
|
|
||
| $secondProcessor = new class implements ContextProcessorInterface { | ||
| public function canProcess(mixed $value): bool | ||
| { | ||
| return \is_int($value); | ||
| } | ||
|
|
||
| public function process(mixed $value, callable $recursiveProcessor): mixed | ||
| { | ||
| return 'second:' . $value; | ||
| } | ||
| }; | ||
|
|
||
| $processorManager = new ContextProcessorManager(); | ||
| $processorManager->addProcessor($firstProcessor); // Added first, should be used | ||
| $processorManager->addProcessor($secondProcessor); // Added second, should be skipped | ||
|
|
||
| $logger = new RpcLogger($this->appLogger, $processorManager); | ||
|
|
||
| $context = ['number' => 42]; | ||
| $logger->debug('Ordering test', $context); | ||
|
|
||
| $this->assertSame(1, $this->rpc->getCallCount()); | ||
| $lastCall = $this->rpc->getLastCall(); | ||
| $this->assertSame('DebugWithContext', $lastCall['method']); | ||
|
|
||
| // The first processor should have been used | ||
| // We can't directly inspect the processed context, but we know it was processed | ||
| $this->assertInstanceOf(\RoadRunner\AppLogger\DTO\V1\LogEntry::class, $lastCall['payload']); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ordering test does not validate ordering due to ScalarProcessor preemption.
Integers are already handled by the default ScalarProcessor, so your two custom int processors won’t be reached. The test cannot validate ordering as written, and the final note acknowledges you can’t inspect processed context.
Options:
- Use addProcessorFirst() to prepend your custom processors so they run before ScalarProcessor.
- Assert evidence of the first processor having run (e.g., capture processed context via a test double of AppLogger or expose processed context in LogEntry for tests).
🧰 Tools
🪛 PHPMD (2.15.0)
677-677: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In tests/Unit/RpcLoggerTest.php around lines 656 to 699, the test fails to
validate processor ordering because the framework's ScalarProcessor handles
integers before your custom processors are reached; change the test to prepend
your custom processors so they run before ScalarProcessor by using
addProcessorFirst() for $firstProcessor and $secondProcessor (instead of
addProcessor()), and update the assertion to verify the first processor was
applied (e.g., inspect the LogEntry payload or use the test double AppLogger to
capture the processed context and assert it contains the expected "first:42"
result); if payload inspection isn't possible, replace or augment the logger
test double to expose the processed context for assertion.
There was a problem hiding this comment.
Hello. I'm thinking about how we can optimize this. The approach with processors is quite flexible, but we need a very high-performance logger because I plan to use it in Temporal Workflows. I think we should simplify some things for better performance.
- We should move the processors to the Internal folder and mark all classes as
@internal. This way, we can redesign them in the future without breaking backward compatibility. - I think we should handle built-in types (like null, string, int, float, bool, array) in one processor or even directly in the manager. We need processors only for objects of specific classes, right?
Anw, what do you think if we allow adding specific processors in user view? |
That's why builtin types might be processed right in the manager. |
… scalar, and resource handling
…TypeProcessor - Removed ArrayProcessor, NullProcessor, and ScalarProcessor. - Introduced BuiltInTypeProcessor to handle null, scalar, and array types efficiently. - Updated ContextProcessorManager to register BuiltInTypeProcessor. - Added unit tests for BuiltInTypeProcessor to ensure correct functionality.
|
@roxblnfk I have updated them, pls confirm again. tysm |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
tests/Unit/ContextProcessor/ContextProcessorManagerTest.php (1)
81-95: Add regression tests for cyclic references to prevent infinite recursionEchoing the earlier note: arrays/objects can self‑reference and will currently recurse unbounded. Add tests for self‑referential arrays and mutually‑referencing objects; then implement a recursion guard in the manager (seen-set + max depth).
I can provide a ready-to-merge guard for ContextProcessorManager plus tests if you’d like.
src/RpcLogger.php (1)
52-56: Potential TypeError: cast Stringable messages to string consistently (duplicate of prior review)Some branches pass $message as \Stringable to AppLogger, which likely expects string. This can throw under strict types. Cast once and reuse to avoid per-branch discrepancies.
Apply this diff to cast once and remove per-branch casts:
// Process context data for structured logging using the processor manager $processedContext = $this->contextProcessor->processContext($context); + $message = (string) $message; match ($normalizedLevel) { PsrLogLevel::EMERGENCY, PsrLogLevel::ALERT, PsrLogLevel::CRITICAL, - 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($message, $processedContext), + PsrLogLevel::WARNING => $this->logger->warning($message, $processedContext), + PsrLogLevel::NOTICE, PsrLogLevel::INFO => $this->logger->info($message, $processedContext), + 'log' => $this->logger->log($message, $processedContext), + PsrLogLevel::DEBUG => $this->logger->debug($message, $processedContext), };tests/Unit/ContextProcessor/ThrowableProcessorTest.php (1)
84-95: Fix brittle assertion for anonymous exception class name.get_class() for an anonymous class produces a name like "class@anonymous..." (parent class name is not guaranteed to be present). Assert the generic marker instead of "Exception@anonymous".
Apply:
- $this->assertTrue(\str_contains($result['class'], 'Exception@anonymous')); + $this->assertStringContainsString('@anonymous', $result['class']);tests/Unit/RpcLoggerTest.php (3)
544-581: Custom processor precedence is ineffective; BuiltInTypeProcessor will preempt strings. Prepend or assert processed output.ContextProcessorManager registers BuiltInTypeProcessor first; addProcessor() appends. Your email processor won’t run for strings because BuiltInTypeProcessor short-circuits first. Also, the test doesn’t assert that masking actually occurred.
Recommended changes:
- Introduce a prepend API on ContextProcessorManager and use it in tests.
- Assert the processed context by invoking $processorManager->processContext($context) directly (in addition to the existing RPC assertions).
Patch the test:
- // Create processor manager with custom processor added first - $processorManager = new ContextProcessorManager(); - $processorManager->addProcessor($emailProcessor); + // Create processor manager and prepend the custom processor so it runs before defaults + $processorManager = new ContextProcessorManager(); + $processorManager->addProcessorFirst($emailProcessor); // Create logger with custom processor manager $logger = new RpcLogger($this->appLogger, $processorManager); $context = [ 'user_email' => '[email protected]', 'admin_email' => '[email protected]', 'regular_string' => 'not an email', 'user_id' => 123, ]; + // Assert transformation happened at the manager level + $processed = $processorManager->processContext($context); + $this->assertSame('jo***@example.com', $processed['user_email']); + $this->assertSame('ad***@company.org', $processed['admin_email']); + $this->assertSame('not an email', $processed['regular_string']); + $this->assertSame(123, $processed['user_id']); + $logger->info('User action performed', $context);Suppress PHPMD “UnusedFormalParameter” for anonymous processor’s process() method:
- public function process(mixed $value, callable $recursiveProcessor): mixed + /** + * @SuppressWarnings(PHPMD.UnusedFormalParameter) + */ + public function process(mixed $value, callable $recursiveProcessor): mixed {Manager addition (outside this file):
--- a/src/Internal/ContextProcessor/ContextProcessorManager.php +++ b/src/Internal/ContextProcessor/ContextProcessorManager.php @@ class ContextProcessorManager public function addProcessor(ContextProcessorInterface $processor): void { $this->processors[] = $processor; } + + /** + * Prepend a processor so it runs before defaults. + */ + public function addProcessorFirst(ContextProcessorInterface $processor): void + { + \array_unshift($this->processors, $processor); + }Quick verification script to confirm current precedence:
#!/bin/bash # Expectation: BuiltInTypeProcessor is registered before others, and addProcessor appends. rg -nC2 'class ContextProcessorManager|registerDefaultProcessors|function\s+addProcessor\(' srcAlso applies to: 561-564, 575-581
583-635: Multiple custom processors have the same precedence issue; also not asserting actual transformation.URL and CC processors are appended after defaults, so strings never reach them. Strengthen the test by prepending processors and asserting processed shapes via the manager.
Apply:
- $processorManager = new ContextProcessorManager(); - $processorManager->addProcessor($urlProcessor); - $processorManager->addProcessor($ccProcessor); + $processorManager = new ContextProcessorManager(); + $processorManager->addProcessorFirst($urlProcessor); + $processorManager->addProcessorFirst($ccProcessor);Add explicit assertions:
- $logger = new RpcLogger($this->appLogger, $processorManager); + $logger = new RpcLogger($this->appLogger, $processorManager); $context = [ 'website' => 'https://example.com/path/to/resource', 'payment_card' => '1234-5678-9012-3456', 'regular_data' => 'normal string', 'amount' => 99.99, ]; + $processed = $processorManager->processContext($context); + $this->assertSame( + ['scheme' => 'https', 'host' => 'example.com', 'path' => '/path/to/resource'], + $processed['website'] + ); + $this->assertSame('****-****-****-3456', $processed['payment_card']); + $this->assertSame('normal string', $processed['regular_data']); + $this->assertSame(99.99, $processed['amount']);Suppress PHPMD in the two anonymous processors:
- public function process(mixed $value, callable $recursiveProcessor): mixed + /** + * @SuppressWarnings(PHPMD.UnusedFormalParameter) + */ + public function process(mixed $value, callable $recursiveProcessor): mixed {(repeat for the credit-card processor)
Also applies to: 616-619, 629-635
656-699: Ordering test doesn’t validate ordering due to BuiltInTypeProcessor handling ints first. Prepend and assert via manager.As written, your int processors won’t run. Prepend them and assert that the first one wins by inspecting the manager’s output.
Apply:
- $processorManager = new ContextProcessorManager(); - $processorManager->addProcessor($firstProcessor); // Added first, should be used - $processorManager->addProcessor($secondProcessor); // Added second, should be skipped + $processorManager = new ContextProcessorManager(); + $processorManager->addProcessorFirst($firstProcessor); // Prepend to run before BuiltInTypeProcessor + $processorManager->addProcessorFirst($secondProcessor); // Still after $firstProcessorAssert ordering effect explicitly:
- $context = ['number' => 42]; + $context = ['number' => 42]; + $processed = $processorManager->processContext($context); + $this->assertSame('first:42', $processed['number']); $logger->debug('Ordering test', $context);Suppress PHPMD in both anonymous processors:
- public function process(mixed $value, callable $recursiveProcessor): mixed + /** + * @SuppressWarnings(PHPMD.UnusedFormalParameter) + */ + public function process(mixed $value, callable $recursiveProcessor): mixed {Also applies to: 683-686, 689-699
🧹 Nitpick comments (26)
composer.json (1)
46-46: Split coverage run into a separate script to keepcomposer testfast and xdebug-agnosticAlways enabling Xdebug slows local/CI runs and fails if Xdebug isn’t installed. Recommend keeping
testlean and adding a dedicatedtest:coveragescript.Apply:
"scripts": { "cs:diff": "php-cs-fixer fix --dry-run -v --diff --show-progress dots", "cs:fix": "php-cs-fixer fix -v", - "test": "XDEBUG_MODE=coverage phpunit", + "test": "phpunit", + "test:coverage": "php -d xdebug.mode=coverage phpunit", "psalm": "psalm" },src/Internal/ContextProcessor/StringableProcessor.php (2)
16-16: Make the classfinalsince it’s internalPrevents unintended extension and allows future internal changes without BC guarantees.
-class StringableProcessor implements ContextProcessorInterface +final class StringableProcessor implements ContextProcessorInterface
23-31: Suppress PHPMD false-positive for unused$recursiveProcessorSignature is mandated by the interface; this processor doesn’t need recursion. Suppress the warning locally.
/** * @param \Stringable $value * @param callable(mixed): mixed $recursiveProcessor * @return string + * @SuppressWarnings("UnusedFormalParameter") */ public function process(mixed $value, callable $recursiveProcessor): mixed { return (string) $value; }src/Internal/ContextProcessor/DateTimeProcessor.php (3)
17-17: Considerfinalfor internal classSame rationale as other internal processors.
-class DateTimeProcessor implements ContextProcessorInterface +final class DateTimeProcessor implements ContextProcessorInterface
24-32: Suppress PHPMD unused-parameter warning on$recursiveProcessorProcessor doesn’t recurse; keep interface but silence the false-positive.
/** * @param \DateTimeInterface $value * @param callable(mixed): mixed $recursiveProcessor * @return string + * @SuppressWarnings("UnusedFormalParameter") */ public function process(mixed $value, callable $recursiveProcessor): mixed { return $value->format(\DateTimeInterface::ATOM); }
29-32: Optional: include microseconds for higher-resolution logsIf you want sub-second precision, consider RFC3339_EXTENDED.
- return $value->format(\DateTimeInterface::ATOM); + return $value->format(\DateTimeInterface::RFC3339_EXTENDED);If compatibility with existing consumers relies on ATOM, keep current behavior.
src/Internal/ContextProcessor/ObjectProcessor.php (2)
17-17: Mark asfinalInternal utility that shouldn’t be extended externally.
-class ObjectProcessor implements ContextProcessorInterface +final class ObjectProcessor implements ContextProcessorInterface
48-50: Fallback to class name is reasonable; consider JsonSerializable laterReturning class name when no public properties exist is a sensible default. As a future enhancement, you might special-case JsonSerializable to serialize its payload, if present.
src/Internal/ContextProcessor/ResourceProcessor.php (3)
16-16: Make classfinalConsistent with other internal processors; avoids accidental inheritance.
-class ResourceProcessor implements ContextProcessorInterface +final class ResourceProcessor implements ContextProcessorInterface
23-31: Suppress PHPMD unused-parameter warningSame interface constraint; suppress locally.
/** * @param resource $value * @param callable(mixed): mixed $recursiveProcessor * @return string + * @SuppressWarnings("UnusedFormalParameter") */ public function process(mixed $value, callable $recursiveProcessor): mixed { return \get_resource_type($value) . ' resource'; }
30-31: Nit: unify resource string format (optional)Some teams prefer
resource(type)for quick scanning. Purely stylistic; current output is fine.- return \get_resource_type($value) . ' resource'; + return 'resource(' . \get_resource_type($value) . ')';src/Internal/ContextProcessor/ContextProcessorInterface.php (1)
25-32: Nit: tighten the contract around the recursive callback and behaviorConsider documenting behavioral guarantees to guide implementors and help static analyzers:
- State that processors should be side‑effect free and avoid throwing (or document expected exceptions).
- Name and describe the recursion callback as a stable “processValue” contract to make intent explicit.
Example doc enhancement:
/** * Process the value and return a serializable representation. * * @param TValue $value The value to process - * @param callable(mixed): mixed $recursiveProcessor Function to process nested values recursively + * @param callable(mixed): mixed $recursiveProcessor Recursor provided by the manager; + * must be used to process nested values. Implementations should be side‑effect free + * and avoid throwing; return a best‑effort representation instead. * @return TProcessed Processed value suitable for logging */src/Internal/ContextProcessor/FallbackProcessor.php (2)
17-17: Make internal processors finalThese classes are internal implementation details; marking them final eases refactoring and enables engine optimizations.
-class FallbackProcessor implements ContextProcessorInterface +final class FallbackProcessor implements ContextProcessorInterface
19-23: Silence PHPMD UnusedFormalParameter warnings intentionallyIn canProcess the parameter is unused by design; in process the recursive callback is unused by design. Suppress these to keep CI noise down.
class FallbackProcessor implements ContextProcessorInterface { + /** + * @SuppressWarnings(PHPMD.UnusedFormalParameter) + */ public function canProcess(mixed $value): bool { // This processor can handle anything as a last resort return true; } /** * @param callable(mixed): mixed $recursiveProcessor * @return string + * @SuppressWarnings(PHPMD.UnusedFormalParameter) */ public function process(mixed $value, callable $recursiveProcessor): mixed { return \gettype($value); } }Also applies to: 25-33
src/Internal/ContextProcessor/ThrowableProcessor.php (2)
17-17: Make internal processors finalSame rationale as other processors.
-class ThrowableProcessor implements ContextProcessorInterface +final class ThrowableProcessor implements ContextProcessorInterface
24-39: Leverage the recursive processor for chained exceptionsIncluding previous exceptions makes the structured payload complete and also removes the “unused parameter” warning. Suggest processing the chain via the supplied recursor.
public function process(mixed $value, callable $recursiveProcessor): mixed { - return [ + $previous = $value->getPrevious(); + + return [ 'class' => \get_class($value), 'message' => $value->getMessage(), 'code' => $value->getCode(), 'file' => $value->getFile(), 'line' => $value->getLine(), - 'trace' => $value->getTraceAsString(), + 'trace' => $value->getTraceAsString(), + // Include previous exception chain (if any) + 'previous' => $previous !== null ? $recursiveProcessor($previous) : null, ]; }tests/Unit/ContextProcessor/ObjectProcessorTest.php (1)
44-49: PHPMD unused private fields in test fixtures are intentionalThe anonymous class’ private field is present to verify exclusion of non‑public props. Consider suppressing PHPMD for tests or configuring the ruleset to ignore tests to avoid false positives.
tests/Unit/ContextProcessor/ContextProcessorManagerTest.php (2)
122-145: Precedence control for custom processorsThis test demonstrates that custom processors added after defaults can’t override built‑ins (strings handled by BuiltInTypeProcessor). Consider supporting a “prepend” registration to enable targeted overrides.
Example API and usage:
- public function addProcessor(ContextProcessorInterface $processor): void + public function addProcessor(ContextProcessorInterface $processor): void { $this->processors[] = $processor; } + + /** + * Register a processor with higher priority (checked before existing ones). + */ + public function prependProcessor(ContextProcessorInterface $processor): void + { + \array_unshift($this->processors, $processor); + }Then in the test:
- $this->manager->addProcessor($customProcessor); + $this->manager->prependProcessor($customProcessor);And assert the transformed value accordingly.
25-38: Optional: add a test asserting numeric keys become stringsprocessContext stringifies keys; add a case like
['0' => 'a']and[0 => 'a']to ensure stable behavior and set expectations in tests.src/RpcLogger.php (1)
22-26: Constructor injection of ContextProcessorManager looks goodOptional enhancement: rename the property to $contextProcessorManager for clarity (it holds a manager, not a single processor). Not a blocker.
tests/Unit/ContextProcessor/BuiltInTypeProcessorTest.php (1)
17-32: Avoid leaking resources from the data providerThe provider opens a stream that is never closed. Wrap the resource creation in a closure and close it in the test after the assertion.
Apply this diff:
public static function builtInTypeValuesProvider(): array { return [ @@ - 'resource' => [\fopen('php://memory', 'r'), false], + // Defer resource creation to the test to ensure proper closing + 'resource' => [static fn() => \fopen('php://memory', 'r'), false], ]; } @@ - public function testCanProcessBuiltInTypes(mixed $value, bool $expected): void + public function testCanProcessBuiltInTypes(mixed $value, bool $expected): void { - $this->assertSame($expected, $this->processor->canProcess($value)); + if (\is_callable($value)) { + $value = $value(); + } + try { + $this->assertSame($expected, $this->processor->canProcess($value)); + } finally { + if (\is_resource($value)) { + \fclose($value); + } + } }tests/Unit/ContextProcessor/ResourceProcessorTest.php (3)
43-53: Use try/finally to guarantee stream closure on assertion failuresMinor hygiene to prevent leaks if an assertion throws.
public function testProcessStreamResource(): void { - $resource = \fopen('php://memory', 'r'); - $recursiveProcessor = static fn($v) => $v; - - $result = $this->processor->process($resource, $recursiveProcessor); - - \fclose($resource); - - $this->assertSame('stream resource', $result); + $resource = \fopen('php://memory', 'r'); + try { + $recursiveProcessor = static fn($v) => $v; + $result = $this->processor->process($resource, $recursiveProcessor); + $this->assertSame('stream resource', $result); + } finally { + \fclose($resource); + } }
55-67: Same: ensure file handle cleanup via try/finallyKeeps temp file and handle lifecycle safe even on failures.
public function testProcessFileResource(): void { $tempFile = \tempnam(\sys_get_temp_dir(), 'test'); - $resource = \fopen($tempFile, 'w'); - $recursiveProcessor = static fn($v) => $v; - - $result = $this->processor->process($resource, $recursiveProcessor); - - \fclose($resource); - \unlink($tempFile); - - $this->assertSame('stream resource', $result); + $resource = \fopen($tempFile, 'w'); + try { + $recursiveProcessor = static fn($v) => $v; + $result = $this->processor->process($resource, $recursiveProcessor); + $this->assertSame('stream resource', $result); + } finally { + \fclose($resource); + \unlink($tempFile); + } }
69-88: Tidy up cURL test: ensure handles are always closedHandles both CurlHandle object (PHP 8+) and resource cleanup deterministically.
public function testProcessCurlResource(): void { if (!\extension_loaded('curl')) { $this->markTestSkipped('cURL extension not available'); } - $curlHandle = \curl_init(); - - // In PHP 8+, curl_init returns CurlHandle object, not resource - // Let's test with a different resource type that's consistently a resource - $resource = \fopen('data://text/plain;base64,SGVsbG8gV29ybGQ=', 'r'); - $recursiveProcessor = static fn($v) => $v; - - $result = $this->processor->process($resource, $recursiveProcessor); - - \fclose($resource); - \curl_close($curlHandle); - - $this->assertSame('stream resource', $result); + $curlHandle = \curl_init(); + $resource = \fopen('data://text/plain;base64,SGVsbG8gV29ybGQ=', 'r'); + try { + $recursiveProcessor = static fn($v) => $v; + $result = $this->processor->process($resource, $recursiveProcessor); + $this->assertSame('stream resource', $result); + } finally { + \fclose($resource); + if (\is_resource($curlHandle) || $curlHandle !== null) { + \curl_close($curlHandle); + } + } }src/Internal/ContextProcessor/ContextProcessorManager.php (1)
64-77: processValue() is now redundant if the guard is inlined into processContextEither:
- keep processValue() as a thin wrapper that calls the guarded closure (for tests using processValue directly), or
- deprecate processValue() in favor of the guarded closure pattern.
If tests call processValue() directly, provide a compatibility wrapper:
- public function processValue(mixed $value): mixed + public function processValue(mixed $value): mixed { - foreach ($this->processors as $processor) { - if ($processor->canProcess($value)) { - return $processor->process($value, [$this, 'processValue']); - } - } - // This should never happen due to FallbackProcessor, but just in case - return \gettype($value); + // Compatibility path without guard; consider routing via processContext for safety + foreach ($this->processors as $processor) { + if ($processor->canProcess($value)) { + return $processor->process($value, [$this, 'processValue']); + } + } + return \gettype($value); }Up to you whether to route it through the guarded path; if you do, annotate the potential performance overhead.
Would you like me to adapt tests (ContextProcessorManagerTest et al.) if you choose to route processValue through the guarded path?
tests/Unit/RpcLoggerTest.php (1)
466-470: Suppress PHPMD false-positive for intentionally unused private/protected props in object-fixture.Private/protected props are present by design to validate that ObjectProcessor only extracts public properties. PHPMD reports UnusedPrivateField here. Add an inline suppression on the private property to keep static analysis green without altering the fixture’s intent.
- private string $privateProp = 'private value'; + /** @SuppressWarnings(PHPMD.UnusedPrivateField) */ + private string $privateProp = 'private value';
📜 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 (20)
composer.json(1 hunks)src/Internal/ContextProcessor/BuiltInTypeProcessor.php(1 hunks)src/Internal/ContextProcessor/ContextProcessorInterface.php(1 hunks)src/Internal/ContextProcessor/ContextProcessorManager.php(1 hunks)src/Internal/ContextProcessor/DateTimeProcessor.php(1 hunks)src/Internal/ContextProcessor/FallbackProcessor.php(1 hunks)src/Internal/ContextProcessor/ObjectProcessor.php(1 hunks)src/Internal/ContextProcessor/ResourceProcessor.php(1 hunks)src/Internal/ContextProcessor/StringableProcessor.php(1 hunks)src/Internal/ContextProcessor/ThrowableProcessor.php(1 hunks)src/RpcLogger.php(2 hunks)tests/Unit/ContextProcessor/BuiltInTypeProcessorTest.php(1 hunks)tests/Unit/ContextProcessor/ContextProcessorManagerTest.php(1 hunks)tests/Unit/ContextProcessor/DateTimeProcessorTest.php(1 hunks)tests/Unit/ContextProcessor/FallbackProcessorTest.php(1 hunks)tests/Unit/ContextProcessor/ObjectProcessorTest.php(1 hunks)tests/Unit/ContextProcessor/ResourceProcessorTest.php(1 hunks)tests/Unit/ContextProcessor/StringableProcessorTest.php(1 hunks)tests/Unit/ContextProcessor/ThrowableProcessorTest.php(1 hunks)tests/Unit/RpcLoggerTest.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/Unit/ContextProcessor/DateTimeProcessorTest.php
- tests/Unit/ContextProcessor/StringableProcessorTest.php
- tests/Unit/ContextProcessor/FallbackProcessorTest.php
🧰 Additional context used
🧬 Code graph analysis (16)
src/Internal/ContextProcessor/StringableProcessor.php (3)
src/Internal/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(23-23)process(32-32)src/Internal/ContextProcessor/DateTimeProcessor.php (2)
canProcess(19-22)process(29-32)src/Internal/ContextProcessor/ObjectProcessor.php (2)
canProcess(19-22)process(29-50)
src/Internal/ContextProcessor/FallbackProcessor.php (6)
src/Internal/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(23-23)process(32-32)src/Internal/ContextProcessor/DateTimeProcessor.php (2)
canProcess(19-22)process(29-32)src/Internal/ContextProcessor/ObjectProcessor.php (2)
canProcess(19-22)process(29-50)src/Internal/ContextProcessor/ResourceProcessor.php (2)
canProcess(18-21)process(28-31)src/Internal/ContextProcessor/StringableProcessor.php (2)
canProcess(18-21)process(28-31)src/Internal/ContextProcessor/ThrowableProcessor.php (2)
canProcess(19-22)process(29-39)
src/Internal/ContextProcessor/ObjectProcessor.php (6)
src/Internal/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(23-23)process(32-32)src/Internal/ContextProcessor/DateTimeProcessor.php (2)
canProcess(19-22)process(29-32)src/Internal/ContextProcessor/FallbackProcessor.php (2)
canProcess(19-23)process(29-32)src/Internal/ContextProcessor/ResourceProcessor.php (2)
canProcess(18-21)process(28-31)src/Internal/ContextProcessor/StringableProcessor.php (2)
canProcess(18-21)process(28-31)src/Internal/ContextProcessor/ThrowableProcessor.php (2)
canProcess(19-22)process(29-39)
src/Internal/ContextProcessor/ResourceProcessor.php (1)
src/Internal/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(23-23)process(32-32)
src/Internal/ContextProcessor/ContextProcessorInterface.php (7)
tests/Unit/ContextProcessor/ContextProcessorManagerTest.php (2)
canProcess(125-128)process(130-133)src/Internal/ContextProcessor/DateTimeProcessor.php (2)
canProcess(19-22)process(29-32)src/Internal/ContextProcessor/FallbackProcessor.php (2)
canProcess(19-23)process(29-32)src/Internal/ContextProcessor/ObjectProcessor.php (2)
canProcess(19-22)process(29-50)src/Internal/ContextProcessor/ResourceProcessor.php (2)
canProcess(18-21)process(28-31)src/Internal/ContextProcessor/StringableProcessor.php (2)
canProcess(18-21)process(28-31)src/Internal/ContextProcessor/ThrowableProcessor.php (2)
canProcess(19-22)process(29-39)
src/Internal/ContextProcessor/DateTimeProcessor.php (1)
src/Internal/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(23-23)process(32-32)
tests/Unit/RpcLoggerTest.php (10)
src/Internal/ContextProcessor/ContextProcessorManager.php (2)
ContextProcessorManager(15-102)addProcessor(29-32)src/RpcLogger.php (2)
log(35-59)RpcLogger(15-60)tests/Unit/RpcSpy.php (4)
getCallCount(42-45)getLastCall(37-40)RpcSpy(17-51)reset(47-50)src/Internal/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(23-23)process(32-32)src/Internal/ContextProcessor/DateTimeProcessor.php (2)
canProcess(19-22)process(29-32)src/Internal/ContextProcessor/FallbackProcessor.php (2)
canProcess(19-23)process(29-32)src/Internal/ContextProcessor/ObjectProcessor.php (2)
canProcess(19-22)process(29-50)src/Internal/ContextProcessor/ResourceProcessor.php (2)
canProcess(18-21)process(28-31)src/Internal/ContextProcessor/StringableProcessor.php (2)
canProcess(18-21)process(28-31)src/Internal/ContextProcessor/ThrowableProcessor.php (2)
canProcess(19-22)process(29-39)
src/Internal/ContextProcessor/ContextProcessorManager.php (9)
src/RpcLogger.php (1)
__construct(22-26)src/Internal/ContextProcessor/BuiltInTypeProcessor.php (3)
canProcess(22-25)process(32-53)BuiltInTypeProcessor(20-54)src/Internal/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(23-23)process(32-32)src/Internal/ContextProcessor/DateTimeProcessor.php (3)
canProcess(19-22)process(29-32)DateTimeProcessor(17-33)src/Internal/ContextProcessor/FallbackProcessor.php (3)
canProcess(19-23)process(29-32)FallbackProcessor(17-33)src/Internal/ContextProcessor/ObjectProcessor.php (3)
canProcess(19-22)process(29-50)ObjectProcessor(17-51)src/Internal/ContextProcessor/ResourceProcessor.php (3)
canProcess(18-21)process(28-31)ResourceProcessor(16-32)src/Internal/ContextProcessor/StringableProcessor.php (3)
canProcess(18-21)process(28-31)StringableProcessor(16-32)src/Internal/ContextProcessor/ThrowableProcessor.php (3)
canProcess(19-22)process(29-39)ThrowableProcessor(17-40)
src/Internal/ContextProcessor/ThrowableProcessor.php (5)
src/Internal/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(23-23)process(32-32)src/Internal/ContextProcessor/DateTimeProcessor.php (2)
canProcess(19-22)process(29-32)src/Internal/ContextProcessor/FallbackProcessor.php (2)
canProcess(19-23)process(29-32)src/Internal/ContextProcessor/ObjectProcessor.php (2)
canProcess(19-22)process(29-50)src/Internal/ContextProcessor/StringableProcessor.php (2)
canProcess(18-21)process(28-31)
tests/Unit/ContextProcessor/BuiltInTypeProcessorTest.php (2)
tests/Unit/ContextProcessor/ContextProcessorManagerTest.php (5)
canProcess(125-128)process(130-133)testProcessScalarValues(25-38)testProcessNestedArray(81-95)setUp(204-207)src/Internal/ContextProcessor/BuiltInTypeProcessor.php (3)
BuiltInTypeProcessor(20-54)canProcess(22-25)process(32-53)
tests/Unit/ContextProcessor/ThrowableProcessorTest.php (2)
src/Internal/ContextProcessor/ThrowableProcessor.php (3)
ThrowableProcessor(17-40)canProcess(19-22)process(29-39)src/Internal/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(23-23)process(32-32)
tests/Unit/ContextProcessor/ResourceProcessorTest.php (2)
src/Internal/ContextProcessor/ResourceProcessor.php (3)
ResourceProcessor(16-32)canProcess(18-21)process(28-31)src/Internal/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(23-23)process(32-32)
src/Internal/ContextProcessor/BuiltInTypeProcessor.php (1)
tests/Unit/ContextProcessor/ContextProcessorManagerTest.php (2)
canProcess(125-128)process(130-133)
src/RpcLogger.php (1)
src/Internal/ContextProcessor/ContextProcessorManager.php (3)
ContextProcessorManager(15-102)__construct(20-23)processContext(42-62)
tests/Unit/ContextProcessor/ContextProcessorManagerTest.php (9)
src/Internal/ContextProcessor/ContextProcessorManager.php (3)
ContextProcessorManager(15-102)processContext(42-62)addProcessor(29-32)src/Internal/ContextProcessor/BuiltInTypeProcessor.php (2)
canProcess(22-25)process(32-53)src/Internal/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(23-23)process(32-32)src/Internal/ContextProcessor/DateTimeProcessor.php (2)
canProcess(19-22)process(29-32)src/Internal/ContextProcessor/FallbackProcessor.php (2)
canProcess(19-23)process(29-32)src/Internal/ContextProcessor/ObjectProcessor.php (2)
canProcess(19-22)process(29-50)src/Internal/ContextProcessor/ResourceProcessor.php (2)
canProcess(18-21)process(28-31)src/Internal/ContextProcessor/StringableProcessor.php (2)
canProcess(18-21)process(28-31)src/Internal/ContextProcessor/ThrowableProcessor.php (2)
canProcess(19-22)process(29-39)
tests/Unit/ContextProcessor/ObjectProcessorTest.php (2)
src/Internal/ContextProcessor/ObjectProcessor.php (3)
ObjectProcessor(17-51)canProcess(19-22)process(29-50)src/Internal/ContextProcessor/ContextProcessorInterface.php (2)
canProcess(23-23)process(32-32)
🪛 PHPMD (2.15.0)
src/Internal/ContextProcessor/StringableProcessor.php
28-28: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
src/Internal/ContextProcessor/FallbackProcessor.php
19-19: Avoid unused parameters such as '$value'. (Unused Code Rules)
(UnusedFormalParameter)
29-29: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
src/Internal/ContextProcessor/ResourceProcessor.php
28-28: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
src/Internal/ContextProcessor/DateTimeProcessor.php
29-29: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
tests/Unit/RpcLoggerTest.php
468-468: Avoid unused private fields such as '$privateProp'. (Unused Code Rules)
(UnusedPrivateField)
553-553: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
610-610: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
677-677: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
src/Internal/ContextProcessor/ThrowableProcessor.php
29-29: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
tests/Unit/ContextProcessor/ContextProcessorManagerTest.php
113-113: Avoid unused private fields such as '$privateProp'. (Unused Code Rules)
(UnusedPrivateField)
130-130: Avoid unused parameters such as '$recursiveProcessor'. (Unused Code Rules)
(UnusedFormalParameter)
tests/Unit/ContextProcessor/ObjectProcessorTest.php
48-48: Avoid unused private fields such as '$secret'. (Unused Code Rules)
(UnusedPrivateField)
64-64: Avoid unused private fields such as '$private'. (Unused Code Rules)
(UnusedPrivateField)
🔇 Additional comments (16)
src/Internal/ContextProcessor/StringableProcessor.php (1)
18-21: Logic is correct: handles all Stringable objects via cast
canProcessand(string)$valueare precise and side-effect free. No issues.src/Internal/ContextProcessor/DateTimeProcessor.php (1)
19-22: LGTM: robust DateTime detectionCovers DateTime and DateTimeImmutable via DateTimeInterface. Clean and predictable.
src/Internal/ContextProcessor/ResourceProcessor.php (1)
18-21: Resource detection is correct
is_resourceis the right gate; behavior is deterministic even for closed resources.src/Internal/ContextProcessor/ContextProcessorInterface.php (1)
7-17: Interface shape and documentation look solidClear internal contract, succinct docs, and practical generics for static analyzers. No issues.
src/Internal/ContextProcessor/ThrowableProcessor.php (1)
31-38: Consider configurable trace verbosity for PII/sizing controlTrace/file/line often contain file paths and can be large. If this is emitted in prod logs, add a way (config flag or alternate processor) to omit or truncate these fields.
Do you want me to sketch a minimal “SafeThrowableProcessor” variant and corresponding tests?
src/RpcLogger.php (1)
45-47: Nice: context is processed via ContextProcessorManager before dispatchThis is the right place to normalize/serialize context once, keeping the switch clean.
src/Internal/ContextProcessor/BuiltInTypeProcessor.php (2)
22-25: Good coverage of built-in types with fast-path checkcanProcess correctly short-circuits for null|scalar|array, keeping the common path cheap.
34-53: Array recursion is correct; preserve keys and delegate via callbackImplementation is clean and allocates a fresh array. No change required.
tests/Unit/ContextProcessor/BuiltInTypeProcessorTest.php (1)
68-101: Nice recursive test; mirrors manager callback behaviorThe nested array test demonstrates the contract expected by processors when delegating via the recursive callback.
src/Internal/ContextProcessor/ContextProcessorManager.php (1)
83-101: Order of default processors is sensible and future-proofBuilt-in types first, specifics next, resource, generic object, then fallback. Matches expected hit rates and specificity.
tests/Unit/ContextProcessor/ThrowableProcessorTest.php (4)
17-26: Solid coverage of Throwable variants in provider.Representative set (Exception, RuntimeException, InvalidArgumentException, Error, TypeError) makes canProcess() behavior explicit. No changes needed.
28-38: Good negative cases for canProcess().Covers common non-throwable inputs including null and arrays. Looks good.
52-69: End-to-end structure assertions are appropriate.Validates class/message/code and basic types for file/line/trace for both Exception and Error. These will catch regressions in ThrowableProcessor::process().
Also applies to: 71-82
97-101: Test fixture initialization is clear.setUp() constructs a fresh processor per test. Good.
tests/Unit/RpcLoggerTest.php (2)
359-376: Broad context coverage looks good.These tests exercise scalars, DateTimeInterface, Throwable, Stringable, nested arrays, plain objects, resources, and mixed contexts, asserting the correct RPC method selection and payload envelope type. This gives confidence that RpcLogger delegates correctly once ContextProcessorManager is involved.
Also applies to: 378-396, 397-413, 415-436, 438-461, 463-483, 485-503, 505-542
14-16: New imports align with the processor-based context pipeline.Importing ContextProcessorInterface and ContextProcessorManager here is appropriate for the added tests.
| /** | ||
| * Process context data recursively. | ||
| * | ||
| * @template TKey of array-key | ||
| * @template TValue | ||
| * @param array<TKey, TValue> $context | ||
| * @return array<string, mixed> | ||
| */ | ||
| public function processContext(array $context): array | ||
| { | ||
| if (empty($context)) { | ||
| return []; | ||
| } | ||
|
|
||
| /** @var array<string, mixed> $processed */ | ||
| $processed = []; | ||
|
|
||
| /** | ||
| * @var TKey $key | ||
| * @var TValue $value | ||
| */ | ||
| foreach ($context as $key => $value) { | ||
| $stringKey = (string) $key; | ||
| /** @psalm-suppress MixedAssignment - Intentionally processing mixed types */ | ||
| $processed[$stringKey] = $this->processValue($value); | ||
| } | ||
|
|
||
| return $processed; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add recursion guards (max depth + object cycle detection) to prevent runaway processing
Self-referential arrays/objects in context can cause infinite recursion and memory exhaustion. Add a shallow guard and track visited objects per processing call. Keep the external API unchanged by capturing guard state in the recursive callback.
Illustrative diff:
- public function processContext(array $context): array
+ public function processContext(array $context): array
{
if (empty($context)) {
return [];
}
/** @var array<string, mixed> $processed */
$processed = [];
+ $visited = new \SplObjectStorage();
+ $maxDepth = 20;
+
+ $processValue = function (mixed $value, int $depth = 0) use (&$processValue, $visited, $maxDepth): mixed {
+ if ($depth > $maxDepth) {
+ return '__max_depth_exceeded__';
+ }
+
+ if (\is_object($value)) {
+ if ($visited->contains($value)) {
+ return '__circular_ref:' . \get_class($value) . '#' . \spl_object_id($value) . '__';
+ }
+ $visited->attach($value);
+ }
+
+ foreach ($this->processors as $processor) {
+ if ($processor->canProcess($value)) {
+ // Delegate with depth increment; keep single-arg callable contract
+ return $processor->process($value, static fn(mixed $v): mixed => $processValue($v, $depth + 1));
+ }
+ }
+
+ return \gettype($value);
+ };
/**
* @var TKey $key
* @var TValue $value
*/
foreach ($context as $key => $value) {
$stringKey = (string) $key;
- /** @psalm-suppress MixedAssignment - Intentionally processing mixed types */
- $processed[$stringKey] = $this->processValue($value);
+ /** @psalm-suppress MixedAssignment - Intentionally processing mixed types */
+ $processed[$stringKey] = $processValue($value, 0);
}
return $processed;
}Note: This change confines guard state to each processContext call, so no cross-call retention.
📝 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.
| /** | |
| * Process context data recursively. | |
| * | |
| * @template TKey of array-key | |
| * @template TValue | |
| * @param array<TKey, TValue> $context | |
| * @return array<string, mixed> | |
| */ | |
| public function processContext(array $context): array | |
| { | |
| if (empty($context)) { | |
| return []; | |
| } | |
| /** @var array<string, mixed> $processed */ | |
| $processed = []; | |
| /** | |
| * @var TKey $key | |
| * @var TValue $value | |
| */ | |
| foreach ($context as $key => $value) { | |
| $stringKey = (string) $key; | |
| /** @psalm-suppress MixedAssignment - Intentionally processing mixed types */ | |
| $processed[$stringKey] = $this->processValue($value); | |
| } | |
| return $processed; | |
| } | |
| /** | |
| * Process context data recursively. | |
| * | |
| * @template TKey of array-key | |
| * @template TValue | |
| * @param array<TKey, TValue> $context | |
| * @return array<string, mixed> | |
| */ | |
| public function processContext(array $context): array | |
| { | |
| if (empty($context)) { | |
| return []; | |
| } | |
| /** @var array<string, mixed> $processed */ | |
| $processed = []; | |
| $visited = new \SplObjectStorage(); | |
| $maxDepth = 20; | |
| $processValue = function (mixed $value, int $depth = 0) use (&$processValue, $visited, $maxDepth): mixed { | |
| if ($depth > $maxDepth) { | |
| return '__max_depth_exceeded__'; | |
| } | |
| if (\is_object($value)) { | |
| if ($visited->contains($value)) { | |
| return '__circular_ref:' . \get_class($value) . '#' . \spl_object_id($value) . '__'; | |
| } | |
| $visited->attach($value); | |
| } | |
| foreach ($this->processors as $processor) { | |
| if ($processor->canProcess($value)) { | |
| // Delegate with depth increment; keep single-arg callable contract | |
| return $processor->process( | |
| $value, | |
| static fn (mixed $v): mixed => $processValue($v, $depth + 1) | |
| ); | |
| } | |
| } | |
| return \gettype($value); | |
| }; | |
| /** | |
| * @var TKey $key | |
| * @var TValue $value | |
| */ | |
| foreach ($context as $key => $value) { | |
| $stringKey = (string) $key; | |
| /** @psalm-suppress MixedAssignment - Intentionally processing mixed types */ | |
| $processed[$stringKey] = $processValue($value, 0); | |
| } | |
| return $processed; | |
| } |
| public function process(mixed $value, callable $recursiveProcessor): mixed | ||
| { | ||
| // Try to convert to array (for objects with public properties) | ||
| $objectVars = \get_object_vars($value); | ||
|
|
||
| if (!empty($objectVars)) { | ||
| /** @var array<string, mixed> $processed */ | ||
| $processed = []; | ||
| /** | ||
| * @var string $property | ||
| * @var mixed $propertyValue | ||
| */ | ||
| foreach ($objectVars as $property => $propertyValue) { | ||
| /** @psalm-suppress MixedAssignment - Intentionally processing mixed types */ | ||
| $processed[$property] = $recursiveProcessor($propertyValue); | ||
| } | ||
| return $processed; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify cycle handling and depth limits in the recursive path
Processing object graphs via $recursiveProcessor can loop on self-references or explode on deeply nested structures without guards. Ensure the manager tracks visited nodes and enforces a sane max depth/size.
Run:
Expected:
- Specific processors (DateTime, Throwable, Stringable) registered before ObjectProcessor.
- Presence of visited-set (e.g., SplObjectStorage) and/or max-depth guards.
🏁 Script executed:
#!/bin/bash
# Inspect manager for processor order, cycle detection, and limits
rg -nP 'class\s+ContextProcessorManager\b' -C3
rg -nP '(register|default).*processor' -n -C3 src/Internal/ContextProcessor/ContextProcessorManager.php
rg -nP '(visited|seen|SplObjectStorage|maxDepth|maxDepth|depth|cycle)' -n -C3 src/Internal/ContextProcessor/ContextProcessorManager.phpLength of output: 1187
🏁 Script executed:
#!/bin/bash
# Display the registerDefaultProcessors method including the registration calls
awk 'NR>=70 && NR<=140' src/Internal/ContextProcessor/ContextProcessorManager.phpLength of output: 1234
Add cycle detection and recursion depth limit to ContextProcessorManager
ContextProcessorManager currently delegates recursion to ObjectProcessor without tracking already-visited objects or capping recursion depth. This opens the door to infinite loops on circular references and stack overflows on deeply nested data.
• In src/Internal/ContextProcessor/ContextProcessorManager.php, processValue() should maintain a visited-object set (e.g. via SplObjectStorage) to skip re-processing the same object instance.
• Introduce a configurable maxDepth property (with a sane default) and carry a depth counter through each recursive call, aborting (or falling back) when the limit is exceeded.
• Update the call signature—either by extending processValue(mixed $value, SplObjectStorage $visited = null, int $depth = 0) or by encapsulating state in the manager—to initialize and propagate both the visited set and depth.
Addressing these will prevent unbounded recursion and ensure safe handling of self-referencing or deeply nested object graphs.
🤖 Prompt for AI Agents
In src/Internal/ContextProcessor/ObjectProcessor.php around lines 29 to 46,
ObjectProcessor delegates recursion without any cycle detection or depth checks;
update the ContextProcessorManager (not just this file) to maintain an
SplObjectStorage visited set and a configurable maxDepth (with a sane default),
and propagate an integer depth counter on recursive calls; change the
recursive/processValue call signature or have the manager provide a closure that
captures visited and depth so that before processing an object you: 1) if depth
> maxDepth return a safe fallback (null or a marker) or throw a controlled
exception; 2) if the object is already in visited skip re-processing (return a
reference marker or previously-processed value); otherwise add the object to
visited, increment depth, call the existing processor (or recursive closure) and
remove the object from visited after processing; ensure the manager initializes
visited and depth on the top-level call and that ObjectProcessor uses the
propagated state instead of unguarded recursion.
| 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), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify: 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:
Does RoadRunner\Logger\Logger accept a 'log' level (as input) distinct from PSR-3 levels, or is 'log' only a generic method name? If not supported as a level, confirm recommended handling for unknown levels.
💡 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:
- PSR-3 Compliance: RoadRunner’s logging is designed to be PSR-3 compliant[1][5]. This means it follows the Psr\Log\LoggerInterface which defines the
log($level, $message, array $context = [])method. The method namedlogis generic and allows you to specify any compliant log level as the first argument, but the levels themselves must conform to the eight PSR-3 levels[6]. - Allowed Levels: Official RoadRunner documentation and its logger plugin describe supported levels for configuration and filtering as debug, info, warn, error, panic[4][10]. However, for PHP userland and the app-logger RPC plugin, only the standard PSR-3 levels are valid as input.
- Purpose of
logMethod: Thelogmethod’s purpose is to allow for passing the level dynamically—it is not a substitute or equivalent for a 'log' level[6]. - Handling Unknown Levels: The PSR-3 specification and common implementations (as seen in Monolog and others) require that if an unknown or unsupported level is provided, an InvalidArgumentException must be thrown[3][6]. Your implementation should therefore either:
- Map only valid PSR-3 levels to the logger.
- Throw an exception or otherwise reject unknown (non-PSR-3) levels.
Summary of best practice:
Use only the officially recognized PSR-3 log levels (emergency, alert, critical, error, warning, notice, info, debug). If an unknown level, such as 'log', is provided, reject it with a suitable exception (e.g., InvalidArgumentException) as per PSR-3 guidelines[3][6].
Additional context:
- RoadRunner’s internal logging and config also allow for levels like 'panic' in addition to PSR-3, but these are for internal server-side config/transport and not exposed as input levels to the application-side logger[4][10].
- If you need custom levels, you’ll need to map them to one of the supported levels before passing to RoadRunner\Logger\Logger, or log them as context data within a permitted level.
Citations:
- 1: https://packagist.org/packages/roadrunner/psr-logger
- 2: https://betterstack.com/community/guides/scaling-php/php-roadrunner/
- 3: PSR-3 compliant logger simplesamlphp/simplesamlphp#371
- 4: https://docs.roadrunner.dev/docs/logging-and-observability/logger
- 5: https://packagist.org/packages/eplightning/roadrunner-logger
- 6: https://www.sitepoint.com/logging-with-psr-3-to-improve-reusability/
- 7: https://sys-bio.github.io/roadrunner/docs-build/PythonAPIReference/cls_Logger.html
- 8: https://docs.roadrunner.dev/docs/general/config
- 9: https://snap.berkeley.edu/project/11166188
- 10: https://docs.roadrunner.dev/docs/logging-and-observability/applogger
Remove unsupported ‘log’ level and enforce PSR-3 compliance
The ‘log’ branch currently treats “log” as if it were a valid PSR-3 level, but RoadRunner’s PHP logger only recognizes the eight standard PSR-3 levels (emergency, alert, critical, error, warning, notice, info, debug). The log() method itself is a generic entry point, not a distinct level. Accepting “log” silently risks masking invalid input.
• Remove the 'log' => $this->logger->log((string) $message, $processedContext), arm from the match statement.
• Add a default arm that throws an InvalidArgumentException for any unrecognized level, e.g.:
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
In src/RpcLogger.php around lines 48 to 56, remove the 'log' match arm (which
treats "log" as a level) and replace the match with one that only handles the
eight PSR-3 levels; add a default arm that throws a
\Psr\Log\InvalidArgumentException with a message like 'Unsupported log level
"%s". Valid levels: emergency, alert, critical, error, warning, notice, info,
debug.' (using sprintf to include the invalid level); and add/update tests to
assert that passing an invalid level (including "log") causes this exception to
be thrown.
| #[DataProvider('nonObjectProvider')] | ||
| public function testCannotProcessNonObject(mixed $value): void | ||
| { | ||
| $this->assertFalse($this->processor->canProcess($value)); | ||
| } |
There was a problem hiding this comment.
Close the resource provided by the data provider to avoid leaks
The provider opens a stream; close it in the test method to prevent handle leakage across runs.
#[DataProvider('nonObjectProvider')]
public function testCannotProcessNonObject(mixed $value): void
{
$this->assertFalse($this->processor->canProcess($value));
+ if (\is_resource($value)) {
+ \fclose($value);
+ }
}📝 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.
| #[DataProvider('nonObjectProvider')] | |
| public function testCannotProcessNonObject(mixed $value): void | |
| { | |
| $this->assertFalse($this->processor->canProcess($value)); | |
| } | |
| #[DataProvider('nonObjectProvider')] | |
| public function testCannotProcessNonObject(mixed $value): void | |
| { | |
| $this->assertFalse($this->processor->canProcess($value)); | |
| if (\is_resource($value)) { | |
| \fclose($value); | |
| } | |
| } |
🤖 Prompt for AI Agents
In tests/Unit/ContextProcessor/ObjectProcessorTest.php around lines 36 to 40,
the data provider returns a stream resource that is not closed in the test;
after asserting $this->processor->canProcess($value) is false, detect if $value
is a resource (e.g. is_resource($value)) and close it (fclose($value)) to avoid
leaking file handles across test runs.
I have separated each context type to a processor class for scalable such as
graph TD A[RpcLogger] --> B[ContextProcessorManager] B --> C[NullProcessor] B --> D[ScalarProcessor] B --> E[DateTimeProcessor] B --> F[ThrowableProcessor] B --> G[StringableProcessor] B --> H[ArrayProcessor] B --> I[ResourceProcessor] B --> J[ObjectProcessor] B --> K[FallbackProcessor] B --> L[CustomProcessor] M[Context Data] --> B B --> N[Processed Context] N --> O[RoadRunner Logger]Summary by CodeRabbit