diff --git a/CHANGELOG.md b/CHANGELOG.md index 57153c82..7f6f2ab0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ * fix: remove unalterable check in `ObjectProcessor` to allow transformers to handle the current target value in all cases * test: test lazy constructor +* fix: double mapping if a property is in the constructor and also has a setter ## 1.11.0 diff --git a/src/Transformer/Model/ConstructorArguments.php b/src/Transformer/Model/ConstructorArguments.php index f73dbf37..980dabd5 100644 --- a/src/Transformer/Model/ConstructorArguments.php +++ b/src/Transformer/Model/ConstructorArguments.php @@ -25,7 +25,7 @@ final class ConstructorArguments */ private array $contructorArguments = []; - private bool $variadicAdded = false; + private ?string $variadicName = null; /** * @var array @@ -34,7 +34,7 @@ final class ConstructorArguments public function addArgument(string $name, mixed $value): void { - if ($this->variadicAdded) { + if ($this->variadicName !== null) { throw new LogicException('Cannot add argument after variadic argument'); } @@ -44,14 +44,18 @@ public function addArgument(string $name, mixed $value): void /** * @param iterable $value */ - public function addVariadicArgument(iterable $value): void + public function addVariadicArgument(string $name, iterable $value): void { + if ($this->variadicName !== null) { + throw new LogicException('Cannot add variadic argument after variadic argument'); + } + + $this->variadicName = $name; + /** @var mixed $item */ foreach ($value as $item) { $this->contructorArguments[] = $item; } - - $this->variadicAdded = true; } public function addUnsetSourceProperty(string $name): void @@ -79,4 +83,24 @@ public function hasArguments(): bool { return $this->contructorArguments !== []; } + + /** + * @return list + */ + public function getArgumentNames(): array + { + $argumentNames = []; + + foreach (array_keys($this->contructorArguments) as $name) { + if (\is_string($name)) { + $argumentNames[] = $name; + } + } + + if ($this->variadicName !== null) { + $argumentNames[] = $this->variadicName; + } + + return $argumentNames; + } } diff --git a/src/TransformerProcessor/ObjectProcessor/ObjectProcessor.php b/src/TransformerProcessor/ObjectProcessor/ObjectProcessor.php index 75d8b362..fe3196c9 100644 --- a/src/TransformerProcessor/ObjectProcessor/ObjectProcessor.php +++ b/src/TransformerProcessor/ObjectProcessor/ObjectProcessor.php @@ -78,14 +78,17 @@ public function transform( extraTargetValues: $extraTargetValues, context: $context, ); + + $constructorArgumentNames = []; } else { - $target = $this->instantiateRealTarget( + [$target, $constructorArgumentNames] = $this->instantiateRealTarget( source: $source, extraTargetValues: $extraTargetValues, context: $context, ); } } else { + $constructorArgumentNames = []; $canUseTargetProxy = false; } @@ -111,6 +114,7 @@ public function transform( source: $source, target: $target, context: $context, + exclude: $constructorArgumentNames, ); } @@ -119,6 +123,7 @@ public function transform( target: $target, propertyMappings: $this->getPropertyMappings(), extraTargetValues: $extraTargetValues, + exclude: $constructorArgumentNames, context: $context, ); } @@ -188,12 +193,13 @@ class: $this->metadata->getTargetClass(), /** * @param array $extraTargetValues + * @return array{object,list} the object & the list of constructor arguments */ private function instantiateRealTarget( object $source, array $extraTargetValues, Context $context, - ): object { + ): array { $targetClass = $this->metadata->getTargetClass(); // check if class is valid & instantiable @@ -211,8 +217,10 @@ private function instantiateRealTarget( try { $reflectionClass = new \ReflectionClass($targetClass); - return $reflectionClass + $target = $reflectionClass ->newInstanceArgs($constructorArguments->getArguments()); + + return [$target, $constructorArguments->getArgumentNames()]; } catch (\TypeError | \ReflectionException $e) { throw new InstantiationFailureException( source: $source, @@ -244,15 +252,18 @@ private function instantiateTargetProxy( // create proxy initializer. this initializer will be executed when the // proxy is first accessed + $constructorArgumentNames = []; + $initializer = function (object $target) use ( $source, $context, $extraTargetValues, + &$constructorArgumentNames, ): void { // if the constructor is lazy, run it here if (!$this->metadata->constructorIsEager()) { - $target = $this->runConstructorManually( + $constructorArgumentNames = $this->runConstructorManually( source: $source, target: $target, extraTargetValues: $extraTargetValues, @@ -267,6 +278,7 @@ private function instantiateTargetProxy( target: $target, propertyMappings: $this->getLazyPropertyMappings(), extraTargetValues: $extraTargetValues, + exclude: $constructorArgumentNames, context: $context, ); }; @@ -282,7 +294,7 @@ class: $targetClass, // if the constructor is eager, run it here if ($this->metadata->constructorIsEager()) { - $target = $this->runConstructorManually( + $constructorArgumentNames = $this->runConstructorManually( source: $source, target: $target, extraTargetValues: $extraTargetValues, @@ -297,6 +309,7 @@ class: $targetClass, target: $target, propertyMappings: $this->getEagerPropertyMappings(), extraTargetValues: $extraTargetValues, + exclude: $constructorArgumentNames, context: $context, ); @@ -305,15 +318,16 @@ class: $targetClass, /** * @param array $extraTargetValues + * @return list the list of the constructor arguments */ private function runConstructorManually( object $source, object $target, array $extraTargetValues, Context $context, - ): object { + ): array { if (!method_exists($target, '__construct')) { - return $target; + return []; } $constructorArguments = $this->generateConstructorArguments( @@ -341,7 +355,7 @@ private function runConstructorManually( ); } - return $target; + return $constructorArguments->getArgumentNames(); } /** @@ -378,7 +392,10 @@ private function generateConstructorArguments( $targetPropertyValue = [$targetPropertyValue]; } - $constructorArguments->addVariadicArgument($targetPropertyValue); + $constructorArguments->addVariadicArgument( + $propertyMapping->getTargetProperty(), + $targetPropertyValue, + ); } else { $constructorArguments->addArgument( $propertyMapping->getTargetProperty(), @@ -417,15 +434,23 @@ private function generateConstructorArguments( /** * @param array $propertyMappings * @param array $extraTargetValues + * @param list $exclude */ private function readSourceAndWriteTarget( object $source, object $target, array $propertyMappings, array $extraTargetValues, + array $exclude, Context $context, ): object { foreach ($propertyMappings as $propertyMapping) { + $argumentName = $propertyMapping->getTargetProperty(); + + if (\in_array($argumentName, $exclude, true)) { + continue; + } + $target = $this->propertyProcessorFactory ->getPropertyProcessor($propertyMapping) ->readSourcePropertyAndWriteTargetProperty( @@ -443,6 +468,10 @@ private function readSourceAndWriteTarget( continue; } + if (\in_array($property, $exclude, true)) { + continue; + } + $propertyMapping = $propertyMappings[$property]; $target = $this->propertyProcessorFactory @@ -458,48 +487,58 @@ private function readSourceAndWriteTarget( return $target; } + /** + * @param list $exclude + */ private function mapDynamicProperties( object $source, object $target, + array $exclude, Context $context, ): void { $sourceProperties = $this->metadata->getSourceProperties(); /** @var mixed $sourcePropertyValue */ foreach (get_object_vars($source) as $sourceProperty => $sourcePropertyValue) { - if (!\in_array($sourceProperty, $sourceProperties, true)) { - try { - if (isset($target->{$sourceProperty})) { - /** @psalm-suppress MixedAssignment */ - $currentTargetPropertyValue = $target->{$sourceProperty}; - } else { - $currentTargetPropertyValue = null; - } + if (\in_array($sourceProperty, $sourceProperties, true)) { + continue; + } + if (\in_array($sourceProperty, $exclude, true)) { + continue; + } - if ( - $currentTargetPropertyValue === null - || \is_scalar($currentTargetPropertyValue) - ) { - /** @psalm-suppress MixedAssignment */ - $targetPropertyValue = $sourcePropertyValue; - } else { - /** @var mixed */ - $targetPropertyValue = $this->mainTransformer->transform( - source: $sourcePropertyValue, - target: $currentTargetPropertyValue, - sourceType: null, - targetTypes: [], - context: $context, - path: $sourceProperty, - ); - } - } catch (\Throwable) { - $targetPropertyValue = null; + try { + if (isset($target->{$sourceProperty})) { + /** @psalm-suppress MixedAssignment */ + $currentTargetPropertyValue = $target->{$sourceProperty}; + } else { + $currentTargetPropertyValue = null; } - $target->{$sourceProperty} = $targetPropertyValue; + + if ( + $currentTargetPropertyValue === null + || \is_scalar($currentTargetPropertyValue) + ) { + /** @psalm-suppress MixedAssignment */ + $targetPropertyValue = $sourcePropertyValue; + } else { + /** @var mixed */ + $targetPropertyValue = $this->mainTransformer->transform( + source: $sourcePropertyValue, + target: $currentTargetPropertyValue, + sourceType: null, + targetTypes: [], + context: $context, + path: $sourceProperty, + ); + } + } catch (\Throwable) { + $targetPropertyValue = null; } + + $target->{$sourceProperty} = $targetPropertyValue; } } } diff --git a/tests/config/rekalogika-mapper/generated-mappings.php b/tests/config/rekalogika-mapper/generated-mappings.php index f1b033c7..795be04b 100644 --- a/tests/config/rekalogika-mapper/generated-mappings.php +++ b/tests/config/rekalogika-mapper/generated-mappings.php @@ -291,6 +291,30 @@ target: \Rekalogika\Mapper\Tests\Fixtures\Basic\PersonDto::class ); + $mappingCollection->addObjectMapping( + // tests/src/IntegrationTest/ConstructorAndPropertyTest.php on line 77 + source: \Rekalogika\Mapper\Tests\Fixtures\ConstructorAndProperty\SourceObject::class, + target: \Rekalogika\Mapper\Tests\Fixtures\ConstructorAndProperty\ObjectWithConstructorAndSetter::class + ); + + $mappingCollection->addObjectMapping( + // tests/src/IntegrationTest/ConstructorAndPropertyTest.php on line 32 + source: \Rekalogika\Mapper\Tests\Fixtures\ConstructorAndProperty\SourceObject::class, + target: \Rekalogika\Mapper\Tests\Fixtures\ConstructorAndProperty\ObjectWithConstructorArgumentsAndGetters::class + ); + + $mappingCollection->addObjectMapping( + // tests/src/IntegrationTest/ConstructorAndPropertyTest.php on line 64 + source: \Rekalogika\Mapper\Tests\Fixtures\ConstructorAndProperty\SourceObject::class, + target: \Rekalogika\Mapper\Tests\Fixtures\ConstructorAndProperty\ObjectWithConstructorArgumentsAndPublicProperties::class + ); + + $mappingCollection->addObjectMapping( + // tests/src/IntegrationTest/ConstructorAndPropertyTest.php on line 48 + source: \Rekalogika\Mapper\Tests\Fixtures\ConstructorAndProperty\SourceObject::class, + target: \Rekalogika\Mapper\Tests\Fixtures\ConstructorAndProperty\ObjectWithIdOnlyOnConstructor::class + ); + $mappingCollection->addObjectMapping( // tests/src/IntegrationTest/DateTimeMappingTest.php on line 195 source: \Rekalogika\Mapper\Tests\Fixtures\DateTime\DateTimeTestObjectInterface::class, diff --git a/tests/src/Fixtures/ConstructorAndProperty/ObjectWithConstructorAndSetter.php b/tests/src/Fixtures/ConstructorAndProperty/ObjectWithConstructorAndSetter.php new file mode 100644 index 00000000..e90f8295 --- /dev/null +++ b/tests/src/Fixtures/ConstructorAndProperty/ObjectWithConstructorAndSetter.php @@ -0,0 +1,53 @@ + + * + * For the full copyright and license information, please view the LICENSE file + * that was distributed with this source code. + */ + +namespace Rekalogika\Mapper\Tests\Fixtures\ConstructorAndProperty; + +class ObjectWithConstructorAndSetter +{ + public function __construct( + private string $id, + private string $name, + private string $description, + ) {} + + public function setId(string $id): void + { + throw new \LogicException('This method should not be called'); + } + + public function setName(string $name): void + { + throw new \LogicException('This method should not be called'); + } + + public function setDescription(string $description): void + { + throw new \LogicException('This method should not be called'); + } + + public function getId(): string + { + return $this->id; + } + + public function getName(): string + { + return $this->name; + } + + public function getDescription(): string + { + return $this->description; + } +} diff --git a/tests/src/Fixtures/ConstructorAndProperty/ObjectWithConstructorArgumentsAndGetters.php b/tests/src/Fixtures/ConstructorAndProperty/ObjectWithConstructorArgumentsAndGetters.php new file mode 100644 index 00000000..d42feb92 --- /dev/null +++ b/tests/src/Fixtures/ConstructorAndProperty/ObjectWithConstructorArgumentsAndGetters.php @@ -0,0 +1,38 @@ + + * + * For the full copyright and license information, please view the LICENSE file + * that was distributed with this source code. + */ + +namespace Rekalogika\Mapper\Tests\Fixtures\ConstructorAndProperty; + +class ObjectWithConstructorArgumentsAndGetters +{ + public function __construct( + private string $id, + private string $name, + private string $description, + ) {} + + public function getId(): string + { + return $this->id; + } + + public function getName(): string + { + return $this->name; + } + + public function getDescription(): string + { + return $this->description; + } +} diff --git a/tests/src/Fixtures/ConstructorAndProperty/ObjectWithConstructorArgumentsAndPublicProperties.php b/tests/src/Fixtures/ConstructorAndProperty/ObjectWithConstructorArgumentsAndPublicProperties.php new file mode 100644 index 00000000..e3bb78c3 --- /dev/null +++ b/tests/src/Fixtures/ConstructorAndProperty/ObjectWithConstructorArgumentsAndPublicProperties.php @@ -0,0 +1,23 @@ + + * + * For the full copyright and license information, please view the LICENSE file + * that was distributed with this source code. + */ + +namespace Rekalogika\Mapper\Tests\Fixtures\ConstructorAndProperty; + +class ObjectWithConstructorArgumentsAndPublicProperties +{ + public function __construct( + public string $id, + public string $name, + public string $description, + ) {} +} diff --git a/tests/src/Fixtures/ConstructorAndProperty/ObjectWithIdOnlyOnConstructor.php b/tests/src/Fixtures/ConstructorAndProperty/ObjectWithIdOnlyOnConstructor.php new file mode 100644 index 00000000..1ac5b43a --- /dev/null +++ b/tests/src/Fixtures/ConstructorAndProperty/ObjectWithIdOnlyOnConstructor.php @@ -0,0 +1,49 @@ + + * + * For the full copyright and license information, please view the LICENSE file + * that was distributed with this source code. + */ + +namespace Rekalogika\Mapper\Tests\Fixtures\ConstructorAndProperty; + +class ObjectWithIdOnlyOnConstructor +{ + public function __construct( + private string $id, + ) {} + + private string $name = ''; + private string $description = ''; + + public function getId(): string + { + return $this->id; + } + + public function getName(): string + { + return $this->name; + } + + public function setName(string $name): void + { + $this->name = $name; + } + + public function getDescription(): string + { + return $this->description; + } + + public function setDescription(string $description): void + { + $this->description = $description; + } +} diff --git a/tests/src/Fixtures/ConstructorAndProperty/SourceObject.php b/tests/src/Fixtures/ConstructorAndProperty/SourceObject.php new file mode 100644 index 00000000..43c497a1 --- /dev/null +++ b/tests/src/Fixtures/ConstructorAndProperty/SourceObject.php @@ -0,0 +1,21 @@ + + * + * For the full copyright and license information, please view the LICENSE file + * that was distributed with this source code. + */ + +namespace Rekalogika\Mapper\Tests\Fixtures\ConstructorAndProperty; + +class SourceObject +{ + public string $id = 'id'; + public string $name = 'name'; + public string $description = 'description'; +} diff --git a/tests/src/IntegrationTest/ConstructorAndPropertyTest.php b/tests/src/IntegrationTest/ConstructorAndPropertyTest.php new file mode 100644 index 00000000..2b96224e --- /dev/null +++ b/tests/src/IntegrationTest/ConstructorAndPropertyTest.php @@ -0,0 +1,86 @@ + + * + * For the full copyright and license information, please view the LICENSE file + * that was distributed with this source code. + */ + +namespace Rekalogika\Mapper\Tests\IntegrationTest; + +use Rekalogika\Mapper\Tests\Common\FrameworkTestCase; +use Rekalogika\Mapper\Tests\Fixtures\ConstructorAndProperty\ObjectWithConstructorAndSetter; +use Rekalogika\Mapper\Tests\Fixtures\ConstructorAndProperty\ObjectWithConstructorArgumentsAndGetters; +use Rekalogika\Mapper\Tests\Fixtures\ConstructorAndProperty\ObjectWithConstructorArgumentsAndPublicProperties; +use Rekalogika\Mapper\Tests\Fixtures\ConstructorAndProperty\ObjectWithIdOnlyOnConstructor; +use Rekalogika\Mapper\Tests\Fixtures\ConstructorAndProperty\SourceObject; +use Symfony\Component\VarExporter\LazyObjectInterface; + +class ConstructorAndPropertyTest extends FrameworkTestCase +{ + /** + * lazy constructor arguments + */ + public function testToConstructorArgumentsAndGetters(): void + { + $source = new SourceObject(); + $result = $this->mapper->map($source, ObjectWithConstructorArgumentsAndGetters::class); + + $this->assertInstanceOf(LazyObjectInterface::class, $result); + $this->assertFalse($result->isLazyObjectInitialized()); + + $this->assertSame('id', $result->getId()); + $this->assertSame('name', $result->getName()); + $this->assertSame('description', $result->getDescription()); + } + + /** + * eager constructor arguments + */ + public function testToIdOnlyOnConstructor(): void + { + $source = new SourceObject(); + $result = $this->mapper->map($source, ObjectWithIdOnlyOnConstructor::class); + + $this->assertInstanceOf(LazyObjectInterface::class, $result); + $this->assertFalse($result->isLazyObjectInitialized()); + + $this->assertSame('id', $result->getId()); + $this->assertSame('name', $result->getName()); + $this->assertSame('description', $result->getDescription()); + } + + /** + * eager constructor arguments + */ + public function testToConstructorArgumentsAndPublicProperties(): void + { + $source = new SourceObject(); + $result = $this->mapper->map($source, ObjectWithConstructorArgumentsAndPublicProperties::class); + + $this->assertInstanceOf(LazyObjectInterface::class, $result); + $this->assertFalse($result->isLazyObjectInitialized()); + + $this->assertSame('id', $result->id); + $this->assertSame('name', $result->name); + $this->assertSame('description', $result->description); + } + + public function testToConstructorAndSetter(): void + { + $source = new SourceObject(); + $result = $this->mapper->map($source, ObjectWithConstructorAndSetter::class); + + $this->assertInstanceOf(LazyObjectInterface::class, $result); + $this->assertFalse($result->isLazyObjectInitialized()); + + $this->assertSame('id', $result->getId()); + $this->assertSame('name', $result->getName()); + $this->assertSame('description', $result->getDescription()); + } +} diff --git a/tests/src/IntegrationTest/LazyObjectTest.php b/tests/src/IntegrationTest/LazyObjectTest.php index cfd1fe50..de17673d 100644 --- a/tests/src/IntegrationTest/LazyObjectTest.php +++ b/tests/src/IntegrationTest/LazyObjectTest.php @@ -18,8 +18,8 @@ use Rekalogika\Mapper\Tests\Common\FrameworkTestCase; use Rekalogika\Mapper\Tests\Fixtures\LazyObject\ChildObjectWithIdDto; use Rekalogika\Mapper\Tests\Fixtures\LazyObject\ObjectWithId; -use Rekalogika\Mapper\Tests\Fixtures\LazyObject\ObjectWithIdAndNameInConstructorDto; use Rekalogika\Mapper\Tests\Fixtures\LazyObject\ObjectWithIdAndName; +use Rekalogika\Mapper\Tests\Fixtures\LazyObject\ObjectWithIdAndNameInConstructorDto; use Rekalogika\Mapper\Tests\Fixtures\LazyObject\ObjectWithIdDto; use Rekalogika\Mapper\Tests\Fixtures\LazyObject\ObjectWithIdEagerDto; use Rekalogika\Mapper\Tests\Fixtures\LazyObject\ObjectWithIdFinalDto;