From 673cb6d4a00edae355189f99c39977eace311326 Mon Sep 17 00:00:00 2001 From: Mehmet Aydin Bahadir Date: Mon, 11 Mar 2024 12:03:04 +0100 Subject: [PATCH 1/7] "class" is not guaranteed to return. See: https://www.php.net/manual/en/function.debug-backtrace.php --- src/Exception/Failure/FailureConverter.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Exception/Failure/FailureConverter.php b/src/Exception/Failure/FailureConverter.php index 107e178c..9ce1c55d 100644 --- a/src/Exception/Failure/FailureConverter.php +++ b/src/Exception/Failure/FailureConverter.php @@ -321,7 +321,7 @@ private static function generateStackTraceString(\Throwable $e, bool $skipIntern ); - if ($skipInternal && \str_starts_with((string)$frame['class'], 'Temporal\\')) { + if ($skipInternal && \str_starts_with((string) ($frame['class'] ?? ''), 'Temporal\\')) { if (!$isFirst) { $internals[] = $renderer; $isFirst = false; From d7bfb2930f32eb41ad4537c9b02b20e8c42758df Mon Sep 17 00:00:00 2001 From: Mehmet Aydin Bahadir Date: Mon, 11 Mar 2024 12:59:31 +0100 Subject: [PATCH 2/7] Update RedundantCastGivenDocblockType --- psalm-baseline.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 4c5bd99d..38cd03cd 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -430,7 +430,7 @@ - + From dcb8bd496a43db07abdf8f8912397fd1e3f7ecf2 Mon Sep 17 00:00:00 2001 From: Mehmet Aydin Bahadir Date: Mon, 11 Mar 2024 13:15:18 +0100 Subject: [PATCH 3/7] Added null coalescing operator for each element of the array. --- src/Exception/Failure/FailureConverter.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Exception/Failure/FailureConverter.php b/src/Exception/Failure/FailureConverter.php index 9ce1c55d..ff0c128f 100644 --- a/src/Exception/Failure/FailureConverter.php +++ b/src/Exception/Failure/FailureConverter.php @@ -314,10 +314,10 @@ private static function generateStackTraceString(\Throwable $e, bool $skipIntern $frame['file'] ?? '[internal function]', empty($frame['line']) ? '' : ":{$frame['line']}", \str_repeat(' ', $numPad), - $frame['class'], - $frame['type'], - $frame['function'], - self::renderTraceAttributes($frame['args']), + $frame['class'] ?? '', + $frame['type'] ?? '', + $frame['function'] ?? '', + self::renderTraceAttributes($frame['args'] ?? []), ); From 2c45582f726541fb8f596043be0a3e45482864b9 Mon Sep 17 00:00:00 2001 From: roxblnfk Date: Mon, 11 Mar 2024 16:35:50 +0400 Subject: [PATCH 4/7] Improve code understanding by Psalm; fix few Psalm issues --- psalm-baseline.xml | 10 ---------- src/Exception/Failure/FailureConverter.php | 19 ++++++++++--------- 2 files changed, 10 insertions(+), 19 deletions(-) diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 38cd03cd..6cb13172 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -395,9 +395,6 @@ - - \sprintf(]]> - getFailure()]]> @@ -429,13 +426,6 @@ - - - - - - - diff --git a/src/Exception/Failure/FailureConverter.php b/src/Exception/Failure/FailureConverter.php index ff0c128f..acb590f4 100644 --- a/src/Exception/Failure/FailureConverter.php +++ b/src/Exception/Failure/FailureConverter.php @@ -286,13 +286,13 @@ private static function createFailureException(Failure $failure, DataConverterIn private static function generateStackTraceString(\Throwable $e, bool $skipInternal = true): string { /** @var list|null, - * file: non-empty-string|null, - * class: class-string, + * function?: non-empty-string, + * line?: int<0, max>, + * file?: non-empty-string, + * class?: class-string, * object?: object, - * type: string, - * args: array|null + * type?: string, + * args?: array|null * }> $frames */ $frames = $e->getTrace(); @@ -308,11 +308,11 @@ private static function generateStackTraceString(\Throwable $e, bool $skipIntern continue; } - $renderer = static fn() => \sprintf( + $renderer = static fn(): string => \sprintf( "%s%s%s\n%s%s%s%s(%s)", \str_pad("#$i", $numPad, ' '), $frame['file'] ?? '[internal function]', - empty($frame['line']) ? '' : ":{$frame['line']}", + isset($frame['line']) ? ":{$frame['line']}" : '', \str_repeat(' ', $numPad), $frame['class'] ?? '', $frame['type'] ?? '', @@ -321,7 +321,7 @@ private static function generateStackTraceString(\Throwable $e, bool $skipIntern ); - if ($skipInternal && \str_starts_with((string) ($frame['class'] ?? ''), 'Temporal\\')) { + if ($skipInternal && \str_starts_with($frame['class'] ?? '', 'Temporal\\')) { if (!$isFirst) { $internals[] = $renderer; $isFirst = false; @@ -355,6 +355,7 @@ private static function generateStackTraceString(\Throwable $e, bool $skipIntern private static function renderTraceAttributes(?array $args): string { + /** @psalm-suppress RiskyTruthyFalsyComparison */ if (empty($args)) { return ''; } From 343983325d5165758c52f9f8eab045cdf042a4d8 Mon Sep 17 00:00:00 2001 From: Mehmet Aydin Bahadir Date: Mon, 11 Mar 2024 15:58:15 +0100 Subject: [PATCH 5/7] Added unit tests. --- src/Exception/Failure/FailureConverter.php | 1 - .../Exception/FailureConverterTestCase.php | 42 +++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/src/Exception/Failure/FailureConverter.php b/src/Exception/Failure/FailureConverter.php index acb590f4..36bc59c1 100644 --- a/src/Exception/Failure/FailureConverter.php +++ b/src/Exception/Failure/FailureConverter.php @@ -320,7 +320,6 @@ private static function generateStackTraceString(\Throwable $e, bool $skipIntern self::renderTraceAttributes($frame['args'] ?? []), ); - if ($skipInternal && \str_starts_with($frame['class'] ?? '', 'Temporal\\')) { if (!$isFirst) { $internals[] = $renderer; diff --git a/tests/Unit/Exception/FailureConverterTestCase.php b/tests/Unit/Exception/FailureConverterTestCase.php index 8aa2a0a1..b7fa2c03 100644 --- a/tests/Unit/Exception/FailureConverterTestCase.php +++ b/tests/Unit/Exception/FailureConverterTestCase.php @@ -4,6 +4,7 @@ namespace Temporal\Tests\Unit\Exception; +use Exception; use Temporal\DataConverter\DataConverter; use Temporal\DataConverter\EncodedValues; use Temporal\Exception\Failure\ApplicationFailure; @@ -30,4 +31,45 @@ public function testApplicationFailureCanTransferData(): void $this->assertSame('abc', $restoredDetails->getValue(0)); $this->assertSame(123, $restoredDetails->getValue(1)); } + + public function testShouldSetStackTraceStringForAdditionalContext(): void + { + $trace = FailureConverter::mapExceptionToFailure( + new Exception(), + DataConverter::createDefault(), + )->getStackTrace(); + + self::assertStringContainsString( + 'Temporal\Tests\Unit\Exception\FailureConverterTestCase->testShouldSetStackTraceStringForAdditionalContext()', + $trace, + ); + + self::assertStringContainsString( + 'PHPUnit\Framework\TestCase->runTest()', + $trace, + ); + } + + public function testShouldSetStackTraceStringForAdditionalContextEvenWhenClassIsNotPresented(): void + { + $trace = FailureConverter::mapExceptionToFailure( + call_user_func(fn () => new Exception()), + DataConverter::createDefault(), + )->getStackTrace(); + + self::assertStringContainsString( + 'Temporal\Tests\Unit\Exception\FailureConverterTestCase->Temporal\Tests\Unit\Exception\{closure}()', + $trace, + ); + + self::assertStringContainsString( + 'call_user_func(Closure)', + $trace, + ); + + self::assertStringContainsString( + 'PHPUnit\Framework\TestCase->runTest()', + $trace, + ); + } } From c5db23f8eff4f28ddc5cedc10e86a2d6e44ff499 Mon Sep 17 00:00:00 2001 From: roxblnfk Date: Mon, 11 Mar 2024 20:04:03 +0400 Subject: [PATCH 6/7] Fix test case --- src/Exception/Failure/FailureConverter.php | 15 +++++++-------- tests/Unit/Exception/FailureConverterTestCase.php | 7 +++++++ 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/Exception/Failure/FailureConverter.php b/src/Exception/Failure/FailureConverter.php index 36bc59c1..d7344c74 100644 --- a/src/Exception/Failure/FailureConverter.php +++ b/src/Exception/Failure/FailureConverter.php @@ -286,12 +286,12 @@ private static function createFailureException(Failure $failure, DataConverterIn private static function generateStackTraceString(\Throwable $e, bool $skipInternal = true): string { /** @var list, - * file?: non-empty-string, - * class?: class-string, - * object?: object, - * type?: string, + * function?: non-empty-string|null, + * line?: int<0, max>|null, + * file?: non-empty-string|null, + * class?: class-string|null, + * object?: object|null, + * type?: string|null, * args?: array|null * }> $frames */ @@ -336,12 +336,11 @@ private static function generateStackTraceString(\Throwable $e, bool $skipIntern '[%d hidden internal calls]', \count($internals), ); - $internals = []; } else { $result = [...$result, ...\array_map(static fn(callable $renderer) => $renderer(), $internals)]; - $internals = []; } + $internals = []; $result[] = $renderer(); } diff --git a/tests/Unit/Exception/FailureConverterTestCase.php b/tests/Unit/Exception/FailureConverterTestCase.php index b7fa2c03..8cb76a9a 100644 --- a/tests/Unit/Exception/FailureConverterTestCase.php +++ b/tests/Unit/Exception/FailureConverterTestCase.php @@ -52,11 +52,18 @@ public function testShouldSetStackTraceStringForAdditionalContext(): void public function testShouldSetStackTraceStringForAdditionalContextEvenWhenClassIsNotPresented(): void { + \ini_set('zend.exception_ignore_args', 'Off'); + $trace = FailureConverter::mapExceptionToFailure( call_user_func(fn () => new Exception()), DataConverter::createDefault(), )->getStackTrace(); + self::assertStringContainsString( + '[internal function]', + $trace, + ); + self::assertStringContainsString( 'Temporal\Tests\Unit\Exception\FailureConverterTestCase->Temporal\Tests\Unit\Exception\{closure}()', $trace, From ec4c8fd54e84883e0ec74f542b0107df4310ef76 Mon Sep 17 00:00:00 2001 From: roxblnfk Date: Mon, 11 Mar 2024 20:26:03 +0400 Subject: [PATCH 7/7] Optimize signature of the `FailureConverter::renderTraceAttributes()` method --- src/Exception/Failure/FailureConverter.php | 5 +-- .../Exception/FailureConverterTestCase.php | 39 +++++++++++++++---- 2 files changed, 34 insertions(+), 10 deletions(-) diff --git a/src/Exception/Failure/FailureConverter.php b/src/Exception/Failure/FailureConverter.php index d7344c74..099f2385 100644 --- a/src/Exception/Failure/FailureConverter.php +++ b/src/Exception/Failure/FailureConverter.php @@ -351,10 +351,9 @@ private static function generateStackTraceString(\Throwable $e, bool $skipIntern return \implode("\n", $result); } - private static function renderTraceAttributes(?array $args): string + private static function renderTraceAttributes(array $args): string { - /** @psalm-suppress RiskyTruthyFalsyComparison */ - if (empty($args)) { + if ($args === []) { return ''; } diff --git a/tests/Unit/Exception/FailureConverterTestCase.php b/tests/Unit/Exception/FailureConverterTestCase.php index 8cb76a9a..6772d3e8 100644 --- a/tests/Unit/Exception/FailureConverterTestCase.php +++ b/tests/Unit/Exception/FailureConverterTestCase.php @@ -32,7 +32,7 @@ public function testApplicationFailureCanTransferData(): void $this->assertSame(123, $restoredDetails->getValue(1)); } - public function testShouldSetStackTraceStringForAdditionalContext(): void + public function testStackTraceStringForAdditionalContext(): void { $trace = FailureConverter::mapExceptionToFailure( new Exception(), @@ -40,7 +40,7 @@ public function testShouldSetStackTraceStringForAdditionalContext(): void )->getStackTrace(); self::assertStringContainsString( - 'Temporal\Tests\Unit\Exception\FailureConverterTestCase->testShouldSetStackTraceStringForAdditionalContext()', + 'Temporal\Tests\Unit\Exception\FailureConverterTestCase->testStackTraceStringForAdditionalContext()', $trace, ); @@ -50,14 +50,19 @@ public function testShouldSetStackTraceStringForAdditionalContext(): void ); } - public function testShouldSetStackTraceStringForAdditionalContextEvenWhenClassIsNotPresented(): void + public function testStackTraceStringForAdditionalContextEvenWhenClassIsNotPresented(): void { + $previous = \ini_get('zend.exception_ignore_args'); \ini_set('zend.exception_ignore_args', 'Off'); - $trace = FailureConverter::mapExceptionToFailure( - call_user_func(fn () => new Exception()), - DataConverter::createDefault(), - )->getStackTrace(); + try { + $trace = FailureConverter::mapExceptionToFailure( + call_user_func(fn () => new Exception()), + DataConverter::createDefault(), + )->getStackTrace(); + } finally { + \ini_set('zend.exception_ignore_args', $previous); + } self::assertStringContainsString( '[internal function]', @@ -79,4 +84,24 @@ public function testShouldSetStackTraceStringForAdditionalContextEvenWhenClassIs $trace, ); } + + public function testStackTraceStringWithoutExceptionArgs(): void + { + $previous = \ini_get('zend.exception_ignore_args'); + \ini_set('zend.exception_ignore_args', 'On'); + + try { + $trace = FailureConverter::mapExceptionToFailure( + call_user_func(static fn() => new Exception()), + DataConverter::createDefault(), + )->getStackTrace(); + } finally { + \ini_set('zend.exception_ignore_args', $previous); + } + + self::assertStringContainsString( + 'call_user_func()', + $trace, + ); + } }