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/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 408db7b6d..3d1c033f6 100644 --- a/src/Decorator/StopwatchDecorator.php +++ b/src/Decorator/StopwatchDecorator.php @@ -9,94 +9,374 @@ 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; use Throwable; use function array_filter; +use function implode; use function is_object; use function str_contains; -use function str_starts_with; +use function str_replace; +use function uniqid; +use function var_export; /** * @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); - $className = $class->getName(); + $reflection = new ReflectionClass($service); - // Do not process core or extensions or already wrapped services - if ($class->getFileName() === false - || $class->isFinal() - || str_starts_with($class->getName(), 'ProxyManagerGeneratedProxy') - || str_contains($class->getName(), 'RequestStack') - || str_contains($class->getName(), 'Mock_') - ) { + if ($this->shouldSkipDecoration($reflection)) { return $service; } - [$prefixInterceptors, $suffixInterceptors] = $this->getPrefixAndSuffixInterceptors($class, $className); + [$prefixInterceptors, $suffixInterceptors] = $this->getPrefixAndSuffixInterceptors($reflection); - try { - $output = $this->factory->createProxy($service, $prefixInterceptors, $suffixInterceptors); - } catch (Throwable) { - $output = $service; - } + /** @var T */ + return $this->createProxy($service, $reflection, $prefixInterceptors, $suffixInterceptors) ?? $service; + } + + // Validation methods - return $output; + private function shouldSkipDecoration(ReflectionClass $class): bool + { + return $class->getFileName() === false + || $class->isFinal() + || $this->isExcludedClassName($class->getName()); + } + + private function isExcludedClassName(string $className): bool + { + return str_contains($className, 'RequestStack') + || str_contains($className, 'Mock_') + || str_contains($className, 'StopwatchProxy_'); } + // Interceptor creation methods + /** - * @return array{0: array, 1: array} + * @return array{0: array, 1: array} */ - private function getPrefixAndSuffixInterceptors(ReflectionClass $class, string $className): array + private function getPrefixAndSuffixInterceptors(ReflectionClass $class): array { + $className = $class->getName(); + $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 $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 + */ + 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 { + $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; + } + } + + // Proxy class generation methods + + private function generateProxyClass( + string $proxyClassName, + string $originalClassName, + ReflectionClass $reflection, + ): string { + $methods = $this->getProxyableMethods($reflection); + $methodsCode = $this->generateProxyMethods($methods); + + // phpcs:disable PSR1.Classes.ClassDeclaration.MultipleClasses + return <<wrappedInstance = \$wrappedInstance; + \$this->prefixInterceptors = \$prefixInterceptors; + \$this->suffixInterceptors = \$suffixInterceptors; + } +$methodsCode +} +CODE; + // phpcs:enable PSR1.Classes.ClassDeclaration.MultipleClasses + } + + /** + * @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); + $methodBody = $this->generateMethodBody($methodName, $argsList, $isVoid); + + return <<prefixInterceptors['$methodName'])) { + (\$this->prefixInterceptors['$methodName'])(); + } +$methodBody } + +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 + { + 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 + { + 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 + + /** + * @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 + { + $result = ''; + + if ($param->isDefaultValueAvailable()) { + /** @psalm-suppress MixedAssignment */ + $defaultValue = $param->getDefaultValue(); + $result = ' = ' . var_export($defaultValue, true); + } + + return $result; + } + + // Return type handling + + /** + * @return array{0: string, 1: bool} + */ + private function getMethodReturnType(ReflectionMethod $method): array + { + $returnType = ''; + $isVoid = false; + + if ($method->hasReturnType()) { + $type = $method->getReturnType(); + + if ($type !== null) { + $typeString = (string)$type; + $returnType = ': ' . $typeString; + $isVoid = $typeString === 'void'; + } + } + + return [$returnType, $isVoid]; + } + + // Method filtering + + /** + * @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(); + } } diff --git a/tests/Integration/Decorator/FinalTestService.php b/tests/Integration/Decorator/FinalTestService.php new file mode 100644 index 000000000..3760bb7cb --- /dev/null +++ b/tests/Integration/Decorator/FinalTestService.php @@ -0,0 +1,20 @@ + + */ + +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..6d292aacb --- /dev/null +++ b/tests/Integration/Decorator/ServiceWithInternalDefaults.php @@ -0,0 +1,29 @@ + + */ + +namespace App\Tests\Integration\Decorator; + +/** + * Helper class for testing internal class default values + * + * @psalm-suppress ClassMustBeFinal + */ +class ServiceWithInternalDefaults +{ + /** + * 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 3f2c80652..2a7021b95 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,70 @@ 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(); + $service = new class() { + final public function someMethod(): string + { + return 'test'; + } + }; - $factory = $this->getMockBuilder(AccessInterceptorValueHolderFactory::class) - ->disableOriginalConstructor() - ->getMock(); + $stopWatch = $this->getMockBuilder(Stopwatch::class)->disableOriginalConstructor()->getMock(); + + $decorator = new StopwatchDecorator($stopWatch, new NullLogger()); + + $result = $decorator->decorate($service); + + // Since the class has final methods, it should return the same instance + 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(); - $factory + $stopWatch ->expects($this->once()) - ->method('createProxy') - ->willThrowException(new Exception('foo')); + ->method('start'); + + $stopWatch + ->expects($this->once()) + ->method('stop'); + + $decorator = new StopwatchDecorator($stopWatch, new NullLogger()); - $decorator = new StopwatchDecorator($factory, $stopWatch); + $result = $decorator->decorate($service); - self::assertSame($service, $decorator->decorate($service)); + // The decorated service should work with methods that have internal class default values + self::assertSame('test', $result->methodWithInternalDefault()); } /** @@ -112,7 +146,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 +185,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 */ @@ -161,11 +195,11 @@ public function testThatDecoratorDoesNotTryToDecorateEntityObjects(): void } /** - * @return Generator + * @return Generator */ public static function dataProviderTestThatDecorateMethodReturnsExpected(): Generator { - yield [AccessInterceptorValueHolderInterface::class, new EntityReferenceExists()]; + yield [EntityReferenceExists::class, new EntityReferenceExists()]; yield [stdClass::class, new stdClass()]; } }