rekalogika / mapper

An object mapper for PHP and Symfony. Maps an object to another object. Primarily used for transforming an entity to a DTO and vice versa.
MIT License
27 stars 1 forks source link

[Backward campability] Error on mapping after bumping version #243

Closed gabplch closed 2 weeks ago

gabplch commented 1 month ago

HI! I've bumped to version 1.13 from 1.8. and started receiving some errors: When trying to map on no setters exists. This situation is possible for cases when id is generated with doctrine id generator, so setter is unnecessary here

...
class Foo
{
    #[ODM\Id(type: UuidType::NAME, options: ['class' => UuidGenerator::class], strategy: 'CUSTOM')]
    private UuidInterface $id;

   public function getId(): UuidInterface
   {...}
}

In this case mapper error is:

Rekalogika\Mapper\Transformer\Exception\NewInstanceReturnedButCannotBeSetOnTargetException: Transformation of property
"id" on object type "App\Foo" results in a different object instance from the original instance, but the new instance cannot be set
on the target object. You may wish to add a setter method to the target class. Mapping path: "(root)".

Expected behavior - the property will be ignored, and setter won't be called

How about to add smth like #[Skip] attribute, that will allow ignore property in mapping, or possibility create custom mapper on property, that will force not to map property

priyadi commented 1 month ago

Skipping the mapping is already possible with #[Map(null)] (or #[Map(false)] in the next release): https://rekalogika.dev/mapper/object/map#ignoring-a-property

Handling private properties without a setter turns out to be more complicated than I expected. I think I'll just replace the exception with a warning. If it is like your case, then it can be dismissed by adding #[Map(false)]. And those who really need a setter and forgot to add it can add the setter to dismiss the warning.

gabplch commented 1 month ago

Now there is no such an error, but it wants to set this value anyway, I've double checked, there is no setter for this property

My use case is:

class Source
{
    public function __construct(
        public string $id,
    ) {}
}

class Target
{
    private UuidInterface $id;

    private string $externalId;

    public function getId(): UuidInterface
    {....}

   public function getExternalId(): string
   {...}

   public function setExternalId(string $externalId): void
   {...}
}

class SourceToTargetMapper
{
    #[AsPropertyMapper(
        property: 'externalId',
        targetClass: Target::class,
    )]
    public function mapExternalId(Source $source): string
    {
        return $source->id;
    }
}
priyadi commented 1 month ago

What do you mean by "wants to set this value anyway"? As of 1.13.2, the exception is no longer there. Your case should work as before, but you will now get a warning in your log file. To dismiss the warning, you need to add #[Map(false)] on your $id property (on either source or target side).

Or do you get a different exception?


Why can't we just ignore target properties without a setter?

Previously, the latter case was ignored. But it led to cases where mappings were silently not working without any clue why that was happening. So I made it throw an exception when that happens, and some heuristics to ignore if the target is an immutable object like DateTime or Uuid.

But the heuristic is not perfect, and it led to confusion when Mapper correctly detected the class as immutable, and when it failed to do so. So, it appears that the safest way forward is to log it as a warning. If it is like your case, you need to add #[Map(false)] to dismiss the warning.

gabplch commented 4 weeks ago

The problem is it still tries (and makes it) to write from source to target even if there is no setter method. I've tried to clear cache, dump autoload, etc, and the problem still was there.

After adding #[Map(false)] attribute, problem disappeared, but I'd like not to reference on mapper from Domain layer.

priyadi commented 4 weeks ago

The problem is it still tries (and makes it) to write from source to target even if there is no setter method. I've tried to clear cache, dump autoload, etc, and the problem still was there.

Not sure if I understand. With v1.13.2, if there is no setId() on the target, Mapper should give up mapping source id to target id, with a warning in the log file.

After adding #[Map(false)] attribute, problem disappeared, but I'd like not to reference on mapper from Domain layer.

You should be able to add #[Map(false)] on either side, and it will work the same. In your case, I think you will want to add it on the source side, not the target side. Will that be sufficient for you?

priyadi commented 3 weeks ago

Hi, I think you might have gotten "CannotFindTransformerException: Cannot find a matching transformer for mapping the source types "null" to the target types "Uuid". If that was the case, then it should be fixed in the latest version.

gabplch commented 2 weeks ago

Hm, thanks, I'll chack later, as we decided to bump a bit later, as there are some troubles with migration to newer version. You can close the issue, I'll text, if it'll reproduce