Refactor - Improve reflection handling and enhance test coverage in StopwatchDecorator

Author: tarlepp

src/Decorator/StopwatchDecorator.php

@@ -46,16 +46,16 @@ public function __construct(
      */
     public function decorate(object $service): object
     {
-        $class = new ReflectionClass($service);
+        $reflection = new ReflectionClass($service);
 
-        if ($this->shouldSkipDecoration($class)) {
+        if ($this->shouldSkipDecoration($reflection)) {
             return $service;
         }
 
-        [$prefixInterceptors, $suffixInterceptors] = $this->getPrefixAndSuffixInterceptors($class);
+        [$prefixInterceptors, $suffixInterceptors] = $this->getPrefixAndSuffixInterceptors($reflection);
 
         /** @var T */
-        return $this->createProxyWithInterceptors($service, $prefixInterceptors, $suffixInterceptors);
+        return $this->createProxy($service, $reflection, $prefixInterceptors, $suffixInterceptors) ?? $service;
     }
 
     // Validation methods
@@ -139,24 +139,6 @@ private function decorateReturnValue(mixed &$returnValue): void
 
     // Proxy creation methods
 
-    /**
-     * @param array<string, Closure> $prefixInterceptors
-     * @param array<string, Closure> $suffixInterceptors
-     */
-    private function createProxyWithInterceptors(
-        object $service,
-        array $prefixInterceptors,
-        array $suffixInterceptors,
-    ): object {
-        $reflection = new ReflectionClass($service);
-
-        if ($reflection->isFinal()) {
-            return $service;
-        }
-
-        return $this->createProxy($service, $reflection, $prefixInterceptors, $suffixInterceptors) ?? $service;
-    }
-
     /**
      * @param array<string, Closure> $prefixInterceptors
      * @param array<string, Closure> $suffixInterceptors
@@ -343,15 +325,10 @@ private function getDefaultValueString(\ReflectionParameter $param): string
     {
         $result = '';
 
-        try {
-            if ($param->isDefaultValueAvailable()) {
-                /** @psalm-suppress MixedAssignment */
-                $defaultValue = $param->getDefaultValue();
-                $result = ' = ' . var_export($defaultValue, true);
-            }
-        } catch (Throwable) {
-            // Default value cannot be determined for internal classes or certain parameter types
-            $result = '';
+        if ($param->isDefaultValueAvailable()) {
+            /** @psalm-suppress MixedAssignment */
+            $defaultValue = $param->getDefaultValue();
+            $result = ' = ' . var_export($defaultValue, true);
         }
 
         return $result;

tests/Integration/Decorator/StopwatchDecoratorTest.php

@@ -84,6 +84,45 @@ final public function someMethod(): string
         self::assertSame('test', $result->someMethod());
     }
 
+    #[TestDox('Test that `decorate` method returns service as-is when class itself is final')]
+    public function testThatDecoratorReturnsTheSameInstanceForFinalClass(): void
+    {
+        $service = new FinalTestService();
+
+        $stopWatch = $this->getMockBuilder(Stopwatch::class)->disableOriginalConstructor()->getMock();
+
+        $decorator = new StopwatchDecorator($stopWatch, new NullLogger());
+
+        $result = $decorator->decorate($service);
+
+        // Since the class is final, it should return the same instance
+        self::assertSame($service, $result);
+        self::assertSame('final-test', $result->testMethod());
+    }
+
+    #[TestDox('Test that decorator handles parameters with internal class default values')]
+    public function testThatDecoratorHandlesInternalClassDefaultValues(): void
+    {
+        $service = new ServiceWithInternalDefaults();
+
+        $stopWatch = $this->getMockBuilder(Stopwatch::class)->disableOriginalConstructor()->getMock();
+
+        $stopWatch
+            ->expects($this->once())
+            ->method('start');
+
+        $stopWatch
+            ->expects($this->once())
+            ->method('stop');
+
+        $decorator = new StopwatchDecorator($stopWatch, new NullLogger());
+
+        $result = $decorator->decorate($service);
+
+        // The decorated service should work with methods that have internal class default values
+        self::assertSame('test', $result->methodWithInternalDefault());
+    }
+
     /**
      * @throws Throwable
      */
@@ -164,3 +203,39 @@ public static function dataProviderTestThatDecorateMethodReturnsExpected(): Gene
         yield [stdClass::class, new stdClass()];
     }
 }
+
+/**
+ * Helper final class for testing
+ */
+final class FinalTestService
+{
+    public function testMethod(): string
+    {
+        return 'final-test';
+    }
+}
+
+/**
+ * Helper class for testing internal class default values
+ * Extends an internal class to potentially trigger reflection edge cases
+ */
+class ServiceWithInternalDefaults extends \SplFileInfo
+{
+    public function __construct()
+    {
+        parent::__construct(__FILE__);
+    }
+
+    /**
+     * Method with parameter that has complex default values
+     * This is for testing the catch block in getDefaultValueString when the decorator
+     * tries to get the default value via reflection for certain edge cases
+     *
+     * @param array<string, mixed> $options
+     */
+    public function methodWithInternalDefault(array $options = []): string
+    {
+        return 'test';
+    }
+}
+