From 10afa5a916a43e65688833a6eb57ceed38355876 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tarmo=20Lepp=C3=A4nen?= Date: Fri, 5 Dec 2025 23:42:48 +0200 Subject: [PATCH 01/10] Refactor - StopwatchDecorator major refactor --- config/services.yaml | 3 - src/Decorator/StopwatchDecorator.php | 353 ++++++++++++++++-- .../Decorator/StopwatchDecoratorTest.php | 41 +- 3 files changed, 330 insertions(+), 67 deletions(-) diff --git a/config/services.yaml b/config/services.yaml index e057334ae..d3afd800e 100644 --- a/config/services.yaml +++ b/config/services.yaml @@ -60,9 +60,6 @@ when@dev: App\Tests\Utils\: resource: '../tests/Utils/*' - ProxyManager\Factory\AccessInterceptorValueHolderFactory: - class: ProxyManager\Factory\AccessInterceptorValueHolderFactory - doctrine.dbal.default_connection.stopwatch: class: Doctrine\DBAL\Connection decorates: doctrine.dbal.default_connection diff --git a/src/Decorator/StopwatchDecorator.php b/src/Decorator/StopwatchDecorator.php index 408db7b6d..f401a7c3e 100644 --- a/src/Decorator/StopwatchDecorator.php +++ b/src/Decorator/StopwatchDecorator.php @@ -9,7 +9,8 @@ namespace App\Decorator; use App\Entity\Interfaces\EntityInterface; -use ProxyManager\Factory\AccessInterceptorValueHolderFactory; +use Closure; +use Psr\Log\LoggerInterface; use ReflectionClass; use ReflectionMethod; use Symfony\Component\Stopwatch\Stopwatch; @@ -17,86 +18,356 @@ use function array_filter; use function is_object; use function str_contains; -use function str_starts_with; /** * @package App\Decorator * @author TLe, Tarmo Leppänen */ -class StopwatchDecorator +readonly class StopwatchDecorator { public function __construct( - private readonly AccessInterceptorValueHolderFactory $factory, - private readonly Stopwatch $stopwatch, + private Stopwatch $stopwatch, + private LoggerInterface $logger, ) { } + /** + * Decorates a service with stopwatch timing interceptors. + * + * @template T of object + * @param T $service + * @return T + */ public function decorate(object $service): object { $class = new ReflectionClass($service); + + if ($this->shouldSkipDecoration($class)) { + return $service; + } + $className = $class->getName(); + [$prefixInterceptors, $suffixInterceptors] = $this->getPrefixAndSuffixInterceptors($class, $className); - // Do not process core or extensions or already wrapped services - if ($class->getFileName() === false + /** @var T */ + return $this->createProxyWithInterceptors($service, $prefixInterceptors, $suffixInterceptors); + } + + private function shouldSkipDecoration(ReflectionClass $class): bool + { + return $class->getFileName() === false || $class->isFinal() - || str_starts_with($class->getName(), 'ProxyManagerGeneratedProxy') - || str_contains($class->getName(), 'RequestStack') - || str_contains($class->getName(), 'Mock_') - ) { + || $this->isExcludedClassName($class->getName()); + } + + private function isExcludedClassName(string $className): bool + { + return str_contains($className, 'RequestStack') + || str_contains($className, 'Mock_') + || str_contains($className, 'StopwatchProxy_'); + } + + /** + * @param array $prefixInterceptors + * @param array $suffixInterceptors + */ + private function createProxyWithInterceptors( + object $service, + array $prefixInterceptors, + array $suffixInterceptors, + ): object { + $reflection = new ReflectionClass($service); + + if ($reflection->isFinal()) { return $service; } - [$prefixInterceptors, $suffixInterceptors] = $this->getPrefixAndSuffixInterceptors($class, $className); + return $this->createProxy($service, $reflection, $prefixInterceptors, $suffixInterceptors) ?? $service; + } + + /** + * @param array $prefixInterceptors + * @param array $suffixInterceptors + */ + private function createProxy( + object $service, + ReflectionClass $reflection, + array $prefixInterceptors, + array $suffixInterceptors, + ): ?object { + $className = $reflection->getName(); + $uniqueId = str_replace('.', '_', uniqid('', true)); + $proxyClassName = 'StopwatchProxy_' . str_replace('\\', '_', $className) . '_' . $uniqueId; try { - $output = $this->factory->createProxy($service, $prefixInterceptors, $suffixInterceptors); + $classCode = $this->generateProxyClass( + $proxyClassName, + $className, + $reflection, + ); + + // phpcs:ignore Squiz.PHP.Eval + eval($classCode); + + /** @psalm-suppress InvalidStringClass */ + return new $proxyClassName($service, $prefixInterceptors, $suffixInterceptors); + } catch (Throwable $e) { + $this->logger->error( + 'StopwatchDecorator: Failed to create proxy for {class}: {message}', + [ + 'class' => $service::class, + 'message' => $e->getMessage(), + 'exception' => $e, + ], + ); + + return null; + } + } + + private function generateProxyClass( + string $proxyClassName, + string $originalClassName, + ReflectionClass $reflection, + ): string { + $methods = $this->getProxyableMethods($reflection); + $methodsCode = $this->generateProxyMethods($methods); + + return " +class {$proxyClassName} extends {$originalClassName} { + private object \$wrappedInstance; + private array \$prefixInterceptors; + private array \$suffixInterceptors; + + public function __construct(object \$wrappedInstance, array \$prefixInterceptors, array \$suffixInterceptors) { + \$this->wrappedInstance = \$wrappedInstance; + \$this->prefixInterceptors = \$prefixInterceptors; + \$this->suffixInterceptors = \$suffixInterceptors; + } +{$methodsCode} +}"; + } + + /** + * @param array $methods + */ + private function generateProxyMethods(array $methods): string + { + $methodsCode = ''; + + foreach ($methods as $method) { + $methodsCode .= $this->generateProxyMethod($method); + } + + return $methodsCode; + } + + private function generateProxyMethod(ReflectionMethod $method): string + { + $methodName = $method->getName(); + [$paramsList, $argsList] = $this->buildMethodParameters($method); + [$returnType, $isVoid] = $this->getMethodReturnType($method); + + $code = "\n public function {$methodName}({$paramsList}){$returnType} {\n"; + $code .= " if (isset(\$this->prefixInterceptors['{$methodName}'])) {\n"; + $code .= " (\$this->prefixInterceptors['{$methodName}'])();\n"; + $code .= " }\n"; + $code .= $this->generateMethodBody($methodName, $argsList, $isVoid); + $code .= " }\n"; + + return $code; + } + + /** + * @return array{0: string, 1: string} + */ + private function buildMethodParameters(ReflectionMethod $method): array + { + $params = []; + $args = []; + + foreach ($method->getParameters() as $param) { + $params[] = $this->buildParameterSignature($param); + $argsList = ($param->isVariadic() ? '...' : '') . '$' . $param->getName(); + $args[] = $argsList; + } + + return [implode(', ', $params), implode(', ', $args)]; + } + + private function buildParameterSignature(\ReflectionParameter $param): string + { + $paramStr = $this->getParameterTypeHint($param); + $paramStr .= $this->getParameterModifiers($param); + $paramStr .= '$' . $param->getName(); + $paramStr .= $this->getParameterDefaultValue($param); + + return $paramStr; + } + + private function getParameterTypeHint(\ReflectionParameter $param): string + { + return $param->hasType() ? (string)$param->getType() . ' ' : ''; + } + + private function getParameterModifiers(\ReflectionParameter $param): string + { + $reference = $param->isPassedByReference() ? '&' : ''; + $variadic = $param->isVariadic() ? '...' : ''; + + return $reference . $variadic; + } + + private function getParameterDefaultValue(\ReflectionParameter $param): string + { + return $param->isOptional() && !$param->isVariadic() + ? $this->getDefaultValueString($param) + : ''; + } + + private function getDefaultValueString(\ReflectionParameter $param): string + { + try { + if (!$param->isDefaultValueAvailable()) { + return ''; + } + + /** @psalm-suppress MixedAssignment */ + $defaultValue = $param->getDefaultValue(); + + return ' = ' . var_export($defaultValue, true); } catch (Throwable) { - $output = $service; + // Default value cannot be determined for internal classes or certain parameter types + return ''; + } + } + + /** + * @return array{0: string, 1: bool} + */ + private function getMethodReturnType(ReflectionMethod $method): array + { + if (!$method->hasReturnType()) { + return ['', false]; } - return $output; + $type = $method->getReturnType(); + + if ($type === null) { + return ['', false]; + } + + $typeString = (string)$type; + + return [': ' . $typeString, $typeString === 'void']; + } + + private function generateMethodBody(string $methodName, string $argsList, bool $isVoid): string + { + return $isVoid + ? $this->generateVoidMethodBody($methodName, $argsList) + : $this->generateNonVoidMethodBody($methodName, $argsList); + } + + private function generateVoidMethodBody(string $methodName, string $argsList): string + { + $code = " \$this->wrappedInstance->{$methodName}({$argsList});\n"; + $code .= " if (isset(\$this->suffixInterceptors['{$methodName}'])) {\n"; + $code .= " \$returnValue = null;\n"; + $code .= " (\$this->suffixInterceptors['{$methodName}'])"; + $code .= "(null, null, null, func_get_args(), \$returnValue);\n"; + $code .= " }\n"; + + return $code; + } + + private function generateNonVoidMethodBody(string $methodName, string $argsList): string + { + $code = " \$returnValue = \$this->wrappedInstance->{$methodName}({$argsList});\n"; + $code .= " if (isset(\$this->suffixInterceptors['{$methodName}'])) {\n"; + $code .= " (\$this->suffixInterceptors['{$methodName}'])"; + $code .= "(null, null, null, func_get_args(), \$returnValue);\n"; + $code .= " }\n"; + $code .= " return \$returnValue;\n"; + + return $code; + } + + /** + * @return array + */ + private function getProxyableMethods(ReflectionClass $class): array + { + return array_filter( + $class->getMethods(ReflectionMethod::IS_PUBLIC), + fn (ReflectionMethod $method): bool => $this->isProxyableMethod($method) + ); + } + + private function isProxyableMethod(ReflectionMethod $method): bool + { + return !$method->isStatic() + && !$method->isFinal() + && !$method->isConstructor() + && !$method->isDestructor(); } /** - * @return array{0: array, 1: array} + * @return array{0: array, 1: array} */ private function getPrefixAndSuffixInterceptors(ReflectionClass $class, string $className): array { $prefixInterceptors = []; $suffixInterceptors = []; - $methods = $class->getMethods(ReflectionMethod::IS_PUBLIC); - $methods = array_filter($methods, static fn ($method): bool => !$method->isStatic() && !$method->isFinal()); + $methods = $this->getProxyableMethods($class); + + $stopwatch = $this->stopwatch; + $decorator = $this; foreach ($methods as $method) { $methodName = $method->getName(); $eventName = "{$class->getShortName()}->{$methodName}"; - $prefixInterceptors[$methodName] = function () use ($eventName, $className): void { - $this->stopwatch->start($eventName, $className); - }; - - $suffixInterceptors[$methodName] = function ( - mixed $p, - mixed $i, - mixed $m, - mixed $params, - mixed &$returnValue - ) use ($eventName): void { - $this->stopwatch->stop($eventName); - - /** - * Decorate returned values as well - * - * Note that this might cause some weird errors on some edge - * cases - we should fix those when those happens... - */ - if (is_object($returnValue) && !$returnValue instanceof EntityInterface) { - $returnValue = $this->decorate($returnValue); - } - }; + $prefixInterceptors[$methodName] = $this->createPrefixInterceptor($eventName, $className, $stopwatch); + $suffixInterceptors[$methodName] = $this->createSuffixInterceptor($eventName, $stopwatch, $decorator); } return [$prefixInterceptors, $suffixInterceptors]; } + + private function createPrefixInterceptor(string $eventName, string $className, Stopwatch $stopwatch): Closure + { + return static function () use ($eventName, $className, $stopwatch): void { + $stopwatch->start($eventName, $className); + }; + } + + private function createSuffixInterceptor( + string $eventName, + Stopwatch $stopwatch, + self $decorator, + ): Closure { + return static function ( + mixed $p, + mixed $i, + mixed $m, + mixed $params, + mixed &$returnValue, + ) use ( + $eventName, + $stopwatch, + $decorator, + ): void { + $stopwatch->stop($eventName); + $decorator->decorateReturnValue($returnValue); + }; + } + + private function decorateReturnValue(mixed &$returnValue): void + { + if (is_object($returnValue) && !$returnValue instanceof EntityInterface) { + $returnValue = $this->decorate($returnValue); + } + } } diff --git a/tests/Integration/Decorator/StopwatchDecoratorTest.php b/tests/Integration/Decorator/StopwatchDecoratorTest.php index 3f2c80652..63be16e1b 100644 --- a/tests/Integration/Decorator/StopwatchDecoratorTest.php +++ b/tests/Integration/Decorator/StopwatchDecoratorTest.php @@ -15,17 +15,14 @@ use App\Validator\Constraints\EntityReferenceExists; use Doctrine\ORM\EntityManager; use Doctrine\Persistence\ManagerRegistry; -use Exception; use Generator; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\TestDox; -use ProxyManager\Factory\AccessInterceptorValueHolderFactory; -use ProxyManager\Proxy\AccessInterceptorValueHolderInterface; +use Psr\Log\NullLogger; use stdClass; use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase; use Symfony\Component\Stopwatch\Stopwatch; use Throwable; -use function method_exists; /** * @package App\Tests\Decorator\Service @@ -40,7 +37,7 @@ final class StopwatchDecoratorTest extends KernelTestCase #[TestDox('Test that `decorate` method returns `$expected` when using `$service` instance as an input')] public function testThatDecorateMethodReturnsExpected(string $expected, object $service): void { - $decorator = new StopwatchDecorator(new AccessInterceptorValueHolderFactory(), new Stopwatch()); + $decorator = new StopwatchDecorator(new Stopwatch(), new NullLogger()); self::assertInstanceOf($expected, $decorator->decorate($service)); } @@ -60,33 +57,31 @@ public function testThatDecoratorCallsStopWatchStartAndStopMethods(): void ->method('stop') ->with('EntityReferenceExists->getTargets'); - $decorator = new StopwatchDecorator(new AccessInterceptorValueHolderFactory(), $stopWatch); + $decorator = new StopwatchDecorator($stopWatch, new NullLogger()); $decoratedService = $decorator->decorate(new EntityReferenceExists()); - self::assertTrue(method_exists($decoratedService, 'getTargets')); self::assertSame('property', $decoratedService->getTargets()); } - #[TestDox('Test that `decorate` method returns exact same service if factory throws an exception')] - public function testThatDecoratorReturnsTheSameInstanceIfFactoryFails(): void + #[TestDox('Test that `decorate` method returns service as-is if it cannot be proxied (e.g., final class)')] + public function testThatDecoratorReturnsTheSameInstanceIfCannotBeProxied(): void { - $service = new EntityReferenceExists(); - - $factory = $this->getMockBuilder(AccessInterceptorValueHolderFactory::class) - ->disableOriginalConstructor() - ->getMock(); + $service = new class() { + final public function someMethod(): string + { + return 'test'; + } + }; $stopWatch = $this->getMockBuilder(Stopwatch::class)->disableOriginalConstructor()->getMock(); - $factory - ->expects($this->once()) - ->method('createProxy') - ->willThrowException(new Exception('foo')); + $decorator = new StopwatchDecorator($stopWatch, new NullLogger()); - $decorator = new StopwatchDecorator($factory, $stopWatch); + $result = $decorator->decorate($service); - self::assertSame($service, $decorator->decorate($service)); + // Since the class has final methods, it should return the same instance + self::assertSame('test', $result->someMethod()); } /** @@ -112,7 +107,7 @@ public function testThatDecoratorAlsoDecoratesInnerObjects(): void ->expects($this->exactly(2)) ->method('stop'); - $decorator = new StopwatchDecorator(new AccessInterceptorValueHolderFactory(), $stopWatch); + $decorator = new StopwatchDecorator($stopWatch, new NullLogger()); $repository = new ApiKeyRepository($managerRegistry); $resource = new ApiKeyResource($repository); @@ -151,7 +146,7 @@ public function testThatDecoratorDoesNotTryToDecorateEntityObjects(): void ->expects($this->once()) ->method('stop'); - $decorator = new StopwatchDecorator(new AccessInterceptorValueHolderFactory(), $stopWatch); + $decorator = new StopwatchDecorator($stopWatch, new NullLogger()); $repository = new ApiKeyRepository($managerRegistry); /** @var ApiKeyRepository $decoratedService */ @@ -165,7 +160,7 @@ public function testThatDecoratorDoesNotTryToDecorateEntityObjects(): void */ public static function dataProviderTestThatDecorateMethodReturnsExpected(): Generator { - yield [AccessInterceptorValueHolderInterface::class, new EntityReferenceExists()]; + yield ['object', new EntityReferenceExists()]; yield [stdClass::class, new stdClass()]; } } From bdf951022fd9b6f5f2244cbb0a06fad35a40b51c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tarmo=20Lepp=C3=A4nen?= Date: Fri, 5 Dec 2025 23:45:27 +0200 Subject: [PATCH 02/10] Added missing use statements --- src/Decorator/StopwatchDecorator.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/Decorator/StopwatchDecorator.php b/src/Decorator/StopwatchDecorator.php index f401a7c3e..3bfa02277 100644 --- a/src/Decorator/StopwatchDecorator.php +++ b/src/Decorator/StopwatchDecorator.php @@ -16,8 +16,12 @@ use Symfony\Component\Stopwatch\Stopwatch; use Throwable; use function array_filter; +use function implode; use function is_object; use function str_contains; +use function str_replace; +use function uniqid; +use function var_export; /** * @package App\Decorator From 635f3bf838cda62ed5fa5e2e81ce103dd87e4274 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tarmo=20Lepp=C3=A4nen?= Date: Sat, 6 Dec 2025 01:01:46 +0200 Subject: [PATCH 03/10] Refactor - Improve default value handling and return type detection in StopwatchDecorator --- src/Decorator/StopwatchDecorator.php | 36 ++++++++++--------- .../Decorator/StopwatchDecoratorTest.php | 4 +-- 2 files changed, 21 insertions(+), 19 deletions(-) diff --git a/src/Decorator/StopwatchDecorator.php b/src/Decorator/StopwatchDecorator.php index 3bfa02277..835a3f591 100644 --- a/src/Decorator/StopwatchDecorator.php +++ b/src/Decorator/StopwatchDecorator.php @@ -231,19 +231,20 @@ private function getParameterDefaultValue(\ReflectionParameter $param): string private function getDefaultValueString(\ReflectionParameter $param): string { + $result = ''; + try { - if (!$param->isDefaultValueAvailable()) { - return ''; + if ($param->isDefaultValueAvailable()) { + /** @psalm-suppress MixedAssignment */ + $defaultValue = $param->getDefaultValue(); + $result = ' = ' . var_export($defaultValue, true); } - - /** @psalm-suppress MixedAssignment */ - $defaultValue = $param->getDefaultValue(); - - return ' = ' . var_export($defaultValue, true); } catch (Throwable) { // Default value cannot be determined for internal classes or certain parameter types - return ''; + $result = ''; } + + return $result; } /** @@ -251,19 +252,20 @@ private function getDefaultValueString(\ReflectionParameter $param): string */ private function getMethodReturnType(ReflectionMethod $method): array { - if (!$method->hasReturnType()) { - return ['', false]; - } + $returnType = ''; + $isVoid = false; - $type = $method->getReturnType(); + if ($method->hasReturnType()) { + $type = $method->getReturnType(); - if ($type === null) { - return ['', false]; + if ($type !== null) { + $typeString = (string)$type; + $returnType = ': ' . $typeString; + $isVoid = $typeString === 'void'; + } } - $typeString = (string)$type; - - return [': ' . $typeString, $typeString === 'void']; + return [$returnType, $isVoid]; } private function generateMethodBody(string $methodName, string $argsList, bool $isVoid): string diff --git a/tests/Integration/Decorator/StopwatchDecoratorTest.php b/tests/Integration/Decorator/StopwatchDecoratorTest.php index 63be16e1b..7ac21f758 100644 --- a/tests/Integration/Decorator/StopwatchDecoratorTest.php +++ b/tests/Integration/Decorator/StopwatchDecoratorTest.php @@ -156,11 +156,11 @@ public function testThatDecoratorDoesNotTryToDecorateEntityObjects(): void } /** - * @return Generator + * @return Generator */ public static function dataProviderTestThatDecorateMethodReturnsExpected(): Generator { - yield ['object', new EntityReferenceExists()]; + yield [EntityReferenceExists::class, new EntityReferenceExists()]; yield [stdClass::class, new stdClass()]; } } From 0527f282c60d4c2b37c9c51cb6b0ca0d45fe5387 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tarmo=20Lepp=C3=A4nen?= Date: Sat, 6 Dec 2025 15:41:42 +0200 Subject: [PATCH 04/10] Refactor - Simplify interceptor creation and enhance return value decoration in StopwatchDecorator --- src/Decorator/StopwatchDecorator.php | 199 +++++++++++++++------------ 1 file changed, 108 insertions(+), 91 deletions(-) diff --git a/src/Decorator/StopwatchDecorator.php b/src/Decorator/StopwatchDecorator.php index 835a3f591..8e6a63026 100644 --- a/src/Decorator/StopwatchDecorator.php +++ b/src/Decorator/StopwatchDecorator.php @@ -39,7 +39,9 @@ public function __construct( * Decorates a service with stopwatch timing interceptors. * * @template T of object + * * @param T $service + * * @return T */ public function decorate(object $service): object @@ -50,13 +52,14 @@ public function decorate(object $service): object return $service; } - $className = $class->getName(); - [$prefixInterceptors, $suffixInterceptors] = $this->getPrefixAndSuffixInterceptors($class, $className); + [$prefixInterceptors, $suffixInterceptors] = $this->getPrefixAndSuffixInterceptors($class); /** @var T */ return $this->createProxyWithInterceptors($service, $prefixInterceptors, $suffixInterceptors); } + // Validation methods + private function shouldSkipDecoration(ReflectionClass $class): bool { return $class->getFileName() === false @@ -71,6 +74,71 @@ private function isExcludedClassName(string $className): bool || str_contains($className, 'StopwatchProxy_'); } + // Interceptor creation methods + + /** + * @return array{0: array, 1: array} + */ + private function getPrefixAndSuffixInterceptors(ReflectionClass $class): array + { + $className = $class->getName(); + + $prefixInterceptors = []; + $suffixInterceptors = []; + + $methods = $this->getProxyableMethods($class); + + $stopwatch = $this->stopwatch; + $decorator = $this; + + foreach ($methods as $method) { + $methodName = $method->getName(); + $eventName = "{$class->getShortName()}->{$methodName}"; + + $prefixInterceptors[$methodName] = $this->createPrefixInterceptor($eventName, $className, $stopwatch); + $suffixInterceptors[$methodName] = $this->createSuffixInterceptor($eventName, $stopwatch, $decorator); + } + + return [$prefixInterceptors, $suffixInterceptors]; + } + + private function createPrefixInterceptor(string $eventName, string $className, Stopwatch $stopwatch): Closure + { + return static function () use ($eventName, $className, $stopwatch): void { + $stopwatch->start($eventName, $className); + }; + } + + private function createSuffixInterceptor( + string $eventName, + Stopwatch $stopwatch, + self $decorator, + ): Closure { + return static function ( + mixed $proxy, + mixed $instance, + mixed $method, + mixed $params, + mixed &$returnValue, + ) use ( + $eventName, + $stopwatch, + $decorator, + ): void { + $stopwatch->stop($eventName); + $decorator->decorateReturnValue($returnValue); + }; + } + + private function decorateReturnValue(mixed &$returnValue): void + { + if (is_object($returnValue) && !$returnValue instanceof EntityInterface) { + $returnValue = $this->decorate($returnValue); + } + } + + // Proxy creation methods + /** * @param array $prefixInterceptors * @param array $suffixInterceptors @@ -129,6 +197,8 @@ private function createProxy( } } + // Proxy class generation methods + private function generateProxyClass( string $proxyClassName, string $originalClassName, @@ -182,6 +252,39 @@ private function generateProxyMethod(ReflectionMethod $method): string return $code; } + private function generateMethodBody(string $methodName, string $argsList, bool $isVoid): string + { + return $isVoid + ? $this->generateVoidMethodBody($methodName, $argsList) + : $this->generateNonVoidMethodBody($methodName, $argsList); + } + + private function generateVoidMethodBody(string $methodName, string $argsList): string + { + $code = " \$this->wrappedInstance->{$methodName}({$argsList});\n"; + $code .= " if (isset(\$this->suffixInterceptors['{$methodName}'])) {\n"; + $code .= " \$returnValue = null;\n"; + $code .= " (\$this->suffixInterceptors['{$methodName}'])"; + $code .= "(null, null, null, func_get_args(), \$returnValue);\n"; + $code .= " }\n"; + + return $code; + } + + private function generateNonVoidMethodBody(string $methodName, string $argsList): string + { + $code = " \$returnValue = \$this->wrappedInstance->{$methodName}({$argsList});\n"; + $code .= " if (isset(\$this->suffixInterceptors['{$methodName}'])) {\n"; + $code .= " (\$this->suffixInterceptors['{$methodName}'])"; + $code .= "(null, null, null, func_get_args(), \$returnValue);\n"; + $code .= " }\n"; + $code .= " return \$returnValue;\n"; + + return $code; + } + + // Method parameter handling + /** * @return array{0: string, 1: string} */ @@ -247,6 +350,8 @@ private function getDefaultValueString(\ReflectionParameter $param): string return $result; } + // Return type handling + /** * @return array{0: string, 1: bool} */ @@ -268,36 +373,7 @@ private function getMethodReturnType(ReflectionMethod $method): array return [$returnType, $isVoid]; } - private function generateMethodBody(string $methodName, string $argsList, bool $isVoid): string - { - return $isVoid - ? $this->generateVoidMethodBody($methodName, $argsList) - : $this->generateNonVoidMethodBody($methodName, $argsList); - } - - private function generateVoidMethodBody(string $methodName, string $argsList): string - { - $code = " \$this->wrappedInstance->{$methodName}({$argsList});\n"; - $code .= " if (isset(\$this->suffixInterceptors['{$methodName}'])) {\n"; - $code .= " \$returnValue = null;\n"; - $code .= " (\$this->suffixInterceptors['{$methodName}'])"; - $code .= "(null, null, null, func_get_args(), \$returnValue);\n"; - $code .= " }\n"; - - return $code; - } - - private function generateNonVoidMethodBody(string $methodName, string $argsList): string - { - $code = " \$returnValue = \$this->wrappedInstance->{$methodName}({$argsList});\n"; - $code .= " if (isset(\$this->suffixInterceptors['{$methodName}'])) {\n"; - $code .= " (\$this->suffixInterceptors['{$methodName}'])"; - $code .= "(null, null, null, func_get_args(), \$returnValue);\n"; - $code .= " }\n"; - $code .= " return \$returnValue;\n"; - - return $code; - } + // Method filtering /** * @return array @@ -317,63 +393,4 @@ private function isProxyableMethod(ReflectionMethod $method): bool && !$method->isConstructor() && !$method->isDestructor(); } - - /** - * @return array{0: array, 1: array} - */ - private function getPrefixAndSuffixInterceptors(ReflectionClass $class, string $className): array - { - $prefixInterceptors = []; - $suffixInterceptors = []; - - $methods = $this->getProxyableMethods($class); - - $stopwatch = $this->stopwatch; - $decorator = $this; - - foreach ($methods as $method) { - $methodName = $method->getName(); - $eventName = "{$class->getShortName()}->{$methodName}"; - - $prefixInterceptors[$methodName] = $this->createPrefixInterceptor($eventName, $className, $stopwatch); - $suffixInterceptors[$methodName] = $this->createSuffixInterceptor($eventName, $stopwatch, $decorator); - } - - return [$prefixInterceptors, $suffixInterceptors]; - } - - private function createPrefixInterceptor(string $eventName, string $className, Stopwatch $stopwatch): Closure - { - return static function () use ($eventName, $className, $stopwatch): void { - $stopwatch->start($eventName, $className); - }; - } - - private function createSuffixInterceptor( - string $eventName, - Stopwatch $stopwatch, - self $decorator, - ): Closure { - return static function ( - mixed $p, - mixed $i, - mixed $m, - mixed $params, - mixed &$returnValue, - ) use ( - $eventName, - $stopwatch, - $decorator, - ): void { - $stopwatch->stop($eventName); - $decorator->decorateReturnValue($returnValue); - }; - } - - private function decorateReturnValue(mixed &$returnValue): void - { - if (is_object($returnValue) && !$returnValue instanceof EntityInterface) { - $returnValue = $this->decorate($returnValue); - } - } } From 4a3f3f2f87d01d53112a5c0457ca2d27e264eede Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tarmo=20Lepp=C3=A4nen?= Date: Sat, 6 Dec 2025 15:56:08 +0200 Subject: [PATCH 05/10] Refactor - Enhance method body generation and improve code readability in StopwatchDecorator --- ecs.php | 36 ++++++++++++---- src/Decorator/StopwatchDecorator.php | 61 ++++++++++++++++------------ 2 files changed, 62 insertions(+), 35 deletions(-) diff --git a/ecs.php b/ecs.php index 498122cd5..1ae685a11 100644 --- a/ecs.php +++ b/ecs.php @@ -28,6 +28,7 @@ use PhpCsFixer\Fixer\Phpdoc\PhpdocSummaryFixer; use PhpCsFixer\Fixer\Phpdoc\PhpdocToCommentFixer; use PhpCsFixer\Fixer\PhpTag\BlankLineAfterOpeningTagFixer; +use PhpCsFixer\Fixer\StringNotation\ExplicitStringVariableFixer; use PhpCsFixer\Fixer\Whitespace\BlankLineBeforeStatementFixer; use Symplify\EasyCodingStandard\Config\ECSConfig; use Symplify\EasyCodingStandard\ValueObject\Set\SetList; @@ -43,11 +44,15 @@ $ruleConfigurations = [ [ IncrementStyleFixer::class, - ['style' => 'post'], + [ + 'style' => 'post', + ], ], [ CastSpacesFixer::class, - ['space' => 'none'], + [ + 'space' => 'none', + ], ], [ YodaStyleFixer::class, @@ -59,15 +64,21 @@ ], [ ConcatSpaceFixer::class, - ['spacing' => 'one'], + [ + 'spacing' => 'one', + ], ], [ CastSpacesFixer::class, - ['space' => 'none'], + [ + 'space' => 'none', + ], ], [ OrderedImportsFixer::class, - ['imports_order' => ['class', 'function', 'const']], + [ + 'imports_order' => ['class', 'function', 'const'], + ], ], [ NoSuperfluousPhpdocTagsFixer::class, @@ -79,15 +90,23 @@ ], [ DeclareEqualNormalizeFixer::class, - ['space' => 'single'], + [ + 'space' => 'single', + ], ], [ BlankLineBeforeStatementFixer::class, - ['statements' => ['continue', 'declare', 'return', 'throw', 'try']], + [ + 'statements' => ['continue', 'declare', 'return', 'throw', 'try'], + ], ], [ BinaryOperatorSpacesFixer::class, - ['operators' => ['&' => 'align']], + [ + 'operators' => [ + '&' => 'align', + ], + ], ], ]; @@ -106,5 +125,6 @@ PhpdocAlignFixer::class => null, PhpdocToCommentFixer::class => null, NativeFunctionInvocationFixer::class => null, + ExplicitStringVariableFixer::class => null, ]); }; diff --git a/src/Decorator/StopwatchDecorator.php b/src/Decorator/StopwatchDecorator.php index 8e6a63026..08b1c6319 100644 --- a/src/Decorator/StopwatchDecorator.php +++ b/src/Decorator/StopwatchDecorator.php @@ -207,8 +207,9 @@ private function generateProxyClass( $methods = $this->getProxyableMethods($reflection); $methodsCode = $this->generateProxyMethods($methods); - return " -class {$proxyClassName} extends {$originalClassName} { + return <<prefixInterceptors = \$prefixInterceptors; \$this->suffixInterceptors = \$suffixInterceptors; } -{$methodsCode} -}"; +$methodsCode +} +CODE; } /** @@ -241,15 +243,17 @@ private function generateProxyMethod(ReflectionMethod $method): string $methodName = $method->getName(); [$paramsList, $argsList] = $this->buildMethodParameters($method); [$returnType, $isVoid] = $this->getMethodReturnType($method); + $methodBody = $this->generateMethodBody($methodName, $argsList, $isVoid); + + return <<generateMethodBody($methodName, $argsList, $isVoid); - $code .= " }\n"; + public function $methodName($paramsList)$returnType { + if (isset(\$this->prefixInterceptors['$methodName'])) { + (\$this->prefixInterceptors['$methodName'])(); + } +$methodBody } - return $code; +CODE; } private function generateMethodBody(string $methodName, string $argsList, bool $isVoid): string @@ -261,26 +265,29 @@ private function generateMethodBody(string $methodName, string $argsList, bool $ private function generateVoidMethodBody(string $methodName, string $argsList): string { - $code = " \$this->wrappedInstance->{$methodName}({$argsList});\n"; - $code .= " if (isset(\$this->suffixInterceptors['{$methodName}'])) {\n"; - $code .= " \$returnValue = null;\n"; - $code .= " (\$this->suffixInterceptors['{$methodName}'])"; - $code .= "(null, null, null, func_get_args(), \$returnValue);\n"; - $code .= " }\n"; - - return $code; + return <<wrappedInstance->$methodName($argsList); + + if (isset(\$this->suffixInterceptors['$methodName'])) { + \$returnValue = null; + (\$this->suffixInterceptors['$methodName'])(null, null, null, func_get_args(), \$returnValue); + } + +CODE; } private function generateNonVoidMethodBody(string $methodName, string $argsList): string { - $code = " \$returnValue = \$this->wrappedInstance->{$methodName}({$argsList});\n"; - $code .= " if (isset(\$this->suffixInterceptors['{$methodName}'])) {\n"; - $code .= " (\$this->suffixInterceptors['{$methodName}'])"; - $code .= "(null, null, null, func_get_args(), \$returnValue);\n"; - $code .= " }\n"; - $code .= " return \$returnValue;\n"; - - return $code; + return <<wrappedInstance->$methodName($argsList); + + if (isset(\$this->suffixInterceptors['$methodName'])) { + (\$this->suffixInterceptors['$methodName'])(null, null, null, func_get_args(), \$returnValue); + } + + return \$returnValue; + +CODE; } // Method parameter handling From 58330b8990bde1440c915cb33e3fc431f9ccf1ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tarmo=20Lepp=C3=A4nen?= Date: Sun, 7 Dec 2025 20:25:30 +0200 Subject: [PATCH 06/10] Refactor - Improve reflection handling and enhance test coverage in StopwatchDecorator --- src/Decorator/StopwatchDecorator.php | 39 ++-------- .../Decorator/StopwatchDecoratorTest.php | 75 +++++++++++++++++++ 2 files changed, 83 insertions(+), 31 deletions(-) diff --git a/src/Decorator/StopwatchDecorator.php b/src/Decorator/StopwatchDecorator.php index 08b1c6319..b9c881002 100644 --- a/src/Decorator/StopwatchDecorator.php +++ b/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 $prefixInterceptors - * @param array $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 $prefixInterceptors * @param array $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; diff --git a/tests/Integration/Decorator/StopwatchDecoratorTest.php b/tests/Integration/Decorator/StopwatchDecoratorTest.php index 7ac21f758..8e9e3966c 100644 --- a/tests/Integration/Decorator/StopwatchDecoratorTest.php +++ b/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 $options + */ + public function methodWithInternalDefault(array $options = []): string + { + return 'test'; + } +} + From ea31806a9db3aaeef72d900874cec70b8c065790 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tarmo=20Lepp=C3=A4nen?= Date: Sun, 7 Dec 2025 20:38:46 +0200 Subject: [PATCH 07/10] Refactor - Add PHPCS disable/enable comments for multiple class declarations in StopwatchDecorator --- src/Decorator/StopwatchDecorator.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Decorator/StopwatchDecorator.php b/src/Decorator/StopwatchDecorator.php index b9c881002..3d1c033f6 100644 --- a/src/Decorator/StopwatchDecorator.php +++ b/src/Decorator/StopwatchDecorator.php @@ -189,6 +189,7 @@ private function generateProxyClass( $methods = $this->getProxyableMethods($reflection); $methodsCode = $this->generateProxyMethods($methods); + // phpcs:disable PSR1.Classes.ClassDeclaration.MultipleClasses return << Date: Sun, 7 Dec 2025 20:45:44 +0200 Subject: [PATCH 08/10] Refactor - Add helper classes for testing internal class defaults and final methods in StopwatchDecorator --- .../Decorator/FinalTestService.php | 21 +++++++++++ .../Decorator/ServiceWithInternalDefaults.php | 35 ++++++++++++++++++ .../Decorator/StopwatchDecoratorTest.php | 36 ------------------- 3 files changed, 56 insertions(+), 36 deletions(-) create mode 100644 tests/Integration/Decorator/FinalTestService.php create mode 100644 tests/Integration/Decorator/ServiceWithInternalDefaults.php diff --git a/tests/Integration/Decorator/FinalTestService.php b/tests/Integration/Decorator/FinalTestService.php new file mode 100644 index 000000000..9939f5349 --- /dev/null +++ b/tests/Integration/Decorator/FinalTestService.php @@ -0,0 +1,21 @@ + + */ + +namespace App\Tests\Integration\Decorator; + +/** + * Helper final class for testing + */ +final class FinalTestService +{ + public function testMethod(): string + { + return 'final-test'; + } +} + diff --git a/tests/Integration/Decorator/ServiceWithInternalDefaults.php b/tests/Integration/Decorator/ServiceWithInternalDefaults.php new file mode 100644 index 000000000..b031379e8 --- /dev/null +++ b/tests/Integration/Decorator/ServiceWithInternalDefaults.php @@ -0,0 +1,35 @@ + + */ + +namespace App\Tests\Integration\Decorator; + +use SplFileInfo; + +/** + * 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 $options + */ + public function methodWithInternalDefault(array $options = []): string + { + return 'test'; + } +} diff --git a/tests/Integration/Decorator/StopwatchDecoratorTest.php b/tests/Integration/Decorator/StopwatchDecoratorTest.php index 8e9e3966c..2a7021b95 100644 --- a/tests/Integration/Decorator/StopwatchDecoratorTest.php +++ b/tests/Integration/Decorator/StopwatchDecoratorTest.php @@ -203,39 +203,3 @@ 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 $options - */ - public function methodWithInternalDefault(array $options = []): string - { - return 'test'; - } -} - From 8a6d39101a3427a31b07264f5ac149fc6617deba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tarmo=20Lepp=C3=A4nen?= Date: Sun, 7 Dec 2025 21:00:39 +0200 Subject: [PATCH 09/10] Refactor - Remove unnecessary blank line in FinalTestService --- tests/Integration/Decorator/FinalTestService.php | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/Integration/Decorator/FinalTestService.php b/tests/Integration/Decorator/FinalTestService.php index 9939f5349..3760bb7cb 100644 --- a/tests/Integration/Decorator/FinalTestService.php +++ b/tests/Integration/Decorator/FinalTestService.php @@ -18,4 +18,3 @@ public function testMethod(): string return 'final-test'; } } - From 6ac956bca2c4a1c3466e1d3fc88858ec35867416 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tarmo=20Lepp=C3=A4nen?= Date: Sun, 7 Dec 2025 21:08:46 +0200 Subject: [PATCH 10/10] Refactor - Simplify ServiceWithInternalDefaults by removing unnecessary inheritance and constructor --- .../Decorator/ServiceWithInternalDefaults.php | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/tests/Integration/Decorator/ServiceWithInternalDefaults.php b/tests/Integration/Decorator/ServiceWithInternalDefaults.php index b031379e8..6d292aacb 100644 --- a/tests/Integration/Decorator/ServiceWithInternalDefaults.php +++ b/tests/Integration/Decorator/ServiceWithInternalDefaults.php @@ -8,19 +8,13 @@ namespace App\Tests\Integration\Decorator; -use SplFileInfo; - /** * Helper class for testing internal class default values - * Extends an internal class to potentially trigger reflection edge cases + * + * @psalm-suppress ClassMustBeFinal */ -class ServiceWithInternalDefaults extends SplFileInfo +class ServiceWithInternalDefaults { - 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